| Message ID | 20240813090518.3252469-5-link@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | udmbuf bug fix and some improvements | expand |
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 >
在 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 --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;
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(-)