Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins
Date: Fri Jun 28 2019 - 04:09:59 EST
On Thu, Jun 27, 2019 at 11:16 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> Quoting Brendan Higgins (2019-06-26 16:00:40)
> > On Tue, Jun 25, 2019 at 8:41 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> > > scenario like below, but where it is a problem. There could be three
> > > CPUs, or even one CPU and three threads if you want to describe the
> > > extra thread scenario.
> > >
> > > Here's my scenario where it isn't needed:
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > kunit_run_test(&test)
> > > test_case_func()
> > > ....
> > > [mock hardirq]
> > > kunit_set_success(&test)
> > > [hardirq ends]
> > > ...
> > > complete(&test_done)
> > > wait_for_completion(&test_done)
> > > kunit_get_success(&test)
> > >
> > > We don't need to care about having locking here because success or
> > > failure only happens in one place and it's synchronized with the
> > > completion.
> > Here is the scenario I am concerned about:
> > CPU0 CPU1 CPU2
> > ---- ---- ----
> > kunit_run_test(&test)
> > test_case_func()
> > ....
> > schedule_work(foo_func)
> > [mock hardirq] foo_func()
> > ... ...
> > kunit_set_success(false) kunit_set_success(false)
> > [hardirq ends] ...
> > ...
> > complete(&test_done)
> > wait_for_completion(...)
> > kunit_get_success(&test)
> > In my scenario, since both CPU1 and CPU2 update the success status of
> > the test simultaneously, even though they are setting it to the same
> > value. If my understanding is correct, this could result in a
> > write-tear on some architectures in some circumstances. I suppose we
> > could just make it an atomic boolean, but I figured locking is also
> > fine, and generally preferred.
> This is what we have WRITE_ONCE() and READ_ONCE() for. Maybe you could
> just use that in the getter and setters and remove the lock if it isn't
> used for anything else.
> It may also be a good idea to have a kunit_fail_test() API that fails
> the test passed in with a WRITE_ONCE(false). Otherwise, the test is
> assumed successful and it isn't even possible for a test to change the
> state from failure to success due to a logical error because the API
> isn't available. Then we don't really need to have a generic
> kunit_set_success() function at all. We could have a kunit_test_failed()
> function too that replaces the kunit_get_success() function. That would
> read better in an if condition.
You know what, I think you are right.
Sorry, for not realizing this earlier, I think you mentioned something
along these lines a long time ago.
Thanks for your patience!
> > Also, to be clear, I am onboard with dropping then IRQ stuff for now.
> > I am fine moving to a mutex for the time being.