| 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: <CAJFsz5w=a1ZSBZaiADxyM32ucaizQiPFTm7=vC2D4W8OkSUp_w@mail.gmail.com>
Date: Mon, 17 Nov 2025 15:05:42 +0800
From: peng li <peng8420.li@...il.com>
To: John Hubbard <jhubbard@...dia.com>
Cc: linux-mm@...ck.org, akpm@...ux-foundation.org, david@...hat.com,
osalvador@...e.de, jgg@...pe.ca, peterx@...hat.com,
linux-kernel@...r.kernel.org, dan.j.williams@...el.com
Subject: Re: [PATCH v2 1/2] Remove the "FOLL_TOUCH" test code from gup_test.c.
John Hubbard <jhubbard@...dia.com> 于2025年11月15日周六 04:25写道：
>
> Hi,
>
> The diffs look good, but the commit log and your replies have many minor
> issues that add up. So you'll want to avoid them in the future, and with
> that in mind, I've listed all of them, along with suggested alternatives.
>
> On 11/14/25 8:11 AM, peng8420.li@...il.com wrote:
> > From: "peng8420.li" <peng8420.li@...il.com>
>
> This should be your real name, such as (my first guess)
> "Peng Li" <peng8420.li@...il.com>.
>
> Also, if you are sending from the same email address as your "From:",
> then the "From:" item shouldn't normally show up (it's harmless here,
> but could be cleaner without it). Check your .gitconfig send-email
> options and you can make it go away.
>
> Also, when replying, your email client is apparently using HTML, please
> fix it to use text only!
>
> Please read the patch submitter guidelines, too:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submit-checklist.rst
>
> > Ever since commit 0f20bba1688b ("mm/gup: explicitly define and check
> > internal GUP flags, disallow FOLL_TOUCH") we marked FOLL_TOUCH as a GUP-internal flag.
>
> Not "ever since", but simply:
>
> commit 0f20bba1688b ("mm/gup: explicitly define and check internal GUP
> flags, disallow FOLL_TOUCH") marked FOLL_TOUCH as a GUP-internal flag.
>
> Also, it's best to keep the line length within about 72 columns for
> commit messages.
>
> >
> > Therefore, remove the "FOLL_TOUCH" test code from gup_test.c;
>
> Trailing semicolon. Should be a period.
>
> >
> > Otherwise, executing the test command "./gup_test -L -r 100 -z" will report the following warning log:
> > TAP version 13
> > 1..1
> > ENCHMARK: Time: [ 39.363371] WARNING: CPU: 1 PID: 117 at mm/gup.c:2512 is_valid_gup_args+0x66/0x8c
> > get:2818 put:46 [ 39.364043] Modules linked in:
> > us# , truncated [ 39.364311] CPU: 1 UID: 0 PID: 117 Comm: gup_test Not tainted 6.18.0-rc5-00324-gd09eaf415c87 #29 NONE
> > (size: 0)#
> > [ 39.364434] Hardware name: riscv-virtio,qemu (DT)
> > [ 39.364546] epc : is_valid_gup_args+0x66/0x8c
> > [ 39.364596] ra : pin_user_pages+0x38/0x78
> > [ 39.364630] epc : ffffffff802079e6 ra : ffffffff8020c214 sp : ff2000000041bd20
> > [ 39.364650] gp : ffffffff81a26068 tp : ff60000080b3b000 t0 : ff2000000041bdf8
> > [ 39.364678] t1 : 000000000000001e t2 : 0000000000000000 s0 : ff2000000041bd30
> > [ 39.364697] s1 : ff60000081300000 a0 : ff60000081300000 a1 : 0000000000000000
> > [ 39.364714] a2 : ff2000000041bd3c a3 : 0000000000080000 a4 : 0000000000000001
> > [ 39.364731] a5 : 0000000000010101 a6 : 0000000000000001 a7 : 0000000000000000
> > [ 39.364747] s2 : 00007fff7eeed000 s3 : 0000000000000001 s4 : 00007fff7eeee000
> > [ 39.364761] s5 : 00007fff7eeeb838 s6 : 0000000000000000 s7 : 00007fff7eeed000
> > [ 39.364825] s8 : ff60000081300000 s9 : 0000000000000000 s10: 0000000000000002
> > [ 39.364842] s11: 000000092869bfdc t3 : 2152ffffffffffc0 t4 : 00000000001fffff
> > [ 39.364855] t5 : ffffffffffffffff t6 : 0000000000000000
> > [ 39.364867] status: 0000000200000120 badaddr: ffffffff802079e6 cause: 0000000000000003
> > [ 39.364949] [<ffffffff802079e6>] is_valid_gup_args+0x66/0x8c
> > [ 39.365036] [<ffffffff8020c214>] pin_user_pages+0x38/0x78
> > [ 39.365049] [<ffffffff80278f78>] gup_test_ioctl+0x2b4/0xc08
> > [ 39.365060] [<ffffffff80297da6>] __riscv_sys_ioctl+0xba/0xc4
> > [ 39.365072] [<ffffffff80b93f96>] do_trap_ecall_u+0x296/0x370
> > [ 39.365093] [<ffffffff80b9ef06>] handle_exception+0x146/0x152
> > [ 39.365220] ---[ end trace 0000000000000000 ]---
> >
>
> We don't need all of this output for such a simple case. Just the
> warning will suffice, and even that is optional for this patch.
>
>
> > Signed-off-by: peng8420.li <peng8420.li@...il.com>
>
> Same rules for the Signed-off-by: real name needed here.
>
> Combining all of that, your commit message would look like this:
>
>
> From: "Peng Li" <peng8420.li@...il.com>
>
> commit 0f20bba1688b ("mm/gup: explicitly define and check internal GUP
> flags, disallow FOLL_TOUCH") marked FOLL_TOUCH as a GUP-internal flag.
>
> This causes a warning to fire when running gup_test, for example:
>
> $ ./gup_test -L -r 100 -z
>
> dmesg:
> WARNING: CPU: 1 PID: 117 at mm/gup.c:2512 is_valid_gup_args+0x66/0x8c
>
> Therefore, remove the "FOLL_TOUCH" test code from gup_test.c.
>
> Signed-off-by: Peng Li <peng8420.li@...il.com>
>
> With the commit log fixed up, please feel free to add:
>
> Reviewed-by: John Hubbard <jhubbard@...dia.com>
>
>
> thanks,
> --
> John Hubbard
>
> > ---
> > tools/testing/selftests/mm/gup_test.c | 22 ++++------------------
> > 1 file changed, 4 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/gup_test.c b/tools/testing/selftests/mm/gup_test.c
> > index 8900b840c17a..75f7134d529d 100644
> > --- a/tools/testing/selftests/mm/gup_test.c
> > +++ b/tools/testing/selftests/mm/gup_test.c
> > @@ -19,7 +19,6 @@
> >
> > /* Just the flags we need, copied from mm.h: */
> > #define FOLL_WRITE 0x01 /* check pte is writable */
> > -#define FOLL_TOUCH 0x02 /* mark page accessed */
> >
> > #define GUP_TEST_FILE "/sys/kernel/debug/gup_test"
> >
> > @@ -93,7 +92,7 @@ int main(int argc, char **argv)
> > {
> > struct gup_test gup = { 0 };
> > int filed, i, opt, nr_pages = 1, thp = -1, write = 1, nthreads = 1, ret;
> > - int flags = MAP_PRIVATE, touch = 0;
> > + int flags = MAP_PRIVATE;
> > char *file = "/dev/zero";
> > pthread_t *tid;
> > char *p;
> > @@ -170,10 +169,6 @@ int main(int argc, char **argv)
> > case 'H':
> > flags |= (MAP_HUGETLB | MAP_ANONYMOUS);
> > break;
> > - case 'z':
> > - /* fault pages in gup, do not fault in userland */
> > - touch = 1;
> > - break;
> > default:
> > ksft_exit_fail_msg("Wrong argument\n");
> > }
> > @@ -244,18 +239,9 @@ int main(int argc, char **argv)
> > else if (thp == 0)
> > madvise(p, size, MADV_NOHUGEPAGE);
> >
> > - /*
> > - * FOLL_TOUCH, in gup_test, is used as an either/or case: either
> > - * fault pages in from the kernel via FOLL_TOUCH, or fault them
> > - * in here, from user space. This allows comparison of performance
> > - * between those two cases.
> > - */
> > - if (touch) {
> > - gup.gup_flags |= FOLL_TOUCH;
> > - } else {
> > - for (; (unsigned long)p < gup.addr + size; p += psize())
> > - p[0] = 0;
> > - }
> > + /* Fault them in here, from user space. */
> > + for (; (unsigned long)p < gup.addr + size; p += psize())
> > + p[0] = 0;
> >
> > tid = malloc(sizeof(pthread_t) * nthreads);
> > assert(tid);
>
>
Hi John Hubbard,
Your review suggestions were very helpful.
Please understand that I'm new here. Thank you so much!
Thanks & Cheers!
Powered by blists - more mailing lists