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

[Lorenzo Stoakes]

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).

Cheers, Lorenzo