lib/raid/xor: x86: Add AVX-512 optimized xor_gen()

Note: for now I omitted the cpu_has_xfeatures() check that the AVX-512 optimized crypto and CRC code does, since it's not implemented on User-Mode Linux and it's never been present in the RAID6 code either.

But I wanted to ask: do we really care about the case where features are "supported" but their XCR0 bits aren't set? Perhaps the kernel just doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?

If this case indeed needs to be handled, could we make things easier for the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:

if (boot_cpu_has(X86_FEATURE_AVX512F) &&
cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))

How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures() check wouldn't be needed. Is there any reason not to do that?

Quoted 5 lines · Eric Biggers

But I wanted to ask: do we really care about the case where features are "supported" but their XCR0 bits aren't set? Perhaps the kernel just doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?

Yes, our aim is to support only configurations which are actually present in real hardware and not a "oh, it would be good if it did that, just because..."

Quoted 7 lines

If this case indeed needs to be handled, could we make things easier for the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:

if (boot_cpu_has(X86_FEATURE_AVX512F) &&
cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))

Quoted 3 lines

How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures() check wouldn't be needed. Is there any reason not to do that?

How do you want to accomplish that? Very early during boot on the BSP you sanity-check XCR0 and clear feature flags if components are not set?

Quoted 7 lines · Borislav Petkov

Quoted 5 lines · Eric Biggers

But I wanted to ask: do we really care about the case where features are "supported" but their XCR0 bits aren't set? Perhaps the kernel just doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?

Quoted 3 lines

Yes, our aim is to support only configurations which are actually present in real hardware and not a "oh, it would be good if it did that, just because..."

Seems reasonable to me. Would the same apply to UML here?

Quoted 11 lines

Quoted 10 lines

If this case indeed needs to be handled, could we make things easier for the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:

if (boot_cpu_has(X86_FEATURE_AVX512F) &&
cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))

How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures() check wouldn't be needed. Is there any reason not to do that?

Quoted 3 lines

How do you want to accomplish that? Very early during boot on the BSP you sanity-check XCR0 and clear feature flags if components are not set?

That would be the idea. Something similar to what arch/x86/kernel/cpu/cpuid-deps.c does.

Except that seems to only enforce the dependencies when the kernel itself is disabling things; if the hypervisor is broken then it just warns.

In any case, I'd like these to go away:

    $ git grep cpu_has_xfeatures | wc -l
    31

Quoted 5 lines

In any case, I'd like these to go away:

    $ git grep cpu_has_xfeatures | wc -l
    31

Yeah, all in crypto. I can certainly see why.

@dhansen, any other thoughts?

Quoted 5 lines · Borislav Petkov

Quoted 5 lines

In any case, I'd like these to go away:

    $ git grep cpu_has_xfeatures | wc -l
    31

Quoted 3 lines

Yeah, all in crypto. I can certainly see why.

@dhansen, any other thoughts?

If we can get rid of cpu_has_xfeatures(), I'm all for it. I'm not quite sure how the code would look so I'm reserving judgement until I see the patches. But it's worth a try.

Quoted 3 lines · Eric Biggers

Note: for now I omitted the cpu_has_xfeatures() check that the AVX-512 optimized crypto and CRC code does, since it's not implemented on User-Mode Linux and it's never been present in the RAID6 code either.

By the way, Sashiko keeps complaining about this decision.

Maybe the x86 maintainers have some advice here?

For context: on x86 processors, executing AVX or AVX512 instructions requires not just that the CPU supports the feature, but also that the operating system has set certain bits in XCR0. For example all EVEX coded instructions (i.e. AVX-512) require XCR0=111xx111b. (See Intel manual "2.6.11.1 State Dependent #UD".)

Therefore most of the kernel's AVX and AVX512 optimized code checks not just X86_FEATURE_AVX* but also calls cpu_has_xfeatures() to check XCR0.

But "most" isn't all. The RAID6 code for example doesn't check cpu_has_xfeatures(). So if you e.g. boot a kernel in QEMU using "-cpu max,xsave=off", it already crashes when the RAID6 code does its boot-time benchmark.

Part of the reason for that omission probably is that UML doesn't provide an implementation of cpu_has_xfeatures(). And the x86 RAID (XOR and RAID6) code is enabled on UML.

