Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Stephen Boyd
Date: Thu Jun 27 2019 - 14:16:40 EST
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
> ---- ---- ----
> [mock hardirq] foo_func()
> ... ...
> kunit_set_success(false) kunit_set_success(false)
> [hardirq ends] ...
> 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.
> 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.