On Wed, Sep 02, 2020 at 08:13:15AM +0200, Marco Elver wrote:
> On Wed, Sep 02, 2020 at 11:30AM +0800, Boqun Feng wrote:
> > Hi Paul and Marco,
> >
> > The whole update patchset looks good to me, just one question out of
> > curiosity fo this one, please see below:
> >
> > On Mon, Aug 31, 2020 at 11:18:04AM -0700, paulmck@xxxxxxxxxx wrote:
> > > From: Marco Elver <elver@xxxxxxxxxx>
> > >
> > > Previous to the change to distinguish read-write accesses, when
> > > CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=y is set, KCSAN would consider
> > > the non-atomic bitops as atomic. We want to partially revert to this
> > > behaviour, but with one important distinction: report racing
> > > modifications, since lost bits due to non-atomicity are certainly
> > > possible.
> > >
> > > Given the operations here only modify a single bit, assuming
> > > non-atomicity of the writer is sufficient may be reasonable for certain
> > > usage (and follows the permissible nature of the "assume plain writes
> > > atomic" rule). In other words:
> > >
> > > 1. We want non-atomic read-modify-write races to be reported;
> > > this is accomplished by kcsan_check_read(), where any
> > > concurrent write (atomic or not) will generate a report.
> > >
> > > 2. We do not want to report races with marked readers, but -do-
> > > want to report races with unmarked readers; this is
> > > accomplished by the instrument_write() ("assume atomic
> > > write" with Kconfig option set).
> > >
> >
> > Is there any code in kernel using the above assumption (i.e.
> > non-atomicity of the writer is sufficient)? IOW, have you observed
> > anything bad (e.g. an anoying false positive) after applying the
> > read_write changes but without this patch?
>
> We were looking for an answer to:
>
> https://lkml.kernel.org/r/20200810124516.GM17456@xxxxxxxxxxxxxxxxxxxx
>
> Initially we thought using atomic bitops might be required, but after a
> longer offline discussion realized that simply marking the reader in
> this case, but retaining the non-atomic bitop is probably all that's
> needed.
>
> The version of KCSAN that found the above was still using KCSAN from
> Linux 5.8, but we realized with the changed read-write instrumentation
> to bitops in this series, we'd regress and still report the race even if
> the reader was marked. To avoid this with the default KCSAN config, we
> determined that we need the patch here.
>
Thanks for the background! Now I see the point of having this patch ;-)
FWIW, feel free to add for the whole series:
Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
Regards,
Boqun
> The bitops are indeed a bit more special, because for both the atomic
> and non-atomic bitops we *can* reason about the generated code (since we
> control it, although not sure about the asm-generic ones), and that
> makes reasoning about accesses racing with non-atomic bitops more
> feasible. At least that's our rationale for deciding that reverting
> non-atomic bitops treatment to it's more relaxed version is ok.
>
> Thanks,
> -- Marco
