Hi Huan,I'll try this in next version
There are some variables in udmabuf_create that are only used inside theI can see that having a loop that iterates from 0 to nr_folios is more intuitive
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;
- }
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().
Yes.
+ u32 k;I think renaming k to something like subpgoff or tailpgoff would make this
+ 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) {
more easier to understand.
Yes, but need take care of when into error, ubuf->pagecount also need update or else, deinit in error path will also "errorr"
+ ubuf->folios[pgbuf] = folios[j];Since pgbuf also reflects the total number of pages (or folios) processed,
+ 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;
I think you can use that instead of having to mess with pagecount.
Thanks,
Vivek
kvfree(folios);
fput(memfd);
memfd = NULL;
--
2.45.2