Re: [PATCH V3 8/9] iommufd: optimize file mapping

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Oct 04, 2024 at 11:48:19AM -0700, Steve Sistare wrote:

> @@ -347,25 +347,41 @@ static void batch_destroy(struct pfn_batch *batch, void *backup)
>  		kfree(batch->pfns);
>  }
>  
> -/* 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 int batch_add_pfn_num(struct pfn_batch *batch, unsigned long pfn,
> +			     unsigned long nr)

This return type is truncating unsigned longs to int..

> @@ -623,6 +639,67 @@ static void batch_from_pages(struct pfn_batch *batch, struct page **pages,
>  			break;
>  }
>  
> +static void batch_from_folios(struct pfn_batch *batch, struct folio **folios,
> +			      size_t npages)
> +{
> +	struct folio **end = folios + npages;
> +
> +	for (; folios != end; folios++)
> +		if (!batch_add_pfn(batch, page_to_pfn(&folios[0]->page)))
> +			break;

This function is really weird, why would we have a function adding
only the first page of each item in a list of folios?

> +static void batch_from_folios_huge(struct pfn_batch *batch,
> +				   struct folio ***folios_p,
> +				   unsigned long *offset_p,
> +				   unsigned long npages)
> +{
> +	unsigned long nr, n, pfn, i = 0;
> +	struct folio *folio, **folios = *folios_p;
> +	unsigned long offset = *offset_p;

Let's make more use of scope local values please (put folio, pfn, etc
in side the narrowest loop scope) and the style in iommufd avoids more
than one variable per declaration

I would like to see this patch when it is applied but I can't apply
it..

Jason