Re: [PATCH v2 3/3] raid6: Add LoongArch SIMD recovery implementation

[WANG Xuerui <kernel@xen0n.name>]

Hi,

On 2023/8/4 03:49, Paul Menzel wrote:
> Dear Xuerui,
> 
> 
> Thank you for your patches.
> 
> 
> Am 03.08.23 um 19:08 schrieb WANG Xuerui:
>> From: WANG Xuerui <git@xen0n.name>
>>
>> Similar to the syndrome calculation, the recovery algorithms also work
>> on 64 bytes at a time to align with the L1 cache line size of current
>> and future LoongArch cores (that we care about). Which means
>> unrolled-by-4 LSX and unrolled-by-2 LASX code.
>>
>> The assembly is originally based on the x86 SSSE3/AVX2 ports, but
>> register allocation has been redone to take advantage of LSX/LASX's 32
>> vector registers, and instruction sequence has been optimized to suit
>> (e.g. LoongArch can perform per-byte srl and andi on vectors, but x86
>> cannot).
>>
>> Performance numbers measured by instrumenting the raid6test code:
> 
> It’d be great, if you also documented your test setup. That’s always 
> good for benchmarking numbers.
> 

Ah they're the same as described in the previous patches: a Loongson 
3A5000 + 7A1000 board (the Lemote A2101 board to be precise), with the 
3A5000 clocked at 2.5GHz. I'll amend the description in v3.

>>> lasx  2data: 354.987 MiB/s
>>> lasx  datap: 350.430 MiB/s
>>> lsx   2data: 340.026 MiB/s
>>> lsx   datap: 337.318 MiB/s
>>> intx1 2data: 164.280 MiB/s
>>> intx1 datap: 187.966 MiB/s
> 
> So the speed is more than doubled. Nice job! The lasx implementation is 
> always the fastest. Is it therefore the preferred one? Or does it come 
> with higher power consumption?

According to my experiments and other public info regarding the LA464 
micro-architecture such as [1], this should be the case for LA464, with 
no power consumption worries.

(Both LASX and LSX are handled by the same vector unit on LA464, so 
whenever LASX is available it should be preferred, as it's LSX that 
would be purely wasteful in this case: a full 256-bit result would get 
computed regardless.)

[1]: 
https://chipsandcheese.com/2023/04/09/loongsons-3a5000-chinas-best-shot/

> 
>> Signed-off-by: WANG Xuerui <git@xen0n.name>
> 
> Out of curiosity, what is your “first” name?

My first name / given name is "Xuerui"; I usually prefer having my 
romanized name in "native-endian" whenever it's convenient ;-)

> 
>> ---
>>   include/linux/raid/pq.h          |   2 +
>>   lib/raid6/Makefile               |   2 +-
>>   lib/raid6/algos.c                |   8 +
>>   lib/raid6/recov_loongarch_simd.c | 515 +++++++++++++++++++++++++++++++
>>   lib/raid6/test/Makefile          |   2 +-
>>   5 files changed, 527 insertions(+), 2 deletions(-)
>>   create mode 100644 lib/raid6/recov_loongarch_simd.c
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [snip]
>> +
>> +    /* Now, pick the proper data tables */
>> +    pbmul = raid6_vgfmul[raid6_gfexi[failb-faila]];
> 
> Should spaces be put around the operator?

Hmm, AFAICS almost all raid6 files "inherit" the scalar reference 
implementation's coding style, that happens to differ from the 
documented one. But given I already adjusted some of the comments, I 
think it would be best to also touch these too. Thanks for spotting this 
(and the others below).

> 
>> +    qmul  = raid6_vgfmul[raid6_gfinv[raid6_gfexp[faila] ^
>> +        raid6_gfexp[failb]]];
>> +
>> [snip]
>> +
>> +    /* Now, pick the proper data tables */
>> +    qmul  = raid6_vgfmul[raid6_gfinv[raid6_gfexp[faila]]];
> 
> Only one space after qmul?

As explained above; I'll fix this one and others in v3.

> [snip]
>> +    /* Now, pick the proper data tables */
>> +    pbmul = raid6_vgfmul[raid6_gfexi[failb-faila]];
> 
> Ditto.
> 
> [snip]
>> +    /* Now, pick the proper data tables */
>> +    qmul  = raid6_vgfmul[raid6_gfinv[raid6_gfexp[faila]]];
> 
> Ditto.
> 
> [snip]
>> diff --git a/lib/raid6/test/Makefile b/lib/raid6/test/Makefile
>> index 7b244bce32b3d..2abe0076a636c 100644
>> --- a/lib/raid6/test/Makefile
>> +++ b/lib/raid6/test/Makefile
>> @@ -65,7 +65,7 @@ else ifeq ($(HAS_ALTIVEC),yes)
>>           OBJS += altivec1.o altivec2.o altivec4.o altivec8.o \
>>                   vpermxor1.o vpermxor2.o vpermxor4.o vpermxor8.o
>>   else ifeq ($(ARCH),loongarch64)
>> -        OBJS += loongarch_simd.o
>> +        OBJS += loongarch_simd.o recov_loongarch_simd.o
>>   endif
>>   .c.o:
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> PS: I brought up the raid speed tests in the past, and Borislav called 
> them a random number generator [1]. ;-)
> 
> 
> [1]: https://lore.kernel.org/all/20210406124126.GM17806@zn.tnic/

Interesting, so the reason I've yet to observe such wild fluctuations 
may simply be that I didn't reboot that rig as many times. :D

And thanks for the review! I'll send v3 some time later (and stress the 
code more meanwhile).

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/