Re: [PATCH v2 01/18] mm: Make folios_put() the basis of release_pages()

[David Hildenbrand <david@redhat.com>]

On 19.02.24 16:03, Matthew Wilcox wrote:
> On Mon, Feb 19, 2024 at 10:43:06AM +0100, David Hildenbrand wrote:
>> On 17.02.24 03:25, Matthew Wilcox (Oracle) wrote:
>>> By making release_pages() call folios_put(), we can get rid of the calls
>>> to compound_head() for the callers that already know they have folios.
>>> We can also get rid of the lock_batch tracking as we know the size
>>> of the batch is limited by folio_batch.  This does reduce the maximum
>>> number of pages for which the lruvec lock is held, from SWAP_CLUSTER_MAX
>>> (32) to PAGEVEC_SIZE (15).  I do not expect this to make a significant
>>> difference, but if it does, we can increase PAGEVEC_SIZE to 31.
>>>
>>
>> I'm afraid that won't apply to current mm-unstable anymore, where we can now
>> put multiple references to a single folio (as part of unmapping
>> large PTE-mapped folios).
> 
> Argh.  I'm not a huge fan of that approach, but let's live with it for
> now.  

I'm hoping we at least can get rid of page ranges at some point (and 
just have folio + nr_refs), but for the time being there is no way 
around that due to delayed rmap handling that needs the exact pages (ugh).

folios_put_refs() does sound reasonable in any case, although likely 
"putting multiple references" is limited to zap/munmap/... code paths.

> How about this as a replacement patch?  It compiles ...
> 

Nothing jumped at me, one comment:

[...]

> +EXPORT_SYMBOL(folios_put);
> +
> +/**
> + * release_pages - batched put_page()
> + * @arg: array of pages to release
> + * @nr: number of pages
> + *
> + * Decrement the reference count on all the pages in @arg.  If it
> + * fell to zero, remove the page from the LRU and free it.
> + *
> + * Note that the argument can be an array of pages, encoded pages,
> + * or folio pointers. We ignore any encoded bits, and turn any of
> + * them into just a folio that gets free'd.
> + */
> +void release_pages(release_pages_arg arg, int nr)
> +{
> +	struct folio_batch fbatch;
> +	int refs[PAGEVEC_SIZE];
> +	struct encoded_page **encoded = arg.encoded_pages;
> +	int i;
> +
> +	folio_batch_init(&fbatch);
> +	for (i = 0; i < nr; i++) {
> +		/* Turn any of the argument types into a folio */
> +		struct folio *folio = page_folio(encoded_page_ptr(encoded[i]));
> +
> +		/* Is our next entry actually "nr_pages" -> "nr_refs" ? */
> +		refs[fbatch.nr] = 1;
> +		if (unlikely(encoded_page_flags(encoded[i]) &
> +			     ENCODED_PAGE_BIT_NR_PAGES_NEXT))
> +			refs[fbatch.nr] = encoded_nr_pages(encoded[++i]);
> +
> +		if (folio_batch_add(&fbatch, folio) > 0)
> +			continue;
> +		folios_put_refs(&fbatch, refs);
> +	}
> +
> +	if (fbatch.nr)
> +		folios_put_refs(&fbatch, refs);

I wonder if it makes sense to remember if any ref !=1, and simply call 
folios_put() if that's the case.

But I guess the whole point about PAGEVEC_SIZE is that it is very 
cache-friendly and traversing it a second time (e.g., when all we are 
doing is freeing order-0 folios) is not too expensive.

-- 
Cheers,

David / dhildenb