Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian

[Rui Salvaterra]

2016-04-06 14:09 GMT+01:00 Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>:
> Cc Chanho Min, Kyungsik Lee
>
>
> Hello,
>
> On (04/06/16 10:39), Rui Salvaterra wrote:
>> > may we please ask you to test the patch first? quite possible there
>> > is nothing to fix there; I've no access to mips h/w but the patch
>> > seems correct to me.
>> >
>> > LZ4_READ_LITTLEENDIAN_16 does get_unaligned_le16(), so
>> > LZ4_WRITE_LITTLEENDIAN_16 must do put_unaligned_le16() /* not put_unaligned() */
>> >
> [..]
>> Consequentially, while I believe the patch will fix the mips case, I'm
>> not so sure about ppc (or any other big endian architecture with
>> efficient unaligned accesses).
>
> frankly, yes, I took a quick look today (after I sent my initial
> message, tho) ... and it is fishy, I agree. was going to followup
> on my email but somehow got interrupted, sorry.
>
> so we have, write:
>         ((U16_S *)(p)) = v    OR    put_unaligned(v, (u16 *)(p))
>
> and only one read:
>         get_unaligned_le16(p))
>
> I guess it's either read part also must depend on
> HAVE_EFFICIENT_UNALIGNED_ACCESS, or write path
> should stop doing so.
>
> I ended up with two patches, NONE was tested (!!!). like at all.
>
> 1) provide CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS-dependent
>    LZ4_READ_LITTLEENDIAN_16
>
> 2) provide common LZ4_WRITE_LITTLEENDIAN_16 and LZ4_READ_LITTLEENDIAN_16
>    regardless CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
>
>
> assuming that common LZ4_WRITE_LITTLEENDIAN_16 will somehow hit the
> performance, I'd probably prefer option #1.
>
> the patch is below. would be great if you can help testing it.
>
> ---
>
>  lib/lz4/lz4defs.h | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
> index abcecdc..a23e6c2 100644
> --- a/lib/lz4/lz4defs.h
> +++ b/lib/lz4/lz4defs.h
> @@ -36,10 +36,14 @@ typedef struct _U64_S { u64 v; } U64_S;
>  #define PUT4(s, d) (A32(d) = A32(s))
>  #define PUT8(s, d) (A64(d) = A64(s))
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
> -       do {    \
> -               A16(p) = v; \
> -               p += 2; \
> +       do {                                    \
> +               A16(p) = v;                     \
> +               p += 2;                         \
>         } while (0)
> +
> +#define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> +       (d = s - A16(p))
> +
>  #else /* CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS */
>
>  #define A64(x) get_unaligned((u64 *)&(((U16_S *)(x))->v))
> @@ -52,10 +56,13 @@ typedef struct _U64_S { u64 v; } U64_S;
>         put_unaligned(get_unaligned((const u64 *) s), (u64 *) d)
>
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
> -       do {    \
> -               put_unaligned(v, (u16 *)(p)); \
> -               p += 2; \
> +       do {                                            \
> +               put_unaligned_le16(v, (u16 *)(p));      \
> +               p += 2;                                 \
>         } while (0)
> +
> +#define LZ4_READ_LITTLEENDIAN_16(d, s, p)              \
> +       (d = s - get_unaligned_le16(p))
>  #endif
>
>  #define COPYLENGTH 8
> @@ -140,9 +147,6 @@ typedef struct _U64_S { u64 v; } U64_S;
>
>  #endif
>
> -#define LZ4_READ_LITTLEENDIAN_16(d, s, p) \
> -       (d = s - get_unaligned_le16(p))
> -
>  #define LZ4_WILDCOPY(s, d, e)          \
>         do {                            \
>                 LZ4_COPYPACKET(s, d);   \
>


Hi again, Sergey


Thanks for the patch, I'll test it as soon as possible. I agree with
your second option, usually one selects lz4 when (especially
decompression) speed is paramount, so it needs all the help it can
get.

Speaking of fishy, the 64-bit detection code also looks suspiciously
bogus. Some of the identifiers don't even exist anywhere in the kernel
(__ppc64__, por example, after grepping all .c and .h files).
Shouldn't we instead check for CONFIG_64BIT or BITS_PER_LONG == 64?


Thanks,

Rui