Re: [PATCH V4 5/9] iommufd: folio subroutines

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Oct 18, 2024 at 02:27:32PM -0700, Steve Sistare wrote:
> -/* true if the pfn was added, false otherwise */
> -static bool batch_add_pfn(struct pfn_batch *batch, unsigned long pfn)
> +/* returns the number of pfn's added */
> +static long batch_add_pfn_num(struct pfn_batch *batch, unsigned long pfn,
> +			      unsigned long nr)
>  {
> -	const unsigned int MAX_NPFNS = type_max(typeof(*batch->npfns));
> +	const unsigned long MAX_NPFNS = type_max(typeof(*batch->npfns));
> +	unsigned long n = 0;
>  
> -	if (batch->end &&
> -	    pfn == batch->pfns[batch->end - 1] + batch->npfns[batch->end - 1] &&
> -	    batch->npfns[batch->end - 1] != MAX_NPFNS) {
> -		batch->npfns[batch->end - 1]++;
> -		batch->total_pfns++;
> -		return true;
> +	if (batch->end) {
> +		unsigned long npfn_end = batch->npfns[batch->end - 1];
> +		unsigned long pfn_end = batch->pfns[batch->end - 1];
> +
> +		if (pfn == pfn_end + npfn_end && npfn_end < MAX_NPFNS) {
> +			n = min_t(unsigned long, MAX_NPFNS - npfn_end, nr);
> +			batch->npfns[batch->end - 1] += n;
> +			batch->total_pfns += n;
> +			if (nr == n)
> +				return n;
> +			nr -= n;

Looking at this a bit more carefully, MAX_NPFNS is 2**32 and a PFN is
2**12, so a batch entry can describe 2**44 bytes of contiguous memory.

I don't think we need all this logic to try to do 'min'. If the thing
we are trying to add doesn't fit then just skip trying to join it and
continue on.

The next empty item will hold 2**44 and that can hold any single folio
- folio npages is limited to an unsigned int.

One reason I bring this up is that we really don't want to split
across folios, like if you get a 2M,2M,1G pattern then we don't want
to split the 1G across batches as that will definately harm
construction of the page table. Theoretical because of how big the
size is but still.

> +static void batch_from_folios(struct pfn_batch *batch, struct folio ***folios_p,
> +			      unsigned long *offset_p, unsigned long npages)
> +{
> +	unsigned long offset = *offset_p;
> +	struct folio **folios = *folios_p;
> +
> +	while (npages) {
> +		unsigned long n;
> +		struct folio *folio = *folios;
> +		unsigned long nr = folio_nr_pages(folio) - offset;
> +		unsigned long pfn = page_to_pfn(folio_page(folio, offset));
> +
> +		nr = min(nr, npages);

Ah, npages is used for the trailing partial folio too..

> +		n = batch_add_pfn_num(batch, pfn, nr);
> +		npages -= n;
> +
> +		if (n == nr) {
> +			folios++;
> +			offset = 0;
> +		} else if (n) {
> +			offset += n;
> +		} else {
> +			break;
> +		}

Then this gets to be quite a bit simpler

And you'll put the refcount adjustment in here like you said?

Otherwise this patch looks like the right thing

Thanks,
Jason