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

[Steven Sistare <steven.sistare@oracle.com>]

On 10/16/2024 9:00 AM, Jason Gunthorpe wrote:
> 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..

Thanks, will fix.

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

>> +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.
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);
   }

> and the style in iommufd avoids more
> than one variable per declaration

OK, I had not realized that.

- Steve

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

- Steve