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

[Jason Gunthorpe <jgg@nvidia.com>]

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..

> > > +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)
> 
> OK.  Some like this, some do not, now I know your preference.

Yeah, I'm in the camp it is easier to follow

> I personally like to use local scope and initializers, but dislike when
> cstyle it forces me to add a blank line after the declarations which disrupts
> the flow/use of some quantity, eg:
>   {
>     long pfn = foo();
> 
>     bar(pfn);
>   }

Yeah, sometimes, maybe don't initialize in those cases?

> > I would like to see this patch when it is applied but I can't apply
> > it..
> 
> I will send you my patched version of pages.c off list.

include a git link in your next cover letter, that makes it easier

Jason