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

[Ankur Arora <ankur.a.arora@oracle.com>]

Dave Hansen <dave.hansen@intel.com> writes:

> 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?

I can specify that we would mispredict on the RET if you use RETHUNK.

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

Yeah this seems good to add. And, sorry should have threshed that comment
out in the new location instead of just getting rid of it.

--
ankur