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
>
