Re: [PATCH] x86/bitops: Account for POPCNT errata on earlier Intel CPUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2025 19:52, Andrew Cooper wrote:
> Manually break the false dependency for the benefit of cases such as
> bitmap_weight() which is a reasonable hotpath.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Not sure if this warrants a fixes or not, but:
> 
> Fixes: 6978602334d9 ("x86/bitops: Use the POPCNT instruction when available")
> 
> Many examples online suggest a 2x improvement perf improvement on tight loops
> by breaking this dependency.  cpumasks in particular make frequent use of this
> loop.
> 
> Still TODO:
> 
>  1) Put a double CS prefix on the CALL instruction to avoid a trailing 2-byte
>     NOP, but this depends on x86_decode_lite() in order to work.
> 
>  2) Revert a buggy GAS diagnostic:
> 
>     ./arch/x86/include/asm/bitops.h: Assembler messages:
>     ./arch/x86/include/asm/bitops.h:493: Error: same type of prefix used twice
>     ./arch/x86/include/asm/bitops.h:493: Error: same type of prefix used twice
> 
>     Multiple prefixes are not an error, and are sometimes the best choice
>     available.

Well. Sane insns have every kind of prefix at most once. The assembler
has (effectively) boolean markers for that. Thing is that multiple
different prefixes of the same kind are ambiguous to encode: Which order
should they be emitted in? One programmer may think "first wins" while
another may expect "last wins". It may be possible to special case "same
prefix", by converting the boolean-ness to a count. Yet I expect more
often than not multiple prefixes of the same type are a mistake. Hence
we probably would then still want to warn about that, requiring a new
way to silence that warning.

However, imo it is better to leave the assembler unaltered, and continue
to require people who want such unusual encodings to specify the prefixes
they want as separate kind-of-insns. Such unusual encodings are likely to
also mean certain positioning of those prefixes relative to other possible
ones; there's no prescribed order the assembler would emit prefixes of
different kinds (REX and REX2 always coming last of course) when they're
part of a real insn.

> It turns out that LZCNT/TZCNT have the same input dependent bug, prior to
> Skylake.

These two do, but BSF/BSR don't? Pretty odd.

> --- a/xen/arch/x86/include/asm/bitops.h
> +++ b/xen/arch/x86/include/asm/bitops.h
> @@ -488,10 +488,16 @@ static always_inline unsigned int 
> arch_hweightl(unsigned long x)
>       *
>       * This limits the POPCNT instruction to using the same ABI as a function
>       * call (input in %rdi, output in %eax) but that's fine.
> +     *
> +     * On Intel CPUs prior to Cannon Lake, the POPCNT instruction has a false
> +     * input dependency on it's destination register (errata HSD146, SKL029
> +     * amongst others), impacting loops such as bitmap_weight().  Insert an
> +     * XOR to manually break the dependency.
>       */

With this being an Intel-only issue, wouldn't we better ...

>      alternative_io("call arch_generic_hweightl",
> +                   "xor %k[res], %k[res]\n\t"

... put this line in #ifdef CONFIG_INTEL then? The extra overhead is small, but
I see no reason not to avoid it if we can. (I realize that's not quite as
straightforward as it reads, for alternative_io() being a macro.)

Jan

>                     "popcnt %[val], %q[res]", X86_FEATURE_POPCNT,
> -                   ASM_OUTPUT2([res] "=a" (r) ASM_CALL_CONSTRAINT),
> +                   ASM_OUTPUT2([res] "=&a" (r) ASM_CALL_CONSTRAINT),
>                     [val] "D" (x));
>  
>      return r;