Groups > linux.kernel > #1573964 > unrolled thread

[PATCH v7 0/5] Update LZ4 compressor module

Started by	Sven Schmidt <4sschmid@informatik.uni-hamburg.de>
First post	2017-02-05 20:20 +0100
Last post	2017-02-13 13:10 +0100
Articles	4 on this page of 24 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  [PATCH v7 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-05 20:20 +0100
    [PATCH v7 5/5] lib/lz4: Remove back-compat wrappers Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-05 20:20 +0100
    [PATCH v7 3/5] crypto: Change LZ4 modules to work with new LZ4 module version Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-05 20:20 +0100
    Re: [PATCH v7 0/5] Update LZ4 compressor module Minchan Kim <minchan@kernel.org> - 2017-02-09 00:50 +0100
      Re: [PATCH v7 0/5] Update LZ4 compressor module Eric Biggers <ebiggers3@gmail.com> - 2017-02-09 01:30 +0100
        Re: [PATCH v7 0/5] Update LZ4 compressor module Eric Biggers <ebiggers3@gmail.com> - 2017-02-09 07:40 +0100
          Re: [PATCH v7 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-09 12:10 +0100
            Re: [PATCH v7 0/5] Update LZ4 compressor module Eric Biggers <ebiggers3@gmail.com> - 2017-02-09 19:30 +0100
          Re: [PATCH v7 0/5] Update LZ4 compressor module Minchan Kim <minchan@kernel.org> - 2017-02-10 01:20 +0100
        Re: [PATCH v7 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-09 12:10 +0100
          Re: [PATCH v7 0/5] Update LZ4 compressor module Eric Biggers <ebiggers3@gmail.com> - 2017-02-09 20:50 +0100
            Re: [PATCH v7 0/5] Update LZ4 compressor module David Miller <davem@davemloft.net> - 2017-02-10 05:30 +0100
      Re: [PATCH v7 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-09 12:00 +0100
        Re: [PATCH v7 0/5] Update LZ4 compressor module Minchan Kim <minchan@kernel.org> - 2017-02-10 02:00 +0100
          Re: [PATCH v7 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-12 12:20 +0100
            [PATCH] lz4: fix performance regressions Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-12 12:20 +0100
              Re: [PATCH] lz4: fix performance regressions Willy Tarreau <w@1wt.eu> - 2017-02-12 14:10 +0100
                Re: [PATCH] lz4: fix performance regressions Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-12 16:30 +0100
                  Re: [PATCH] lz4: fix performance regressions Willy Tarreau <w@1wt.eu> - 2017-02-12 22:50 +0100
                    Re: [PATCH] lz4: fix performance regressions Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-13 13:00 +0100
                      Re: [PATCH] lz4: fix performance regressions Willy Tarreau <w@1wt.eu> - 2017-02-13 14:40 +0100
              Re: [PATCH] lz4: fix performance regressions Eric Biggers <ebiggers3@gmail.com> - 2017-02-13 00:40 +0100
            Re: [PATCH v7 0/5] Update LZ4 compressor module Minchan Kim <minchan@kernel.org> - 2017-02-13 01:10 +0100
              Re: [PATCH v7 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-02-13 13:10 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1579790 — Re: [PATCH] lz4: fix performance regressions

From	Willy Tarreau <w@1wt.eu>
Date	2017-02-13 14:40 +0100
Subject	Re: [PATCH] lz4: fix performance regressions
Message-ID	<tap1g-4iT-13@gated-at.bofh.it>
In reply to	#1579659

On Mon, Feb 13, 2017 at 12:53:49PM +0100, Sven Schmidt wrote:
> On Sun, Feb 12, 2017 at 10:41:17PM +0100, Willy Tarreau wrote:
> > On Sun, Feb 12, 2017 at 04:20:00PM +0100, Sven Schmidt wrote:
> > > On Sun, Feb 12, 2017 at 02:05:08PM +0100, Willy Tarreau wrote:
> > > > Hi Sven,
> > > > 
> > > > On Sun, Feb 12, 2017 at 12:16:18PM +0100, Sven Schmidt wrote:
> > > > > Fix performance regressions compared to current kernel LZ4
> > > > 
> > > > Your patch contains mostly style cleanups which certainly are welcome
> > > > but make the whole patch hard to review. These cleanups would have been
> > > > better into a separate, preliminary patch IMHO.
> > > > 
> > > > Regards,
> > > > Willy
> > > 
> > > Hi Willy,
> > > 
> > > the problem was, I wanted to compare my version to the upstream LZ4 to find bugs (as with my last patch version: wrong indentation in LZ4HC 
> > > in two for loops). But since the LZ4 code is a pain to read, I made additional style cleanups "on the way".
> > 
> > Oh I can easily understand!
> > 
> > > Hope you can manage to review the patch though, because it is difficult to separate the cleanups now.
> > 
> > When I need to split a patch into pieces, usually what I do is that I
> > revert it, re-apply it without committing, then "git add -p", validate
> > all the hunks to be taken as the first patch (ie here the cleanups),
> > commit, then commit the rest as a separate one. It seems to me that the
> > fix is in the last few hunks though I'm not sure yet.
> > 
> > Thanks,
> > Willy
> 
> Hi Willy,
> 
> I didn't know about this 'trick' until now. Thanks for sharing it! I gave it a short try recently, that's really cool!
> 
> Since the problem discussed in this branch of this thread seems to be solved (see Minchans E-Mail), I won't split the patches, though.
> Or is there an actual need for doing so? I will send an updated patchset (containing these patches + the other ones suggested by Eric) later.

It's probably too late for this time, but keep it in mind for next time :-)

willy

[toc] | [prev] | [next] | [standalone]

#1579324 — Re: [PATCH] lz4: fix performance regressions

From	Eric Biggers <ebiggers3@gmail.com>
Date	2017-02-13 00:40 +0100
Subject	Re: [PATCH] lz4: fix performance regressions
Message-ID	<tabUl-491-7@gated-at.bofh.it>
In reply to	#1579200

Hi Sven,

On Sun, Feb 12, 2017 at 12:16:18PM +0100, Sven Schmidt wrote:
>  /*-************************************
>   *	Reading and writing into memory
>   **************************************/
> +typedef union {
> +	U16 u16;
> +	U32 u32;
> +	size_t uArch;
> +} __packed unalign;
> 
> -static inline U16 LZ4_read16(const void *memPtr)
> +static FORCE_INLINE __maybe_unused U16 LZ4_read16(const void *ptr)
>  {
> -	U16 val;
> -
> -	memcpy(&val, memPtr, sizeof(val));
> -
> -	return val;
> +	return ((const unalign *)ptr)->u16;
>  }
> 
> -static inline U32 LZ4_read32(const void *memPtr)
> +static FORCE_INLINE __maybe_unused U32 LZ4_read32(const void *ptr)
>  {
> -	U32 val;
> -
> -	memcpy(&val, memPtr, sizeof(val));
> -
> -	return val;
> +	return ((const unalign *)ptr)->u32;
>  }
> 
> -static inline size_t LZ4_read_ARCH(const void *memPtr)
> +static FORCE_INLINE __maybe_unused size_t LZ4_read_ARCH(const void *ptr)
>  {
> -	size_t val;
> -
> -	memcpy(&val, memPtr, sizeof(val));
> -
> -	return val;
> +	return ((const unalign *)ptr)->uArch;
>  }
> 
> -static inline void LZ4_write16(void *memPtr, U16 value)
> +static FORCE_INLINE __maybe_unused void LZ4_write16(void *memPtr, U16 value)
>  {
> -	memcpy(memPtr, &value, sizeof(value));
> +	((unalign *)memPtr)->u16 = value;
>  }
> 
> -static inline void LZ4_write32(void *memPtr, U32 value)
> -{
> -	memcpy(memPtr, &value, sizeof(value));
> +static FORCE_INLINE __maybe_unused void LZ4_write32(void *memPtr, U32 value) {
> +	((unalign *)memPtr)->u32 = value;
>  }
> 
> -static inline U16 LZ4_readLE16(const void *memPtr)
> +static FORCE_INLINE __maybe_unused U16 LZ4_readLE16(const void *memPtr)
>  {
> -#ifdef __LITTLE_ENDIAN__
> +#if LZ4_LITTLE_ENDIAN
>  	return LZ4_read16(memPtr);
>  #else
>  	const BYTE *p = (const BYTE *)memPtr;
> @@ -137,19 +143,19 @@ static inline U16 LZ4_readLE16(const void *memPtr)
>  #endif
>  }

Since upstream LZ4 is intended to be compiled at -O3, this may allow it to get
away with using memcpy() for unaligned memory accesses.  The reason it uses
memcpy() is that, other than a byte-by-byte copy, it is the only portable way to
express unaligned memory accesses.  But the Linux kernel is sometimes compiled
optimized for size (-Os), and I wouldn't be *too* surprised if some of the
memcpy()'s don't always get inlined then, which could be causing the performance
regression being observed.  (Of course, this could be verified by checking
whether CONFIG_CC_OPTIMIZE_FOR_SIZE=y is set, then reading the assembly.)

But I don't think accessing a __packed structure directly is the right
alternative.  Instead, Linux already includes macros for unaligned memory
accesses which have been optimized for every supported architecture.  Those
should just be used instead, e.g. like this:

static FORCE_INLINE U16 LZ4_read16(const void *ptr)
{
	return get_unaligned((const u16 *)ptr);
}

static FORCE_INLINE U32 LZ4_read32(const void *ptr)
{
	return get_unaligned((const u32 *)ptr);
}

static FORCE_INLINE size_t LZ4_read_ARCH(const void *ptr)
{
	return get_unaligned((const size_t *)ptr);
}

static FORCE_INLINE void LZ4_write16(void *memPtr, U16 value)
{
	put_unaligned(value, (u16 *)memPtr);
}

static FORCE_INLINE void LZ4_write32(void *memPtr, U32 value)
{
	put_unaligned(value, (u32 *)memPtr);
}

static FORCE_INLINE U16 LZ4_readLE16(const void *memPtr)
{
	return get_unaligned_le16(memPtr);
}

static FORCE_INLINE void LZ4_writeLE16(void *memPtr, U16 value)
{
	return put_unaligned_le16(value, memPtr);
}

static FORCE_INLINE void LZ4_copy8(void *dst, const void *src)
{
	if (LZ4_64bits()) {
		u64 a = get_unaligned((const u64 *)src);
		put_unaligned(a, (u64 *)dst);
	} else {
		u32 a = get_unaligned((const u32 *)src);
		u32 b = get_unaligned((const u32 *)src + 1);
		put_unaligned(a, (u32 *)dst);
		put_unaligned(b, (u32 *)dst + 1);
	}
}


Note that I dropped __maybe_unused as it's not needed on inline functions.
That should be done everywhere else the patch proposes to add it too.

> -#if LZ4_ARCH64
> -#ifdef __BIG_ENDIAN__
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
> +static FORCE_INLINE unsigned int LZ4_NbCommonBytes(register size_t val)
> +{
> +#if LZ4_LITTLE_ENDIAN
> +#if LZ4_ARCH64 /* 64 Bits Little Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> +	static const int DeBruijnBytePos[64] = {
> +		0, 0, 0, 0, 0, 1, 1, 2, 0, 3, 1, 3, 1, 4, 2, 7,
> +		0, 2, 3, 6, 1, 5, 3, 5, 1, 3, 4, 4, 2, 5, 6, 7,
> +		7, 0, 1, 2, 3, 3, 4, 6, 2, 6, 5, 5, 3, 4, 5, 6,
> +		7, 1, 2, 4, 6, 4, 4, 5, 7, 2, 6, 5, 7, 6, 7, 7
> +	};
> +
> +	return DeBruijnBytePos[((U64)((val & -(long long)val)
> +		* 0x0218A392CDABBD3FULL)) >> 58];
>  #else
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_ctzll(val) >> 3)
> -#endif
> +	return (__builtin_ctzll((U64)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#else /* 32 Bits Little Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> +	static const int DeBruijnBytePos[32] = {
> +		0, 0, 3, 0, 3, 1, 3, 0, 3, 2, 2, 1, 3, 2, 0, 1,
> +		3, 3, 1, 2, 2, 2, 2, 0, 3, 1, 2, 0, 1, 0, 1, 1
> +	};
> +
> +	return DeBruijnBytePos[((U32)((val & -(S32)val)
> +		* 0x077CB531U)) >> 27];
>  #else
> -#ifdef __BIG_ENDIAN__
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_clz(val) >> 3)
> +	return (__builtin_ctz((U32)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#endif /* LZ4_ARCH64 */
> +#else /* Big Endian */
> +#if LZ4_ARCH64 /* 64 Bits Big Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> +	unsigned int r;
> +
> +	if (!(val >> 32)) {
> +		r = 4;
> +	} else {
> +		r = 0;
> +		val >>= 32;
> +	}
> +
> +	if (!(val >> 16)) {
> +		r += 2;
> +		val >>= 8;
> +	} else {
> +		val >>= 24;
> +	}
> +
> +	r += (!val);
> +
> +	return r;
>  #else
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_ctz(val) >> 3)
> -#endif
> -#endif
> +	return (__builtin_clzll((U64)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#else /* 32 Bits Big Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> +	unsigned int r;
> +
> +	if (!(val >> 16)) {
> +		r = 2;
> +		val >>= 8;
> +	} else {
> +		r = 0;
> +		val >>= 24;
> +	}
> +
> +	r += (!val);
> +
> +	return r;
> +#else
> +	return (__builtin_clz((U32)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#endif /* LZ4_ARCH64 */
> +#endif /* LZ4_LITTLE_ENDIAN */
> +}

The reason LZ4_NbCommonBytes() in upstream LZ4 is so complicated is that it
needs to provide portable fallbacks that work on *any* platform and compiler.
This isn't needed in the Linux kernel, and it should just call the functions
already defined that do the right thing:

static FORCE_INLINE unsigned int LZ4_NbCommonBytes(register size_t val)
{
        if (LZ4_isLittleEndian())
                return __ffs(val) >> 3;
        else
                return (BITS_PER_LONG - 1 - __fls(val)) >> 3;
}                      

To be clear, when I said that upstream LZ4 shouldn't generally be changed, I'm
primarily talking about the core code, not the platform-specific parts.  What we
need to do is define platform-specific stuff, like LZ4_read*(), LZ4_write*(),
LZ4_NbCommonBytes(), LZ4_64bits(), and FORCE_INLINE, in a way that makes sense
for the Linux kernel and the environment it's compiled in.  Also I think it's
fine, and maybe even necessary for performance, to *add* inline or FORCE_INLINE
in some places too, given that LZ4 in Linux may get compiled with a lower
optimization level than that intended to be used for upstream LZ4 --- though it
may be worth considering updating the Makefile to just always compile the LZ4
files with -O3 instead.  What should be avoided is making unnecessary changes to
the *users* of the platform-specific code or to the core (de)compression
parameters or templates.

Eric

[toc] | [prev] | [next] | [standalone]

#1579329

From	Minchan Kim <minchan@kernel.org>
Date	2017-02-13 01:10 +0100
Message-ID	<tacnn-4ym-15@gated-at.bofh.it>
In reply to	#1579199

Hi Sven,

On Sun, Feb 12, 2017 at 12:16:17PM +0100, Sven Schmidt wrote:
> 
> 
> 
> On 02/10/2017 01:13 AM, Minchan Kim wrote:
> > Hello Sven,
> >
> > On Thu, Feb 09, 2017 at 11:56:17AM +0100, Sven Schmidt wrote:
> >> Hey Minchan,
> >>
> >> On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
> >>> Hello Sven,
> >>>
> >>> On Sun, Feb 05, 2017 at 08:09:03PM +0100, Sven Schmidt wrote:
> >>>>
> >>>> This patchset is for updating the LZ4 compression module to a version based
> >>>> on LZ4 v1.7.3 allowing to use the fast compression algorithm aka LZ4 fast
> >>>> which provides an "acceleration" parameter as a tradeoff between
> >>>> high compression ratio and high compression speed.
> >>>>
> >>>> We want to use LZ4 fast in order to support compression in lustre
> >>>> and (mostly, based on that) investigate data reduction techniques in behalf of
> >>>> storage systems.
> >>>>
> >>>> Also, it will be useful for other users of LZ4 compression, as with LZ4 fast
> >>>> it is possible to enable applications to use fast and/or high compression
> >>>> depending on the usecase.
> >>>> For instance, ZRAM is offering a LZ4 backend and could benefit from an updated
> >>>> LZ4 in the kernel.
> >>>>
> >>>> LZ4 homepage: http://www.lz4.org/
> >>>> LZ4 source repository: https://github.com/lz4/lz4
> >>>> Source version: 1.7.3
> >>>>
> >>>> Benchmark (taken from [1], Core i5-4300U @1.9GHz):
> >>>> ----------------|--------------|----------------|----------
> >>>> Compressor      | Compression  | Decompression  | Ratio
> >>>> ----------------|--------------|----------------|----------
> >>>> memcpy          |  4200 MB/s   |  4200 MB/s     | 1.000
> >>>> LZ4 fast 50     |  1080 MB/s   |  2650 MB/s     | 1.375
> >>>> LZ4 fast 17     |   680 MB/s   |  2220 MB/s     | 1.607
> >>>> LZ4 fast 5      |   475 MB/s   |  1920 MB/s     | 1.886
> >>>> LZ4 default     |   385 MB/s   |  1850 MB/s     | 2.101
> >>>>
> >>>> [1] http://fastcompression.blogspot.de/2015/04/sampling-or-faster-lz4.html
> >>>>
> >>>> [PATCH 1/5] lib: Update LZ4 compressor module
> >>>> [PATCH 2/5] lib/decompress_unlz4: Change module to work with new LZ4 module version
> >>>> [PATCH 3/5] crypto: Change LZ4 modules to work with new LZ4 module version
> >>>> [PATCH 4/5] fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version
> >>>> [PATCH 5/5] lib/lz4: Remove back-compat wrappers
> >>>
> >>> Today, I did zram-lz4 performance test with fio in current mmotm and
> >>> found it makes regression about 20%.
> >>>
> >>> "lz4-update" means current mmots(git://git.cmpxchg.org/linux-mmots.git) so
> >>> applied your 5 patches. (But now sure current mmots has recent uptodate
> >>> patches)
> >>> "revert" means I reverted your 5 patches in current mmots.
> >>>
> >>>                      revert    lz4-update
> >>>
> >>>       seq-write       1547       1339      86.55%
> >>>      rand-write      22775      19381      85.10%
> >>>        seq-read       7035       5589      79.45%
> >>>       rand-read      78556      68479      87.17%
> >>>    mixed-seq(R)       1305       1066      81.69%
> >>>    mixed-seq(W)       1205        984      81.66%
> >>>   mixed-rand(R)      17421      14993      86.06%
> >>>   mixed-rand(W)      17391      14968      86.07%
> >>
> >> which parts of the output (as well as units) are these values exactly?
> >> I did not work with fio until now, so I think I might ask before misinterpreting my results.
> >
> > It is IOPS.
> >
> >>  
> >>> My fio description file
> >>>
> >>> [global]
> >>> bs=4k
> >>> ioengine=sync
> >>> size=100m
> >>> numjobs=1
> >>> group_reporting
> >>> buffer_compress_percentage=30
> >>> scramble_buffers=0
> >>> filename=/dev/zram0
> >>> loops=10
> >>> fsync_on_close=1
> >>>
> >>> [seq-write]
> >>> bs=64k
> >>> rw=write
> >>> stonewall
> >>>
> >>> [rand-write]
> >>> rw=randwrite
> >>> stonewall
> >>>
> >>> [seq-read]
> >>> bs=64k
> >>> rw=read
> >>> stonewall
> >>>
> >>> [rand-read]
> >>> rw=randread
> >>> stonewall
> >>>
> >>> [mixed-seq]
> >>> bs=64k
> >>> rw=rw
> >>> stonewall
> >>>
> >>> [mixed-rand]
> >>> rw=randrw
> >>> stonewall
> >>>
> >>
> >> Great, this makes it easy for me to reproduce your test.
> >
> > If you have trouble to reproduce, feel free to ask me. I'm happy to test it. :)
> >
> > Thanks!
> >
> 
> Hi Minchan,
> 
> I will send an updated patch as a reply to this E-Mail. Would be really grateful If you'd test it and provide feedback!
> The patch should be applied to the current mmots tree.
> 
> In fact, the updated LZ4 _is_ slower than the current one in kernel. But I was not able to reproduce such large regressions
> as you did. I now tried to define FORCE_INLINE as Eric suggested. I also inlined some functions which weren't in upstream LZ4,
> but are defined as macros in the current kernel LZ4. The approach to replace LZ4_ARCH64 with the function call _seemed_ to behave
> worse than the macro, so I withdrew the change.
> 
> The main difference is, that I replaced the read32/read16/write... etc. functions using memcpy with the other ones defined 
> in upstream LZ4 (which can be switched using a macro). 
> The comment of the author stated, that they're as fast as the memcpy variants (or faster), but not as portable
> (which does not matter since we're not dependent for multiple compilers).
> 
> In my tests, this version is mostly as fast as the current kernel LZ4.

With a patch you sent, I cannot see enhancement so I wanted to dig in and
found how I was really careless.

I have tested both test with CONFIG_KASAN. OMG. With disabling it, I don't
see any regression any more. So, I'm really really *sorry* about noise and
wasting your time. However, I am curious why KASAN makes such difference.

The reason I tested new updated lz4 is description says lz4 fast and
want to use it in zram. How can I do that? and How faster it is compared
to old?

Thanks for you work!

[toc] | [prev] | [next] | [standalone]

#1579664

From	Sven Schmidt <4sschmid@informatik.uni-hamburg.de>
Date	2017-02-13 13:10 +0100
Message-ID	<tanC9-3wl-5@gated-at.bofh.it>
In reply to	#1579329

On Mon, Feb 13, 2017 at 09:03:24AM +0900, Minchan Kim wrote:
> Hi Sven,
> 
> On Sun, Feb 12, 2017 at 12:16:17PM +0100, Sven Schmidt wrote:
> > 
> > 
> > 
> > On 02/10/2017 01:13 AM, Minchan Kim wrote:
> > > Hello Sven,
> > >
> > > On Thu, Feb 09, 2017 at 11:56:17AM +0100, Sven Schmidt wrote:
> > >> Hey Minchan,
> > >>
> > >> On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
> > >>> Hello Sven,
> > >>>
> > >>> On Sun, Feb 05, 2017 at 08:09:03PM +0100, Sven Schmidt wrote:
> > >>>>
> > >>>> This patchset is for updating the LZ4 compression module to a version based
> > >>>> on LZ4 v1.7.3 allowing to use the fast compression algorithm aka LZ4 fast
> > >>>> which provides an "acceleration" parameter as a tradeoff between
> > >>>> high compression ratio and high compression speed.
> > >>>>
> > >>>> We want to use LZ4 fast in order to support compression in lustre
> > >>>> and (mostly, based on that) investigate data reduction techniques in behalf of
> > >>>> storage systems.
> > >>>>
> > >>>> Also, it will be useful for other users of LZ4 compression, as with LZ4 fast
> > >>>> it is possible to enable applications to use fast and/or high compression
> > >>>> depending on the usecase.
> > >>>> For instance, ZRAM is offering a LZ4 backend and could benefit from an updated
> > >>>> LZ4 in the kernel.
> > >>>>
> > >>>> LZ4 homepage: http://www.lz4.org/
> > >>>> LZ4 source repository: https://github.com/lz4/lz4
> > >>>> Source version: 1.7.3
> > >>>>
> > >>>> Benchmark (taken from [1], Core i5-4300U @1.9GHz):
> > >>>> ----------------|--------------|----------------|----------
> > >>>> Compressor      | Compression  | Decompression  | Ratio
> > >>>> ----------------|--------------|----------------|----------
> > >>>> memcpy          |  4200 MB/s   |  4200 MB/s     | 1.000
> > >>>> LZ4 fast 50     |  1080 MB/s   |  2650 MB/s     | 1.375
> > >>>> LZ4 fast 17     |   680 MB/s   |  2220 MB/s     | 1.607
> > >>>> LZ4 fast 5      |   475 MB/s   |  1920 MB/s     | 1.886
> > >>>> LZ4 default     |   385 MB/s   |  1850 MB/s     | 2.101
> > >>>>
> > >>>> [1] http://fastcompression.blogspot.de/2015/04/sampling-or-faster-lz4.html
> > >>>>
> > >>>> [PATCH 1/5] lib: Update LZ4 compressor module
> > >>>> [PATCH 2/5] lib/decompress_unlz4: Change module to work with new LZ4 module version
> > >>>> [PATCH 3/5] crypto: Change LZ4 modules to work with new LZ4 module version
> > >>>> [PATCH 4/5] fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version
> > >>>> [PATCH 5/5] lib/lz4: Remove back-compat wrappers
> > >>>
> > >>> Today, I did zram-lz4 performance test with fio in current mmotm and
> > >>> found it makes regression about 20%.
> > >>>
> > >>> "lz4-update" means current mmots(git://git.cmpxchg.org/linux-mmots.git) so
> > >>> applied your 5 patches. (But now sure current mmots has recent uptodate
> > >>> patches)
> > >>> "revert" means I reverted your 5 patches in current mmots.
> > >>>
> > >>>                      revert    lz4-update
> > >>>
> > >>>       seq-write       1547       1339      86.55%
> > >>>      rand-write      22775      19381      85.10%
> > >>>        seq-read       7035       5589      79.45%
> > >>>       rand-read      78556      68479      87.17%
> > >>>    mixed-seq(R)       1305       1066      81.69%
> > >>>    mixed-seq(W)       1205        984      81.66%
> > >>>   mixed-rand(R)      17421      14993      86.06%
> > >>>   mixed-rand(W)      17391      14968      86.07%
> > >>
> > >> which parts of the output (as well as units) are these values exactly?
> > >> I did not work with fio until now, so I think I might ask before misinterpreting my results.
> > >
> > > It is IOPS.
> > >
> > >>  
> > >>> My fio description file
> > >>>
> > >>> [global]
> > >>> bs=4k
> > >>> ioengine=sync
> > >>> size=100m
> > >>> numjobs=1
> > >>> group_reporting
> > >>> buffer_compress_percentage=30
> > >>> scramble_buffers=0
> > >>> filename=/dev/zram0
> > >>> loops=10
> > >>> fsync_on_close=1
> > >>>
> > >>> [seq-write]
> > >>> bs=64k
> > >>> rw=write
> > >>> stonewall
> > >>>
> > >>> [rand-write]
> > >>> rw=randwrite
> > >>> stonewall
> > >>>
> > >>> [seq-read]
> > >>> bs=64k
> > >>> rw=read
> > >>> stonewall
> > >>>
> > >>> [rand-read]
> > >>> rw=randread
> > >>> stonewall
> > >>>
> > >>> [mixed-seq]
> > >>> bs=64k
> > >>> rw=rw
> > >>> stonewall
> > >>>
> > >>> [mixed-rand]
> > >>> rw=randrw
> > >>> stonewall
> > >>>
> > >>
> > >> Great, this makes it easy for me to reproduce your test.
> > >
> > > If you have trouble to reproduce, feel free to ask me. I'm happy to test it. :)
> > >
> > > Thanks!
> > >
> > 
> > Hi Minchan,
> > 
> > I will send an updated patch as a reply to this E-Mail. Would be really grateful If you'd test it and provide feedback!
> > The patch should be applied to the current mmots tree.
> > 
> > In fact, the updated LZ4 _is_ slower than the current one in kernel. But I was not able to reproduce such large regressions
> > as you did. I now tried to define FORCE_INLINE as Eric suggested. I also inlined some functions which weren't in upstream LZ4,
> > but are defined as macros in the current kernel LZ4. The approach to replace LZ4_ARCH64 with the function call _seemed_ to behave
> > worse than the macro, so I withdrew the change.
> > 
> > The main difference is, that I replaced the read32/read16/write... etc. functions using memcpy with the other ones defined 
> > in upstream LZ4 (which can be switched using a macro). 
> > The comment of the author stated, that they're as fast as the memcpy variants (or faster), but not as portable
> > (which does not matter since we're not dependent for multiple compilers).
> > 
> > In my tests, this version is mostly as fast as the current kernel LZ4.
> 
> With a patch you sent, I cannot see enhancement so I wanted to dig in and
> found how I was really careless.
> 
> I have tested both test with CONFIG_KASAN. OMG. With disabling it, I don't
> see any regression any more. So, I'm really really *sorry* about noise and
> wasting your time. However, I am curious why KASAN makes such difference.
> 

Hey Minchan,

I'm glad to hear that! Nevertheless, the changes discussed here made some differences in my own tests (I believe it got a bit
faster now) and we have the functions properly inlined, where this makes sense. Also, I added the '-O3' C-flag as Eric suggested.
So, this was not really a waste of time, I think.

> The reason I tested new updated lz4 is description says lz4 fast and
> want to use it in zram. How can I do that? and How faster it is compared
> to old?
>

Unfortunately, in the current implementation (in crypto/lz4.c, which is used by zram) I'm setting the acceleration parameter 
(which is the paramer making the compression 'fast', see LZ4_compress_fast) to 1 (which is the default) since I did not know how this
patchset is accepted and this equals the behaviour currently available in kernel.

Basically, the logic is 'higher acceleration = faster compression = lower compression ratio' and vice versa. 
I included some benchmarks in my patch 0/5 E-Mail taken from the official LZ4:

> > >>>> ----------------|--------------|----------------|----------
> > >>>> Compressor      | Compression  | Decompression  | Ratio
> > >>>> ----------------|--------------|----------------|----------
> > >>>> memcpy          |  4200 MB/s   |  4200 MB/s     | 1.000
> > >>>> LZ4 fast 50     |  1080 MB/s   |  2650 MB/s     | 1.375
> > >>>> LZ4 fast 17     |   680 MB/s   |  2220 MB/s     | 1.607
> > >>>> LZ4 fast 5      |   475 MB/s   |  1920 MB/s     | 1.886
> > >>>> LZ4 default     |   385 MB/s   |  1850 MB/s     | 2.101
> > >>>>

fast 50 means: acceleration=50, default: acceleration=1.

Besides the proposed patchset, I tried to implement a module parameter in crypto/lz4.c to set the acceleration factor.
In my tests, the module parameter works out great.
But I think this is subject to a future, separate patch. Especially since I had to 'work around' the crypto/testmgr.c, 
which only tests acceleration=1 and there's no limit for acceleration.  

Thanks for your help,

Sven

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

csiph-web

[PATCH v7 0/5] Update LZ4 compressor module

Contents

#1579790 — Re: [PATCH] lz4: fix performance regressions

#1579324 — Re: [PATCH] lz4: fix performance regressions

#1579329

#1579664