From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754928AbcDFML3 (ORCPT ); Wed, 6 Apr 2016 08:11:29 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:35411 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753495AbcDFML0 (ORCPT ); Wed, 6 Apr 2016 08:11:26 -0400 Date: Wed, 6 Apr 2016 22:09:11 +0900 From: Sergey Senozhatsky To: Rui Salvaterra Cc: Sergey Senozhatsky , Greg KH , linux-kernel@vger.kernel.org, eunb.song@samsung.com, minchan@kernel.org, linux-mm@kvack.org, Sergey Senozhatsky , Chanho Min , Kyungsik Lee Subject: Re: [BUG] lib: zram lz4 compression/decompression still broken on big endian Message-ID: <20160406130911.GA584@swordfish> References: <20160405153439.GA2647@kroah.com> <20160406053325.GA415@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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); \