Re: [PATCH v3 4/5] udmabuf: codestyle cleanup

[Huan Yang <link@vivo.com>]

在 2024/8/17 8:58, Kasireddy, Vivek 写道:
> Hi Huan,
>
>> There are some variables in udmabuf_create that are only used inside the
>> loop. Therefore, there is no need to declare them outside the scope.
>> This patch moved it into loop.
>>
>> It is difficult to understand the loop condition of the code that adds
>> folio to the unpin_list.
>>
>> The outer loop of this patch iterates through folios, while the inner
>> loop correctly sets the folio and corresponding offset into the udmabuf
>> starting from the offset. if reach to pgcnt or nr_folios, end of loop.
>>
>> By this, more readable.
>>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 65 ++++++++++++++++++++-------------------
>>   1 file changed, 33 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index 4ec54c60d76c..8f9cb0e2e71a 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -296,12 +296,12 @@ 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;
>> +	pgoff_t upgcnt = 0, pglimit, pgbuf = 0;
>> +	long ret = -EINVAL;
>>   	struct file *memfd = NULL;
>>   	struct folio **folios;
>>   	struct udmabuf *ubuf;
>> -	u32 i, j, k, flags;
>> +	u32 i, flags;
>>   	loff_t end;
>>
>>   	ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>> @@ -315,22 +315,21 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   			goto err;
>>   		if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
>>   			goto err;
>> -		ubuf->pagecount += list[i].size >> PAGE_SHIFT;
>> +		upgcnt += list[i].size >> PAGE_SHIFT;
>>   		if (ubuf->pagecount > pglimit)
>>   			goto err;
>>   	}
>>
>> -	if (!ubuf->pagecount)
>> +	if (!upgcnt)
>>   		goto err;
>>
>> -	ubuf->folios = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf-
>>> folios),
>> +	ubuf->folios = kvmalloc_array(upgcnt, 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(upgcnt, sizeof(*ubuf->offsets),
>> GFP_KERNEL);
>>   	if (!ubuf->offsets) {
>>   		ret = -ENOMEM;
>>   		goto err;
>> @@ -338,6 +337,10 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>
>>   	pgbuf = 0;
>>   	for (i = 0; i < head->count; i++) {
>> +		pgoff_t pgoff, pgcnt;
>> +		u32 j, cnt;
>> +		long nr_folios;
>> +
>>   		memfd = fget(list[i].memfd);
>>   		ret = check_memfd_seals(memfd);
>>   		if (ret < 0)
>> @@ -351,38 +354,36 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   		}
>>
>>   		end = list[i].offset + (pgcnt << PAGE_SHIFT) - 1;
>> -		ret = memfd_pin_folios(memfd, list[i].offset, end,
>> -				       folios, pgcnt, &pgoff);
>> -		if (ret <= 0) {
>> +		nr_folios = memfd_pin_folios(memfd, list[i].offset, end,
>> folios,
>> +					     pgcnt, &pgoff);
>> +		if (nr_folios <= 0) {
>>   			kvfree(folios);
>> -			if (!ret)
>> -				ret = -EINVAL;
>> +			ret = nr_folios ? nr_folios : -EINVAL;
>>   			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;
>> -				}
> I can see that having a loop that iterates from 0 to nr_folios is more intuitive
> compared to the previous version which was a legacy carryover.
>
>> +		for (j = 0, cnt = 0; j < nr_folios; ++j, pgoff = 0) {
> Can all the code in this outer loop be moved into a separate function? This
> would reduce the length of udmabuf_create().
I'll try this in next version
>
>> +			u32 k;
>> +			long fsize = folio_size(folios[j]);
>> +
>> +			ret = add_to_unpin_list(&ubuf->unpin_list, folios[j]);
>> +			if (ret < 0) {
>> +				kfree(folios);
>> +				goto err;
>>   			}
>>
>> -			pgbuf++;
>> -			if (++pgoff == folio_nr_pages(folios[k])) {
>> -				pgoff = 0;
>> -				if (++k == nr_folios)
>> -					break;
>> +			for (k = pgoff; k < fsize; k += PAGE_SIZE) {
> I think renaming k to something like subpgoff or tailpgoff would make this
> more easier to understand.
Yes.
>
>> +				ubuf->folios[pgbuf] = folios[j];
>> +				ubuf->offsets[pgbuf] = k;
>> +				++pgbuf;
>> +
>> +				if (++cnt >= pgcnt)
>> +					goto end;
>>   			}
>>   		}
>> -
>> +end:
>> +		// update the number of pages that have already been set
>> up.
>> +		ubuf->pagecount += pgcnt;
> Since pgbuf also reflects the total number of pages (or folios) processed,
> I think you can use that instead of having to mess with pagecount.
Yes, but need take care of when into error, ubuf->pagecount also need 
update or else, deinit in error path will also "errorr"
>
> Thanks,
> Vivek
>
>>   		kvfree(folios);
>>   		fput(memfd);
>>   		memfd = NULL;
>> --
>> 2.45.2
>>