On 26.11.2024 18:13, Andrew Cooper wrote:
> On 25/11/2024 2:28 pm, Jan Beulich wrote:
>> ---
>> We may want to consider branching over the REP STOSQ as well, if the
>> number of qwords turns out to be zero.
>
> Until FSR{S,M} (Fast Short Rep {STO,MOV}SB), which is far newer than
> ERMS, passing 0 into any REP instruction is expensive.
Is this a request to add such a conditional branch then, perhaps patched
out when FSRS is available? And then perhaps also for the JZ that's
already there?
> I wonder how often we memset with a size less than 8.
Hence why I raised the point, rather than putting the jump there directly.
>> We may also want to consider using non-REP STOS{L,W,B} for the tail.
>
> Probably, yes. We use this form in non-ERMS cases, where we're advised
> to stay away from STOSB entirely.
Yet then we'll end up with three conditional branches - do we really want
that?
> Interestingly, Linux doesn't have a STOSQ case at all. Or rather, it
> was deleted by Linus in 20f3337d350c last year. It was also identified
> as causing a performance regression.
> https://lore.kernel.org/lkml/CANn89iKUbyrJ=r2+_kk+sb2zsshiffz7qkpldpatkj8v4wu...@mail.gmail.com/T/#u
> although the memset() path was not reverted as part of the fix
> (47ee3f1dd93bcb eventually).
>
> Yet ca96b162bfd2 shows that REP MOVSQ is still definitely a win on Rome
> CPUs.
>
> I expect we probably do want some non-rep forms in here.
>
> Do you have any benchmarks with this series?
What I specifically measured were the clear_page() variants. I didn't do
any measurements for memset() (or memcpy()), first ad foremost because
any selection of inputs is going to be arbitrary rather than representative.
>> --- /dev/null
>> +++ b/xen/arch/x86/memset.S
>> @@ -0,0 +1,30 @@
>> +#include <asm/asm_defns.h>
>> +
>> +.macro memset
>> + and $7, %edx
>> + shr $3, %rcx
>> + movzbl %sil, %esi
>> + mov $0x0101010101010101, %rax
>> + imul %rsi, %rax
>> + mov %rdi, %rsi
>> + rep stosq
>> + or %edx, %ecx
>> + jz 0f
>> + rep stosb
>> +0:
>> + mov %rsi, %rax
>
> Could you use %r8/9/etc instead of %rsi please? This is deceptively
> close to looking like a bug, and it took me a while to figure out it's
> only correct because STOSB only edits %rdi.
Well, I can certainly switch (the number of REX prefixes will remain the
same as it looks), but the fact that STOS, unlike MOVS, doesn't touch
%rsi is a pretty basic one.
Does your request extend to all uses of %rsi (and %esi), or merely the
latter two (across the REP STOS)?
Jan
