Hi Matt,

> The raid6 Q syndrome check has been optimised using the vpermxor
> instruction.

Very much a nit, but normally we'd write the change that the patch makes
as a command: "Optimise the raid6 Q syndrome generation using the
vpermxor instruction" - see
https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html#describe-your-changes

> +static void noinline raid6_vpermxor$#_gen_syndrome_real(int disks, size_t 
> bytes,
> +                                                     void **ptrs)
> +{
> +     u8 **dptr = (u8 **)ptrs;
> +     u8 *p, *q;
> +     int d, z, z0;
> +     unative_t wp$$, wq$$, wd$$;
> +
> +     z0 = disks - 3;         /* Highest data disk */
> +     p = dptr[z0+1];         /* XOR parity */
> +     q = dptr[z0+2];         /* RS syndrome */
> +
> +     for (d = 0; d < bytes; d += NSIZE*$#) {
> +             wp$$ = wq$$ = *(unative_t *)&dptr[z0][d+$$*NSIZE];
> +
> +             for (z = z0-1; z>=0; z--) {
> +                     wd$$ = *(unative_t *)&dptr[z][d+$$*NSIZE];
> +                     /* P syndrome */
> +                     wp$$ = vec_xor(wp$$, wd$$);
> +
> +                     /*Q syndrome */
> +                     asm("vpermxor %0,%1,%2,%3":"=v"(wq$$):"v"(gf_high), 
> "v"(gf_low), "v"(wq$$));

Initially I thought "why can't we break this over 2 lines?" and then I
remembered that the awk script can't handle that. A space between /* and
Q would be good though.

> +                     wq$$ = vec_xor(wq$$, wd$$);

I generated some of the unrolled code and inspected it. It's non-trivial
to follow but that's justifiable, it's due to:
 - the complex maths
 - the unrolling process
 - consistency with the altivec code, which I think is worth keeping
I am not sure how you could make it any easier to read, so I don't think
that should block its acceptance into the kernel.

I am confident that this code works correctly and as described.

Reviewed-by: Daniel Axtens <d...@axtens.net>

Regards,
Daniel

> -- 
> 2.9.3

Reply via email to