Re: [PATCH 5/7] maccess: Batch TCO handling in kernel nofault loops

[bot+bpf-ci]

> The number of dynamic MSR TCO executions therefore changes as follows:
>
>     Work                         Before    After
>     N nofault accesses          2N        2
>     4 KiB nofault copy          1,024     2
>     N-byte strncpy              2N        2
>
> The 4 KiB case assumes 512 64-bit accesses. These figures come from the
> control flow rather than a runtime measurement, so the time saved depends
> on the CPU and workload.

This isn't a bug, but would it be worth either dropping the table in favour
of one sentence (one TCO pair per operation instead of one per access) or
backing it with a measurement on a real HW-tag-KASAN async config? Also,
could the "N nofault accesses" row say whether N accesses are within a
single batched operation?

> diff --git a/mm/maccess.c b/mm/maccess.c
> index f695ceefe6fc..87486bf98d15 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -19,7 +19,7 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
>   */
>  #define copy_from_kernel_nofault_loop(dst, src, len, type, err_label)	\
>  	while (len >= sizeof(type)) {					\
> -		__get_kernel_nofault(dst, src, type, err_label);	\
> +		__get_kernel_nofault_bare(dst, src, type, err_label);	\
>  		kmsan_check_memory(src, sizeof(type));			\
>  		dst += sizeof(type);					\
>  		src += sizeof(type);					\
> @@ -39,13 +39,15 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
>  		return 0;
>
>  	scoped_guard(pagefault) {
> -		if (!(align & 7))
> -			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> -		if (!(align & 3))
> -			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> -		if (!(align & 1))
> -			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> -		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> +		scoped_guard(__kernel_nofault_bare) {
> +			if (!(align & 7))
> +				copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> +			if (!(align & 3))
> +				copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> +			if (!(align & 1))
> +				copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> +			copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> +		}
>  	}
>  	return 0;
>  Efault:

A subsystem pattern flags this as potentially concerning: the nested
scoped_guard(__kernel_nofault_bare) introduces a second cleanup scope
inside an existing goto-based error path. include/linux/cleanup.h states
"the expectation is that usage of 'goto' and cleanup helpers is never
mixed in the same function. I.e. for a given routine, convert all
resources that need a 'goto' cleanup to scope-based cleanup, or convert
none of them." Every goto Efault now escapes two nested __cleanup()
scopes (the inner __kernel_nofault_bare and outer pagefault), relying on
the compiler to run both destructors in the correct order. The same
pattern also appears in copy_to_kernel_nofault and
strncpy_from_kernel_nofault below.

Tracing through the implementation suggests the unwind is correct in
practice: GCC and Clang run __attribute__((cleanup)) destructors when
leaving a scope via goto, in reverse order of declaration, so goto Efault
runs __end_kernel_nofault_bare() followed by pagefault_enable() in the
expected order. include/linux/uaccess.h's scoped-access documentation also
explicitly blesses leaving these scopes "with 'return', 'break' and 'goto'
at any point".

Is this an acceptable deepening of the goto-plus-guard mixing to two
levels, or should this be restructured to avoid the nested cleanup scopes?

This isn't a bug, but could this use guard(__kernel_nofault_bare)() as the
first statement of the existing pagefault scope instead of a nested
scoped_guard(), to keep the reindentation out of the diff and match the
form used in the BPF patch later in the series?

> @@ -55,7 +57,7 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
>
>  #define copy_to_kernel_nofault_loop(dst, src, len, type, err_label)	\
>  	while (len >= sizeof(type)) {					\
> -		__put_kernel_nofault(dst, src, type, err_label);	\
> +		__put_kernel_nofault_bare(dst, src, type, err_label);	\
>  		instrument_write(dst, sizeof(type));			\
>  		dst += sizeof(type);					\
>  		src += sizeof(type);					\

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32751368606