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. > > > > Ok. Thanks!