It could be implemented for UML by using the xgetbv instruction, like what userspace programs do. (We'd also need to copy the XFEATURE_MASK_* constants, as UML can't include arch/x86/include/asm/fpu/types.h)

But I wanted to ask: do we really care about the case where features are "supported" but their XCR0 bits aren't set? Perhaps the kernel just doesn't/shouldn't support weird cases like "-cpu max,xsave=off"?

If this case indeed needs to be handled, could we make things easier for the kernel's AVX and AVX-512 optimized code? Currently AVX-512 needs:

if (boot_cpu_has(X86_FEATURE_AVX512F) &&
cpu_has_xfeatures(XFEATURE_MASK_FP | XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512, NULL))

How about we make X86_FEATURE_AVX512F depend on XCR0=111xx111, and X86_FEATURE_AVX depend on XCR0=xxxxx111? Then the cpu_has_xfeatures() check wouldn't be needed. Is there any reason not to do that?

- Eric

Benchmark on AMD Ryzen 9 9950X (Zen 5):

    src_cnt    avx          avx512       Improvement
    =======    ==========   ==========   ===========
    1          56353 MB/s   75388 MB/s   33%
    2          54274 MB/s   68409 MB/s   26%
    3          44649 MB/s   64042 MB/s   43%
    4          41315 MB/s   55002 MB/s   33%

Can use the xor: prefix used for all other commits to lib/raid/xor?

Quoted 8 lines

Benchmark on AMD Ryzen 9 9950X (Zen 5):

    src_cnt    avx          avx512       Improvement
    =======    ==========   ==========   ===========
    1          56353 MB/s   75388 MB/s   33%
    2          54274 MB/s   68409 MB/s   26%
    3          44649 MB/s   64042 MB/s   43%
    4          41315 MB/s   55002 MB/s   33%  

On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not sure if is just the different benchmarking or something else going on.

FYI, one or 2 sources are basically useless as they RAID5 configs that have no benefits over simple mirroring and thus the numbers aren't too interesting.

lib/raid/xor/x86/xor-avx512.c line 104

Quoted 3 lines

lib/raid/xor/x86/xor-avx512.c new file +2 -0

+DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
+	      xor_avx512_5);  

Is there really much of a benefit of doing the historic DO_XOR_BLOCKS vs doing the loop manually? Especially as the common cases for a modern RAID will usually loop over more disks than this was built for. I.e., in practice one or two source buffers only happen at the end of a loop over more disks.

Quoted 3 lines

FYI, one or 2 sources are basically useless as they RAID5 configs that have no benefits over simple mirroring and thus the numbers aren't too interesting.

With three disks you xor two buffers (src_count == 1) to get the parity to write to the third - so that is a valid RAID5 config.

Quoted 6 lines · David Laight

Quoted 3 lines

FYI, one or 2 sources are basically useless as they RAID5 configs that have no benefits over simple mirroring and thus the numbers aren't too interesting.

With three disks you xor two buffers (src_count == 1) to get the parity to write to the third - so that is a valid RAID5 config.

Sure. It did not say it is invalid, it just isn't very useful.

Can use the xor: prefix used for all other commits to lib/raid/xor?

Quoted 8 lines

Benchmark on AMD Ryzen 9 9950X (Zen 5):

    src_cnt    avx          avx512       Improvement
    =======    ==========   ==========   ===========
    1          56353 MB/s   75388 MB/s   33%
    2          54274 MB/s   68409 MB/s   26%
    3          44649 MB/s   64042 MB/s   43%
    4          41315 MB/s   55002 MB/s   33%

On my Zen 5 mobile (AMD Ryzen AI 7 PRO 350) both the existing AVX2 and this AVX512 code give numbers in the 200+ GB/s range. Not sure if is just the different benchmarking or something else going on.

FYI, one or 2 sources are basically useless as they RAID5 configs that have no benefits over simple mirroring and thus the numbers aren't too interesting.

lib/raid/xor/x86/xor-avx512.c line 104

Quoted 2 lines

lib/raid/xor/x86/xor-avx512.c new file +2 -0

+DO_XOR_BLOCKS(avx512_inner, xor_avx512_2, xor_avx512_3, xor_avx512_4,
+	      xor_avx512_5);

Is there really much of a benefit of doing the historic DO_XOR_BLOCKS vs doing the loop manually? Especially as the common cases for a modern RAID will usually loop over more disks than this was built for. I.e., in practice one or two source buffers only happen at the end of a loop over more disks.