On Fri, Jul 23, 2021 at 12:59:47PM -0400, Alan Stern wrote:
> On Fri, Jul 23, 2021 at 09:24:31AM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 22, 2021 at 10:08:46PM -0400, Alan Stern wrote:
>
> > > > + void do_something_locked(struct foo *fp)
> > > > + {
> > > > + bool gf = true;
> > > > +
> > > > + /* IMPORTANT: Heuristic plus spin_lock()! */
> > > > + if (!data_race(global_flag)) {
> > > > + spin_lock(&fp->f_lock);
> > > > + if (!smp_load_acquire(&global_flag)) {
>
> > > > + void begin_global(void)
> > > > + {
> > > > + int i;
> > > > +
> > > > + spin_lock(&global_lock);
> > > > + WRITE_ONCE(global_flag, true);
> > >
> > > Why does this need to be WRITE_ONCE? It still races with the first read
> > > of global_flag above.
> >
> > But also with the smp_load_acquire() of global_flag, right?
>
> What I'm curious about is why, given these two races, you notate one of
> them by changing a normal write to WRITE_ONCE and you notate the other
> by changing a normal read to a data_race() read. Why not handle them
> both the same way?
Because the code can tolerate the first read returning complete nonsense,
but needs the value from the second read to be exact at that point in
time. (If the value changes immediately after being read, the fact that
->f_lock is held prevents begin_global() from completing.)
Thanx, Paul
