Quoting Brendan Higgins (2019-08-13 00:52:03)
On Mon, Aug 12, 2019 at 10:56 PM Stephen Boyd sboyd@kernel.org wrote:
Quoting Brendan Higgins (2019-08-12 21:57:55)
On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd sboyd@kernel.org wrote:
Quoting Brendan Higgins (2019-08-12 11:24:12)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 2625bcfeb19ac..93381f841e09f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -184,6 +191,13 @@ struct kunit { struct list_head resources; /* Protected by lock. */ };
+static inline void kunit_set_death_test(struct kunit *test, bool death_test) +{
spin_lock(&test->lock);test->death_test = death_test;spin_unlock(&test->lock);+}
These getters and setters are using spinlocks again. It doesn't make any sense. It probably needs a rework like was done for the other bool member, success.
No, this is intentional. death_test can transition from false to true and then back to false within the same test. Maybe that deserves a comment?
Yes. How does it transition from true to false again?
The purpose is to tell try_catch that it was expected for the test to bail out. Given the default implementation there is no way for this to happen aside from abort() being called, but in some implementations it is possible to implement a fault catcher which allows a test suite to recover from an unexpected failure.
Maybe it would be best to drop this until I add one of those alternative implementations.
Ok.
Either way, having a spinlock around a read/write API doesn't make sense because it just makes sure that two writes don't overlap, but otherwise does nothing to keep things synchronized. For example a set to true after a set to false when the two calls to set true or false aren't synchronized means they can happen in any order. So I don't see how it needs a spinlock. The lock needs to be one level higher.
There shouldn't be any cases where one thread is trying to set it while another is trying to unset it. The thing I am worried about here is making sure all the cores see the write, and making sure no reads or writes get reordered before it. So I guess I just want a fence. So I take it I should probably have is a WRITE_ONCE here and READ_ONCE in the getter?
Are the gets and sets in program order? If so, WRITE_ONCE and READ_ONCE aren't required. Otherwise, if it's possible for one thread to write it and another to read it but the threads are ordered with some other barrier like a completion or lock, then again the macros aren't needed. It would be good to read memory-barriers.txt to understand when to use the read/write macros.