Re: [PATCH v3] mm: annotate data-race in cpu_needs_drain()

[Pedro Falcato]

On Mon, Jun 29, 2026 at 05:45:22PM +0100, Lorenzo Stoakes wrote:
> On Mon, Jun 29, 2026 at 09:34:02AM -0700, Andrew Morton wrote:
> > On Mon, 29 Jun 2026 11:23:26 +0100 Lorenzo Stoakes <ljs@xxxxxxxxxx> wrote:
> >
> > > > >  		folio_batch_count(&fbatches->lru_lazyfree) ||
> > > > >  		folio_batch_count(&fbatches->lru_activate) ||
> > > > > -		need_mlock_drain(cpu) ||
> > > > > +		need_mlock_drain(cpu)) ||
> > > >
> > > > The indentation is a bit suboptimal now.
> > > >
> > > > Would read nicer as
> > > >
> > > > diff --git a/mm/swap.c b/mm/swap.c
> > > > index 588f50d8f1a8c..5958e6fdd3593 100644
> > > > --- a/mm/swap.c
> > > > +++ b/mm/swap.c
> > > > @@ -828,13 +828,13 @@ static bool cpu_needs_drain(unsigned int cpu)
> > > >         struct cpu_fbatches *fbatches = &per_cpu(cpu_fbatches, cpu);
> > > >
> > > >         /* Check these in order of likelihood that they're not zero */
> > > > -       return folio_batch_count(&fbatches->lru_add) ||
> > > > -               folio_batch_count(&fbatches->lru_move_tail) ||
> > > > -               folio_batch_count(&fbatches->lru_deactivate_file) ||
> > > > -               folio_batch_count(&fbatches->lru_deactivate) ||
> > > > -               folio_batch_count(&fbatches->lru_lazyfree) ||
> > > > -               folio_batch_count(&fbatches->lru_activate) ||
> > > > -               need_mlock_drain(cpu) ||
> > > > +       return data_race(folio_batch_count(&fbatches->lru_add) ||
> > > > +                        folio_batch_count(&fbatches->lru_move_tail) ||
> > > > +                        folio_batch_count(&fbatches->lru_deactivate_file) ||
> > > > +                        folio_batch_count(&fbatches->lru_deactivate) ||
> > > > +                        folio_batch_count(&fbatches->lru_lazyfree) ||
> > > > +                        folio_batch_count(&fbatches->lru_activate) ||
> > > > +                        need_mlock_drain(cpu)) ||
> > > >                 has_bh_in_lru(cpu, NULL);
> > >
> > > Yeah that works for me.
> > >
> > > Andrew - maybe easier if you fix that up? :)
> > >
> >
> > Sure.
> >
> > --- a/mm/swap.c~mm-annotate-data-race-in-cpu_needs_drain-fix
> > +++ a/mm/swap.c
> > @@ -832,12 +832,12 @@ static bool cpu_needs_drain(unsigned int
> >
> >  	/* Check these in order of likelihood that they're not zero */
> >  	return data_race(folio_batch_count(&fbatches->lru_add) ||
> > -		folio_batch_count(&fbatches->lru_move_tail) ||
> > -		folio_batch_count(&fbatches->lru_deactivate_file) ||
> > -		folio_batch_count(&fbatches->lru_deactivate) ||
> > -		folio_batch_count(&fbatches->lru_lazyfree) ||
> > -		folio_batch_count(&fbatches->lru_activate) ||
> > -		need_mlock_drain(cpu)) ||
> > +			 folio_batch_count(&fbatches->lru_move_tail) ||
> > +			 folio_batch_count(&fbatches->lru_deactivate_file) ||
> > +			 folio_batch_count(&fbatches->lru_deactivate) ||
> > +			 folio_batch_count(&fbatches->lru_lazyfree) ||
> > +			 folio_batch_count(&fbatches->lru_activate) ||
> > +			 need_mlock_drain(cpu)) ||
> >  		has_bh_in_lru(cpu, NULL);
> >  }
> >
> >
> > The removal of data_race() in need_mlock_drain() is a little worrisome
> > - perhaps any future callers would have needed it?
> >
> > need_mlock_drain() has only a single caller.  How about I remove it and
> > open-code it within cpu_needs_drain()?
> 
> That references a static per-CPU variable (mlock_fbatch) in mlock.c's
> compilation unit so I think it has to stay as it us unfortunately.
> 
> And it's better I think to only use the data_race() here where we definitely
> know we need it (and as the only instance of that).

I agree. FWIW something that would perhaps be nice to pull off would be:
lockdep_assert_held_or_data_race(), or something with a less excrutiating
name. Which could assert "either we hold a lock, or the caller is aware
that this can race". Which sounds nice. In that case, we could simply
slap that on need_mlock_drain() as requiring the mlock_fbatch's local lock,
or data_race().

It night be too special purpose though.

-- 
Pedro