diff mbox series

[v3,4/5] udmabuf: codestyle cleanup

Message ID 20240813090518.3252469-5-link@vivo.com
State New
Headers show
Series udmbuf bug fix and some improvements | expand

Commit Message

Huan Yang Aug. 13, 2024, 9:05 a.m. UTC
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(-)

Comments

Kasireddy, Vivek Aug. 17, 2024, 12:58 a.m. UTC | #1
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().

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

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

Thanks,
Vivek

>  		kvfree(folios);
>  		fput(memfd);
>  		memfd = NULL;
> --
> 2.45.2
>
Huan Yang Aug. 20, 2024, 1:37 a.m. UTC | #2
在 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
>>
diff mbox series

Patch

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;
-				}
+		for (j = 0, cnt = 0; j < nr_folios; ++j, pgoff = 0) {
+			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) {
+				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;
 		kvfree(folios);
 		fput(memfd);
 		memfd = NULL;