Re: [PATCH v4 10/13] x86/mm: Simplify clear_page_*

[Dave Hansen]

On 6/15/25 22:22, Ankur Arora wrote:
> clear_page_rep() and clear_page_erms() are wrappers around "REP; STOS"
> variations. Inlining gets rid of the costly call/ret (for cases with
> speculative execution related mitigations.)

Could you elaborate a bit on which "speculative execution related
mitigations" are so costly with these direct calls?


> -	kmsan_unpoison_memory(page, PAGE_SIZE);
> -	alternative_call_2(clear_page_orig,
> -			   clear_page_rep, X86_FEATURE_REP_GOOD,
> -			   clear_page_erms, X86_FEATURE_ERMS,
> -			   "=D" (page),
> -			   "D" (page),
> -			   "cc", "memory", "rax", "rcx");

I've got to say, I don't dislike the old code. It's utterly clear from
that code what's going on. It's arguable that it's not clear that the
rep/erms variants are just using stosb vs. stosq, but the high level
concept of "use a feature flag to switch between three implementations
of clear page" is crystal clear.

> +	kmsan_unpoison_memory(page, len);
> +	asm volatile(ALTERNATIVE_2("call memzero_page_aligned_unrolled",
> +				   "shrq $3, %%rcx; rep stosq", X86_FEATURE_REP_GOOD,
> +				   "rep stosb", X86_FEATURE_ERMS)
> +			: "+c" (len), "+D" (page), ASM_CALL_CONSTRAINT
> +			: "a" (0)
> +			: "cc", "memory");
>  }

This is substantially less clear. It also doesn't even add comments to
make up for the decreased clarity.

>  void copy_page(void *to, void *from);
> diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> index a508e4a8c66a..27debe0c018c 100644
> --- a/arch/x86/lib/clear_page_64.S
> +++ b/arch/x86/lib/clear_page_64.S
> @@ -6,30 +6,15 @@
>  #include <asm/asm.h>
>  
>  /*
> - * Most CPUs support enhanced REP MOVSB/STOSB instructions. It is
> - * recommended to use this when possible and we do use them by default.
> - * If enhanced REP MOVSB/STOSB is not available, try to use fast string.
> - * Otherwise, use original.
> + * Zero page aligned region.
> + * %rdi	- dest
> + * %rcx	- length
>   */

That comment was pretty useful, IMNHO.

How about we add something like this above it? I think it explains the
whole landscape, including the fact that X86_FEATURE_REP_GOOD is
synthetic and X86_FEATURE_ERMS is not:

Switch between three implementation of page clearing based on CPU
capabilities:

 1. memzero_page_aligned_unrolled(): the oldest, slowest and universally
    supported method. Uses a for loop (in assembly) to write a 64-byte
    cacheline on each loop. Each loop iteration writes to memory using
    8x 8-byte MOV instructions.
 2. "rep stosq": Really old CPUs had crummy REP implementations.
    Vendor CPU setup code sets 'REP_GOOD' on CPUs where REP can be
    trusted. The instruction writes 8 bytes per REP iteration but CPUs
    internally batch these together and do larger writes.
 3. "rep stosb": CPUs that enumerate 'ERMS' have an improved STOS
    implementation that is less picky about alignment and where STOSB
    (1 byte at a time) is actually faster than STOSQ (8 bytes at a
    time).