Re: [PATCH v5 4/4] percpu: Fix hint invariant breakage

[Joonwon Kang <joonwonkang@google.com>]

> On Sun, 10 May 2026 07:21:49 +0000 Joonwon Kang <joonwonkang@google.com> wrote:
> 
> > The invariant "scan_hint_start > contig_hint_start if and only if
> > scan_hint == contig_hint" should be kept for hint management. However,
> > it could be broken in some cases:
> > 
> >   - if (new contig == contig_hint == scan_hint) && (contig_hint_start <
> >     scan_hint_start < new contig start) && the new contig is to become a
> >     new contig_hint due to its better alignment, then scan_hint should
> >     be invalidated instead of keeping the old value.
> > 
> >   - if (new contig == contig_hint > scan_hint) && (new contig start <
> >     contig_hint_start) && the new contig is not to become a new
> >     contig_hint, then scan_hint should be not updated to the new contig.
> > 
> > This commit mainly fixes this invariant breakage and includes more:
> 
> Thanks.  AI review asked a single question:
> 	https://sashiko.dev/#/patchset/20260510072149.1279887-1-joonwonkang@google.com

Thanks for sharing it. Let me answer it inline.

> > diff --git a/mm/percpu.c b/mm/percpu.c
> > index 35e058f56a6fd..359f92d9637db 100644
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> [ ... ]
> > @@ -661,21 +710,41 @@ static void pcpu_block_update(struct pcpu_block_md *block, int start, int end)
> >  		if (block->contig_hint.start &&
> >  		    (!free.start ||
> >  		     __ffs(free.start) > __ffs(block->contig_hint.start))) {
> [ ... ]
> >  			/* new start has a better alignment so use it */
> >  			block->contig_hint.start = free.start;
> > -			if (block->scan_hint.size &&
> > -			    free.start < block->scan_hint.start &&
> > -			    block->contig_hint.size > block->scan_hint.size)
> > -				block->scan_hint.size = 0;
> > -		} else if ((block->scan_hint.size &&
> > -			    free.start > block->scan_hint.start) ||
> > -			   block->contig_hint.size > block->scan_hint.size) {
> > -			/*
> > -			 * Knowing new contig size == contig_hint.size, update
> > -			 * the scan_hint if it is farther than or larger than
> > -			 * the current scan_hint.
> > -			 */
> > -			block->scan_hint = free;
> > +		} else {
> > +			if (block->contig_hint.size > block->scan_hint.size) {
> > +				if (free.start < block->contig_hint.start) {
> > +					/*
> > +					 * old scan_hint.size < new contig size
> > +					 * == old contig_hint.size. But, the new
> > +					 * contig is before the old contig_hint
> > +					 * so hold the invariant
> > +					 * scan_hint.start > contig_hint.start
> > +					 * iff scan_hint.size ==
> > +					 * contig_hint.size.
> > +					 */
> > +					block->scan_hint.size = 0;
> Does this code unnecessarily invalidate the existing scan_hint by setting its
> size to 0?
> Since block->contig_hint.size > block->scan_hint.size in this branch, it
> appears the existing scan_hint already satisfies the invariant.
> While updating scan_hint to free would violate the invariant because
> free.start < contig_hint.start, simply retaining the old scan_hint rather
> than actively zeroing it seems to preserve the invariant.
> Could we leave the existing scan_hint intact here instead of clearing it to
> prevent a regression from losing a valid allocator hint?

If the old scan_hint is before the new free region, it makes sense to leave
the old scan_hint as-is and I don't see much demerit of it. If the old
scan_hint is after the new free region, however, leaving the old scan_hint
as-is is more likely to break the contig_hint later and I guess it is still
better to scan from 0 in that case. So, let me more diverge in this case.

Thanks,
Joonwon Kang

> > +				} else {
> > +					block->scan_hint = free;
> > +				}
> > +			} else if (free.start > block->scan_hint.start) {
> > +				block->scan_hint = free;
> > +			}
> >  		}