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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>