On Tue, 31 Mar 2020 at 15:10, Will Deacon <will@xxxxxxxxxx> wrote:
>
> On Tue, Mar 24, 2020 at 05:23:30PM +0100, Marco Elver wrote:
> > On Tue, 24 Mar 2020 at 16:37, Will Deacon <will@xxxxxxxxxx> wrote:
> > > Some list predicates can be used locklessly even with the non-RCU list
> > > implementations, since they effectively boil down to a test against
> > > NULL. For example, checking whether or not a list is empty is safe even
> > > in the presence of a concurrent, tearing write to the list head pointer.
> > > Similarly, checking whether or not an hlist node has been hashed is safe
> > > as well.
> > >
> > > Annotate these lockless list predicates with data_race() and READ_ONCE()
> > > so that KCSAN and the compiler are aware of what's going on. The writer
> > > side can then avoid having to use WRITE_ONCE() in the non-RCU
> > > implementation.
> > >
> > > Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > Cc: Marco Elver <elver@xxxxxxxxxx>
> > > Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> > > ---
> > > include/linux/list.h | 10 +++++-----
> > > include/linux/list_bl.h | 5 +++--
> > > include/linux/list_nulls.h | 6 +++---
> > > include/linux/llist.h | 2 +-
> > > 4 files changed, 12 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/include/linux/list.h b/include/linux/list.h
> > > index 4fed5a0f9b77..4d9f5f9ed1a8 100644
> > > --- a/include/linux/list.h
> > > +++ b/include/linux/list.h
> > > @@ -279,7 +279,7 @@ static inline int list_is_last(const struct list_head *list,
> > > */
> > > static inline int list_empty(const struct list_head *head)
> > > {
> > > - return READ_ONCE(head->next) == head;
> > > + return data_race(READ_ONCE(head->next) == head);
> >
> > Double-marking should never be necessary, at least if you want to make
> > KCSAN happy. From what I gather there is an unmarked write somewhere,
> > correct? In that case, KCSAN will still complain because if it sees a
> > race between this read and the other write, then at least one is still
> > plain (the write).
>
> Ok, then I should drop the data_race() annotation and stick to READ_ONCE(),
> I think (but see below).
>
> > Then, my suggestion would be to mark the write with data_race() and
> > just leave this as a READ_ONCE(). Having a data_race() somewhere only
> > makes KCSAN stop reporting the race if the paired access is also
> > marked (be it with data_race() or _ONCE, etc.).
>
> The problem with taking that approach is that it ends up much of the
> list implementation annotated with either WRITE_ONCE() or data_race(),
> meaning that concurrent, racy list operations will no longer be reported
> by KCSAN. I think that's a pretty big deal and I'm strongly against
> annotating the internals of library code such as this because it means
> that buggy callers will largely go undetected.
>
> The situation we have here is that some calls, e.g. hlist_empty() are
> safe even in the presence of a racy write and I'd like to suppress KCSAN
> reports without annotating the writes at all.
>
> > Alternatively, if marking the write is impossible, you can surround
> > the access with kcsan_disable_current()/kcsan_enable_current(). Or, as
> > a last resort, just leaving as-is is fine too, because KCSAN's default
> > config (still) has KCSAN_ASSUME_PLAIN_WRITES_ATOMIC selected.
>
> Hmm, I suppose some bright spark will want to change the default at the some
> point though, no? ;) I'll look at using
> kcsan_disable_current()/kcsan_enable_current(), thanks.
I think this will come up again (it did already come up in some other
patch I reviewed, and Paul also mentioned it), so it seems best to
change data_race() to match the intuitive semantics of just completely
ignoring the access marked with it. I.e. marking accesses racing with
accesses marked with data_race() is now optional:
https://lkml.kernel.org/r/20200331193233.15180-1-elver@xxxxxxxxxx
In which case, the original patch you had here works just fine.
Thanks,
-- Marco
