| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <80b41d38-2912-4a34-9cd4-ac2c71b210c8@vivo.com>
Date: Tue, 20 Aug 2024 09:37:34 +0800
From: Huan Yang <link@...o.com>
To: "Kasireddy, Vivek" <vivek.kasireddy@...el.com>,
Gerd Hoffmann <kraxel@...hat.com>, Sumit Semwal <sumit.semwal@...aro.org>,
Christian König <christian.koenig@....com>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc: "opensource.kernel@...o.com" <opensource.kernel@...o.com>
Subject: Re: [PATCH v3 4/5] udmabuf: codestyle cleanup
在 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@...o.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
>>
Powered by blists - more mailing lists