Re: [PATCH V1 4/9] iommufd: pfn reader for file mappings

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

On 9/15/2024 4:51 PM, Jason Gunthorpe wrote:
> On Sat, Sep 14, 2024 at 06:05:53AM -0700, Steve Sistare wrote:
>> Extend pfn_reader_user to pin file mappings, by calling memfd_pin_folios.
>> Repin at small page granularity and unpack pages into upages[] to mesh
>> with the existing code paths.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   drivers/iommu/iommufd/io_pagetable.h |  5 ++++
>>   drivers/iommu/iommufd/pages.c        | 58 ++++++++++++++++++++++++++++++++----
>>   2 files changed, 57 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
>> index 7c4a338..3a28f46 100644
>> --- a/drivers/iommu/iommufd/io_pagetable.h
>> +++ b/drivers/iommu/iommufd/io_pagetable.h
>> @@ -177,6 +177,7 @@ enum {
>>   
>>   enum iopt_address_type {
>>   	IOPT_ADDRESS_USER = 0,
>> +	IOPT_ADDRESS_FILE = 1,
>>   };
>>   
>>   /*
>> @@ -202,6 +203,10 @@ struct iopt_pages {
>>   	enum iopt_address_type type;
>>   	union {
>>   		void __user *uptr;		/* IOPT_ADDRESS_USER */
>> +		struct {			/* IOPT_ADDRESS_FILE */
>> +			struct file *file;
>> +			unsigned long start;
>> +		};
>>   	};
>>   	bool writable:1;
>>   	u8 account_mode;
>> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
>> index 69822d4..065c28f 100644
>> --- a/drivers/iommu/iommufd/pages.c
>> +++ b/drivers/iommu/iommufd/pages.c
>> @@ -695,6 +695,7 @@ static unsigned long batch_rw(struct pfn_batch *batch, void *data,
>>   struct pfn_reader_user {
>>   	struct page **upages;
>>   	size_t upages_len;
>> +	struct file *file;
>>   	unsigned long upages_start;
>>   	unsigned long upages_end;
>>   	unsigned int gup_flags;
>> @@ -712,6 +713,7 @@ static void pfn_reader_user_init(struct pfn_reader_user *user,
>>   	user->upages_start = 0;
>>   	user->upages_end = 0;
>>   	user->locked = -1;
>> +	user->file = (pages->type == IOPT_ADDRESS_FILE) ? pages->file : NULL;
>>   
>>   	user->gup_flags = FOLL_LONGTERM;
>>   	if (pages->writable)
>> @@ -733,13 +735,54 @@ static void pfn_reader_user_destroy(struct pfn_reader_user *user,
>>   	user->upages = NULL;
>>   }
>>   
>> +static long pin_memfd_pages(struct pfn_reader_user *user,
>> +			    unsigned long start,
>> +			    unsigned long npages)
>> +{
>> +	unsigned long end, nr, i, j, k, npin, nfolios, pgoff, max_folios;
>> +	unsigned long npages_orig = npages;
>> +	struct folio *folio;
>> +	size_t size = npages * sizeof(folio);
>> +	struct folio **folios = temp_kmalloc(&size, NULL, 0);
>> +
>> +	if (!folios)
>> +		return -ENOMEM;
>> +
>> +	k = 0;
>> +	max_folios = size / sizeof(folio);
>> +	end = start + (npages << PAGE_SHIFT) - 1;
>> +
>> +	while (npages > 0) {
>> +		nfolios = memfd_pin_folios(user->file, start, end,
>> +					   folios, max_folios, &pgoff);
>> +		if (nfolios <= 0)
>> +			return nfolios;
>> +
>> +		pgoff >>= PAGE_SHIFT;
>> +		for (i = 0; i < nfolios; i++) {
>> +			folio = folios[i];
>> +			nr = folio_nr_pages(folio);
>> +			npin = min(nr - pgoff, npages);
>> +			repin_folio_unhugely(folio, npin);
>> +			for (j = pgoff; j < pgoff + npin; j++)
>> +				user->upages[k++] = folio_page(folio, j);
>> +			npages -= npin;
>> +			start += npin << PAGE_SHIFT;
>> +			pgoff = 0;
> 
> You should try harder to avoid this loop, the batch already can just
> swallow a full folio, so this would be much better to copy more of the
> code from it's caller and be more fully stand alone. Use only a temp
> folio array and stick that directly into the batch full folio at a
> time.

Yes, I optimize in a later patch as you already saw in the other thread.
I submitted this slow version as an intermediate step to show a working
implementation with less code to review.  I forgot to mention that in the
commit message.

- Steve

> This above will be functionally the same, but alot slower..
> 
> Jason