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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 17, 2024 at 03:37:49PM -0400, Steven Sistare wrote:
> On 10/17/2024 3:24 PM, Jason Gunthorpe wrote:
> > On Thu, Oct 17, 2024 at 01:02:14PM -0400, Steven Sistare wrote:
> > 
> > > > > @@ -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?
> > > 
> > > It is only called when we know that the number of pages in all folios is 1
> > > (user->ufolios_huge is false).
> > 
> > Oh, lets add some comments about this please, and maybe give the
> > function a more scary name. batch_from_folios doesn't suggest it does
> > something so strange. batch_from_single_page_folios ?
> > 
> > But, still, *why*? If the folios in the list all have 1 page size then
> > why not just add them in the normal way?
> > 
> > I still didn't quite entirely follow what the ufolios_huge was doing,
> > it is what I wanted to look at in an editor..
> 
> pin_memfd_pages()
>    for (i = 0; i < nfolios; i++) {
>         nr = folio_nr_pages(folio);
>         if (nr > 1)
>             user->ufolios_huge = true;
> 
> batch_from_folios is an optimized version of batch_from_folios_huge,
> usable when we know that nr==1 for all folios.

I see..

This seems to be a side effect of processing the folio list twice,
once in pin_memfd_pages() which figures out how big it is in terms of
pages, then again when filling the batch

If the list was iterated only once during the batch fill then there
wouldn't be any value in having two paths.

This seems to be like this largely because the upages_start/end are
counting in pages instead of ulist items.

Do you think that is necessary, or could things be done so it index
based and there is only one loop?


Ie instead of things like:

		batch_from_folios_huge(&pfns->batch, &user->ufolios_next,
				       &user->ufolios_offset, npages);

Where npages is the total number of pages inside the folio list, it
would be nfolios.

Jason