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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
> ---
>  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
> 





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux