Re: [PATCH 1/2] Remove the "FOLL_TOUCH" test code from gup_test.c.

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 14.11.25 09:50, peng8420.li@gmail.com wrote:
> From: "peng8420.li" <peng8420.li@gmail.com>
> 
>  From submission 0f20bba1688bdf3b32df0162511a67d4eda15790, we know that "FOLL_TOUCH" is only for internal scope use;

That is not the proper way to cite commit IDs. Likely checkpatch.pl would complain here.

"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."

> 
> Therefore, remove the "FOLL_TOUCH" test code from gup_test.c;
> 
> 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 ]---
> ok 1 ioctl status 0

Right, gup_tests simply forward any flags we pass in. I wonder if we would
want to filter them for ones we expect to test?


I guess we should add

Fixes: 0f20bba1688b ("mm/gup: explicitly define and check internal GUP flags, disallow FOLL_TOUCH")

Although it's a bit complicated with gup_tests.

> 
> Signed-off-by: peng8420.li <peng8420.li@gmail.com>
> ---
>   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;

AFAIKT, run_vmtests.sh does not set that flag, so dropping it should be fine.

-- 
Cheers

David