lkml.org 
[lkml]   [2023]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 3/3] raid6: Add LoongArch SIMD recovery implementation
From
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/

\
 
 \ /
  Last update: 2023-08-04 08:25    [W:0.113 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site