Re: [PATCH v4 3/3] mm/page_alloc: Batch page freeing in free_frozen_page_commit

[Vlastimil Babka <vbabka@suse.cz>]

On 10/14/25 15:15, Joshua Hahn wrote:
> On Tue, 14 Oct 2025 11:38:00 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
> 
>> On 10/13/25 21:08, Joshua Hahn wrote:
>> > Before returning, free_frozen_page_commit calls free_pcppages_bulk using
>> > nr_pcp_free to determine how many pages can appropritately be freed,
>> > based on the tunable parameters stored in pcp. While this number is an
>> > accurate representation of how many pages should be freed in total, it
>> > is not an appropriate number of pages to free at once using
>> > free_pcppages_bulk, since we have seen the value consistently go above
>> > 2000 in the Meta fleet on larger machines.
>> > 
>> > As such, perform batched page freeing in free_pcppages_bulk by using
>> > pcp->batch member. In order to ensure that other processes are not
>> > starved of the zone lock, free both the zone lock and pcp lock to yield to
>> > other threads.
>> > 
>> > Note that because free_frozen_page_commit now performs a spinlock inside the
>> > function (and can fail), the function may now return with a freed pcp.
>> > To handle this, return true if the pcp is locked on exit and false otherwise.
>> > 
>> > In addition, since free_frozen_page_commit must now be aware of what UP
>> > flags were stored at the time of the spin lock, and because we must be
>> > able to report new UP flags to the callers, add a new unsigned long*
>> > parameter UP_flags to keep track of this.
> 
> [...snip...]
> 
>> > @@ -2861,15 +2871,47 @@ static void free_frozen_page_commit(struct zone *zone,
>> >  		 * Do not attempt to take a zone lock. Let pcp->count get
>> >  		 * over high mark temporarily.
>> >  		 */
>> > -		return;
>> > +		return true;
>> >  	}
>> >  
>> >  	high = nr_pcp_high(pcp, zone, batch, free_high);
>> >  	if (pcp->count < high)
>> > -		return;
>> > +		return true;
>> > +
>> > +	to_free = nr_pcp_free(pcp, batch, high, free_high);
>> > +	if (to_free == 0)
>> > +		return true;
> 
> Hello Vlastimil, thank you for your patience and review on this iteration!

No problem!

> 
>> > +		to_free_batched = min(to_free, batch);
>> > +		free_pcppages_bulk(zone, to_free_batched, pcp, pindex);
>> > +		to_free -= to_free_batched;
>> > +		if (pcp->count >= high) {
> 
> Here, I think I should change this in the next version to also just check
> for the same condition in the while loop (i.e. to_free > 0 && pcp->count > 0)

You're right, I missed that one.
> The idea is that if we have another iteration, we will re-lock. Otherwise, we
> can just ignore the case inside the if statement. I think if it is left as
> a check for pcp->count >= high, then there will be a weird case for when
> 0 < pcp->count <= high, where we continue to call free_pcppages_bulk but
> do not re-lock.
> 
> So unfortunately, I will have to check for the same condition of the
> while loop in the if statement : -( I'll send a new version with the changes;
> I don't expect there to be a drastic performance change, since I think the
> early termination case would have only applied if there was a race condition
> that freed the pcp remotely.

Great!