[PATCH v6 4/7] udmabuf: udmabuf_create pin folio codestyle cleanup

[Huan Yang]

在 2024/9/13 6:43, Kasireddy, Vivek 写道:
> Hi Huan,
>
>> Subject: [PATCH v6 4/7] udmabuf: udmabuf_create pin folio codestyle
>> cleanup
>>
>> This patch aims to simplify the pinning of folio during the udmabuf
>> creation. No functional changes.
>>
>> This patch moves the memfd pin folio to udmabuf_pin_folios and modifies
>> the original loop condition, using the pinned folio as the external
>> loop condition, and sets the offset and folio during the traversal process.
>>
>> By this, more readable.
>>
>> Suggested-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
>> Signed-off-by: Huan Yang <link at vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 134 +++++++++++++++++++++-----------------
>>   1 file changed, 76 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index aa182a9dcdfa..fe1466f7d55a 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -328,17 +328,68 @@ static int export_udmabuf(struct udmabuf *ubuf,
>>   	return dma_buf_fd(buf, flags);
>>   }
>>
>> +static long udmabuf_pin_folios(struct udmabuf *ubuf, struct file *memfd,
>> +			       loff_t start, loff_t size)
>> +{
>> +	pgoff_t pgoff, pgcnt, upgcnt = ubuf->pagecount;
>> +	struct folio **folios = NULL;
>> +	u32 cur_folio, cur_pgcnt;
>> +	long nr_folios;
>> +	long ret = 0;
>> +	loff_t end;
>> +
>> +	pgcnt = size >> PAGE_SHIFT;
>> +	folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
>> +	if (!folios)
>> +		return -ENOMEM;
>> +
>> +	end = start + (pgcnt << PAGE_SHIFT) - 1;
>> +	nr_folios = memfd_pin_folios(memfd, start, end, folios, pgcnt,
>> &pgoff);
>> +	if (nr_folios <= 0) {
>> +		ret = nr_folios ? nr_folios : -EINVAL;
>> +		goto end;
>> +	}
>> +
>> +	cur_pgcnt = 0;
>> +	for (cur_folio = 0; cur_folio < nr_folios; ++cur_folio) {
>> +		pgoff_t subpgoff = pgoff;
>> +		size_t fsize = folio_size(folios[cur_folio]);
>> +
>> +		ret = add_to_unpin_list(&ubuf->unpin_list, folios[cur_folio]);
>> +		if (ret < 0)
>> +			goto end;
>> +
>> +		for (; subpgoff < fsize; subpgoff += PAGE_SIZE) {
>> +			ubuf->folios[upgcnt] = folios[cur_folio];
>> +			ubuf->offsets[upgcnt] = subpgoff;
>> +			++upgcnt;
>> +
>> +			if (++cur_pgcnt >= pgcnt)
>> +				goto end;
>> +		}
>> +
>> +		/**
>> +		 * In a given range, only the first subpage of the first folio
>> +		 * has an offset, that is returned by memfd_pin_folios().
>> +		 * The first subpages of other folios (in the range) have an
>> +		 * offset of 0.
>> +		 */
>> +		pgoff = 0;
>> +	}
>> +end:
>> +	ubuf->pagecount = upgcnt;
>> +	kvfree(folios);
>> +	return ret;
>> +}
>> +
>>   static long udmabuf_create(struct miscdevice *device,
>>   			   struct udmabuf_create_list *head,
>>   			   struct udmabuf_create_item *list)
>>   {
>> -	pgoff_t pgoff, pgcnt, pglimit, pgbuf = 0;
>> -	long nr_folios, ret = -EINVAL;
>> -	struct file *memfd = NULL;
>> -	struct folio **folios;
>> +	pgoff_t pgcnt = 0, pglimit;
>>   	struct udmabuf *ubuf;
>> -	u32 i, j, k, flags;
>> -	loff_t end;
>> +	long ret = -EINVAL;
>> +	u32 i, flags;
>>
>>   	ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>>   	if (!ubuf)
>> @@ -347,81 +398,50 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   	INIT_LIST_HEAD(&ubuf->unpin_list);
>>   	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
>>   	for (i = 0; i < head->count; i++) {
>> -		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
>> +		if (!PAGE_ALIGNED(list[i].offset))
>>   			goto err;
>> -		if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
>> +		if (!PAGE_ALIGNED(list[i].size))
>>   			goto err;
>> -		ubuf->pagecount += list[i].size >> PAGE_SHIFT;
>> -		if (ubuf->pagecount > pglimit)
>> +
>> +		pgcnt += list[i].size >> PAGE_SHIFT;
>> +		if (pgcnt > pglimit)
>>   			goto err;
>>   	}
>>
>> -	if (!ubuf->pagecount)
>> +	if (!pgcnt)
>>   		goto err;
>>
>> -	ubuf->folios = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf-
>>> folios),
>> -				      GFP_KERNEL);
>> +	ubuf->folios = kvmalloc_array(pgcnt, sizeof(*ubuf->folios),
>> GFP_KERNEL);
>>   	if (!ubuf->folios) {
>>   		ret = -ENOMEM;
>>   		goto err;
>>   	}
>> -	ubuf->offsets = kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
>> -				 GFP_KERNEL);
>> +
>> +	ubuf->offsets = kvcalloc(pgcnt, sizeof(*ubuf->offsets), GFP_KERNEL);
>>   	if (!ubuf->offsets) {
>>   		ret = -ENOMEM;
>>   		goto err;
>>   	}
>>
>> -	pgbuf = 0;
>>   	for (i = 0; i < head->count; i++) {
>> -		memfd = fget(list[i].memfd);
>> -		ret = check_memfd_seals(memfd);
>> -		if (ret < 0)
>> -			goto err;
>> +		struct file *memfd = fget(list[i].memfd);
>>
>> -		pgcnt = list[i].size >> PAGE_SHIFT;
>> -		folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
>> -		if (!folios) {
>> -			ret = -ENOMEM;
>> +		if (!memfd) {
>> +			ret = -EBADFD;
> Nit: with the above change added, please remove the redundant if (!memfd)
> check from check_memfd_seals().

Yes, right, I'll do it.

Thank you.

>
> Acked-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
>
>>   			goto err;
>>   		}
>>
>> -		end = list[i].offset + (pgcnt << PAGE_SHIFT) - 1;
>> -		ret = memfd_pin_folios(memfd, list[i].offset, end,
>> -				       folios, pgcnt, &pgoff);
>> -		if (ret <= 0) {
>> -			kvfree(folios);
>> -			if (!ret)
>> -				ret = -EINVAL;
>> +		ret = check_memfd_seals(memfd);
>> +		if (ret < 0) {
>> +			fput(memfd);
>>   			goto err;
>>   		}
>>
>> -		nr_folios = ret;
>> -		pgoff >>= PAGE_SHIFT;
>> -		for (j = 0, k = 0; j < pgcnt; j++) {
>> -			ubuf->folios[pgbuf] = folios[k];
>> -			ubuf->offsets[pgbuf] = pgoff << PAGE_SHIFT;
>> -
>> -			if (j == 0 || ubuf->folios[pgbuf-1] != folios[k]) {
>> -				ret = add_to_unpin_list(&ubuf->unpin_list,
>> -							folios[k]);
>> -				if (ret < 0) {
>> -					kfree(folios);
>> -					goto err;
>> -				}
>> -			}
>> -
>> -			pgbuf++;
>> -			if (++pgoff == folio_nr_pages(folios[k])) {
>> -				pgoff = 0;
>> -				if (++k == nr_folios)
>> -					break;
>> -			}
>> -		}
>> -
>> -		kvfree(folios);
>> +		ret = udmabuf_pin_folios(ubuf, memfd, list[i].offset,
>> +					 list[i].size);
>>   		fput(memfd);
>> -		memfd = NULL;
>> +		if (ret)
>> +			goto err;
>>   	}
>>
>>   	flags = head->flags & UDMABUF_FLAGS_CLOEXEC ? O_CLOEXEC : 0;
>> @@ -432,8 +452,6 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   	return ret;
>>
>>   err:
>> -	if (memfd)
>> -		fput(memfd);
>>   	unpin_all_folios(&ubuf->unpin_list);
>>   	kvfree(ubuf->offsets);
>>   	kvfree(ubuf->folios);
>> --
>> 2.45.2