Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567159 > unrolled thread

[PATCH v5 0/5] Update LZ4 compressor module

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.

Contents

  [PATCH v5 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-01-26 09:00 +0100
    [PATCH v5 5/5] lib/lz4: Remove back-compat wrappers Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-01-26 09:00 +0100
    Re: [PATCH v5 0/5] Update LZ4 compressor module Eric Biggers <ebiggers3@gmail.com> - 2017-01-26 10:30 +0100
      Re: [PATCH v5 0/5] Update LZ4 compressor module Sven Schmidt <4sschmid@informatik.uni-hamburg.de> - 2017-01-26 15:20 +0100

#1567159 — [PATCH v5 0/5] Update LZ4 compressor module

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
fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version

[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

v2:
- Changed order of the patches since in the initial patchset the lz4.h was in the
  last patch but was referenced by the other ones
- Split lib/decompress_unlz4.c in an own patch
- Fixed errors reported by the buildbot
- Further refactorings
- Added more appropriate copyright note to include/linux/lz4.h

v3:
- Adjusted the code to satisfy kernel coding style (checkpatch.pl)
- Made sure the changes to LZ4 in Kernel (overflow checks etc.)
  are included in the new module (they are)
- Removed the second LZ4_compressBound function with related name but
  different return type
- Corrected version number (was LZ4 1.7.3)
- Added missing LZ4 streaming functions

v4:
- Fixed kbuild errors
- Re-added lz4_compressbound as alias for LZ4_compressBound
  to ensure backwards compatibility
- Wrapped LZ4_hash5 with check for LZ4_ARCH64 since it is only used there
  and triggers an unused function warning when false

v5:
- Added a fifth patch to remove the back-compat wrappers introduced
  to ensure bisectibility between the patches (the functions are no longer
  needed since there's no callers left)

[toc] | [next] | [standalone]

#1567160 — [PATCH v5 5/5] lib/lz4: Remove back-compat wrappers

This patch removes the functions introduced as wrappers for providing
backwards compatibility to the prior LZ4 version.
They're not needed anymore since there's no callers left.

Signed-off-by: Sven Schmidt <4sschmid@informatik.uni-hamburg.de>
---
 include/linux/lz4.h      | 73 ------------------------------------------------
 lib/lz4/lz4_compress.c   | 22 ---------------
 lib/lz4/lz4_decompress.c | 42 ----------------------------
 lib/lz4/lz4hc_compress.c | 23 ---------------
 4 files changed, 160 deletions(-)

diff --git a/include/linux/lz4.h b/include/linux/lz4.h
index 2072255..5958f7d 100644
--- a/include/linux/lz4.h
+++ b/include/linux/lz4.h
@@ -173,14 +173,6 @@ static inline int LZ4_compressBound(size_t isize)
 }
 
 /*
- * For backward compatibility
- */
-static inline int lz4_compressbound(size_t isize)
-{
-	return LZ4_COMPRESSBOUND(isize);
-}
-
-/*
  * LZ4_compress_default()
  *	Compresses 'sourceSize' bytes from buffer 'source'
  *	into already allocated 'dest' buffer of size 'maxOutputSize'.
@@ -249,23 +241,6 @@ int LZ4_compress_fast(const char *source, char *dest, int inputSize,
 int LZ4_compress_destSize(const char *source, char *dest, int *sourceSizePtr,
 	int targetDestSize, void *wrkmem);
 
-/*
- * lz4_compress()
- *	src    : source address of the original data
- *	src_len: size of the original data
- *	dst    : output buffer address of the compressed data
- *           This requires 'dst' of size LZ4_COMPRESSBOUND.
- *	dst_len: is the output size, which is returned after compress done
- *	workmem: address of the working memory.
- *           This requires 'workmem' of size LZ4_MEM_COMPRESS.
- *	return	: Success if return 0
- *            Error if return (< 0)
- *	note :	Destination buffer and workmem must be already allocated with
- *		the defined size.
- */
-int lz4_compress(const unsigned char *src, size_t src_len, unsigned char *dst,
-	size_t *dst_len, void *wrkmem);
-
 /*-************************************************************************
  *	Decompression Functions
  **************************************************************************/
@@ -340,37 +315,6 @@ int LZ4_decompress_safe(const char *source, char *dest, int compressedSize,
 int LZ4_decompress_safe_partial(const char *source, char *dest,
 	int compressedSize, int targetOutputSize, int maxDecompressedSize);
 
-
-/*
- * lz4_decompress_unknownoutputsize() :
- *	src     : source address of the compressed data
- *	src_len : is the input size, therefore the compressed size
- *	dest    : output buffer address of the decompressed data
- *	dest_len: is the max size of the destination buffer, which is
- *            returned with actual size of decompressed data after
- *            decompress done
- * return: Success if return 0
- *         Error if return (< 0)
- * note:   Destination buffer must be already allocated.
- */
-int lz4_decompress_unknownoutputsize(const unsigned char *src, size_t src_len,
-	unsigned char *dest, size_t *dest_len);
-
-/*
- * lz4_decompress() :
- *	src            : source address of the compressed data
- *	src_len        : is the input size,
- *                   which is returned after decompress done
- *	dest           : output buffer address of the decompressed data
- *	actual_dest_len: is the size of uncompressed data, supposing it's known
- *	return: Success if return 0
- *          Error if return (< 0)
- *	note  : Destination buffer must be already allocated.
- *          slightly faster than lz4_decompress_unknownoutputsize()
- */
-int lz4_decompress(const unsigned char *src, size_t *src_len,
-	unsigned char *dest, size_t actual_dest_len);
-
 /*-************************************************************************
  *	LZ4 HC Compression
  **************************************************************************/
@@ -399,23 +343,6 @@ int LZ4_compress_HC(const char *src, char *dst, int srcSize, int dstCapacity,
 	int compressionLevel, void *wrkmem);
 
 /*
- * lz4hc_compress()
- *	src    : source address of the original data
- *	src_len: size of the original data
- *	dst    : output buffer address of the compressed data
- *           This requires 'dst' of size LZ4_COMPRESSBOUND.
- *	dst_len: is the output size, which is returned after compress done
- *	workmem: address of the working memory.
- *           This requires 'workmem' of size LZ4HC_MEM_COMPRESS.
- * return	: Success if return 0
- *          Error if return (< 0)
- * note   : Destination buffer and workmem must be already allocated with
- *          the defined size.
- */
-int lz4hc_compress(const unsigned char *src, size_t src_len, unsigned char *dst,
-	size_t *dst_len, void *wrkmem);
-
-/*
  *	These functions compress data in successive blocks of any size,
  *	using previous blocks as dictionary. One key assumption is that previous
  *	blocks (up to 64 KB) remain read-accessible while
diff --git a/lib/lz4/lz4_compress.c b/lib/lz4/lz4_compress.c
index e595896..b625226 100644
--- a/lib/lz4/lz4_compress.c
+++ b/lib/lz4/lz4_compress.c
@@ -872,27 +872,5 @@ int LZ4_compress_fast_continue(LZ4_stream_t *LZ4_stream, const char *source,
 }
 EXPORT_SYMBOL(LZ4_compress_fast_continue);
 
-/*-******************************
- *	For backwards compatibility
- ********************************/
-int lz4_compress(const unsigned char *src, size_t src_len, unsigned char *dst,
-	size_t *dst_len, void *wrkmem) {
-	*dst_len = LZ4_compress_default(src, dst, (int)src_len,
-		(int)((size_t)dst_len), wrkmem);
-
-	/*
-	 * Prior lz4_compress will return -1 in case of error
-	 * and 0 on success
-	 * while new LZ4_compress_fast/default
-	 * returns 0 in case of error
-	 * and the output length on success
-	 */
-	if (!dst_len)
-		return -1;
-	else
-		return 0;
-}
-EXPORT_SYMBOL(lz4_compress);
-
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_DESCRIPTION("LZ4 compressor");
diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
index ca71375..053d09d 100644
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -483,47 +483,5 @@ int LZ4_decompress_fast_usingDict(const char *source, char *dest,
 }
 EXPORT_SYMBOL(LZ4_decompress_fast_usingDict);
 
-/*-******************************
- *	For backwards compatibility
- ********************************/
-int lz4_decompress_unknownoutputsize(const unsigned char *src,
-	size_t src_len, unsigned char *dest, size_t *dest_len) {
-	 *dest_len = LZ4_decompress_safe(src, dest,
-		 (int)src_len, (int)((size_t)dest_len));
-
-		/*
-		 * Prior lz4_decompress_unknownoutputsize will return
-		 * 0 for success and a negative result for error
-		 * new LZ4_decompress_safe returns
-		 * - the length of data read on success
-		 * - and also a negative result on error
-		 * meaning when result > 0, we just return 0 here
-		 */
-		if (src_len > 0) {
-			return 0;
-		} else
-			return -1;
-}
-EXPORT_SYMBOL(lz4_decompress_unknownoutputsize);
-
-int lz4_decompress(const unsigned char *src, size_t *src_len,
-	unsigned char *dest, size_t actual_dest_len) {
-	*src_len = LZ4_decompress_fast(src, dest, (int)actual_dest_len);
-
-	/*
-	 * Prior lz4_decompress will return
-	 * 0 for success and a negative result for error
-	 * new LZ4_decompress_fast returns
-	 * - the length of data read on success
-	 * - and also a negative result on error
-	 * meaning when result > 0, we just return 0 here
-	 */
-	if ((int)((size_t)src_len) > 0)
-		return 0;
-	else
-		return -1;
-}
-EXPORT_SYMBOL(lz4_decompress);
-
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_DESCRIPTION("LZ4 decompressor");
diff --git a/lib/lz4/lz4hc_compress.c b/lib/lz4/lz4hc_compress.c
index 880c778..a29dad0 100644
--- a/lib/lz4/lz4hc_compress.c
+++ b/lib/lz4/lz4hc_compress.c
@@ -603,29 +603,6 @@ int LZ4_compress_HC(const char *src, char *dst, int srcSize,
 }
 EXPORT_SYMBOL(LZ4_compress_HC);
 
-/*-******************************
- *	For backwards compatibility
- ********************************/
-int lz4hc_compress(const unsigned char *src, size_t src_len,
-	unsigned char *dst, size_t *dst_len, void *wrkmem)
-{
-	*dst_len = LZ4_compress_HC(src, dst, (int)src_len,
-		(int)((size_t)dst_len), LZ4HC_DEFAULT_CLEVEL, wrkmem);
-
-	/*
-	 * Prior lz4hc_compress will return -1 in case of error
-	 * and 0 on success
-	 * while new LZ4_compress_HC
-	 * returns 0 in case of error
-	 * and the output length on success
-	 */
-	if (!dst_len)
-		return -1;
-	else
-		return 0;
-}
-EXPORT_SYMBOL(lz4hc_compress);
-
 /**************************************
  *	Streaming Functions
  **************************************/
-- 
2.1.4

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

#1567215

On Thu, Jan 26, 2017 at 08:57:30AM +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.
> 

Hi Sven,

[For some reason I didn't receive patch 1/5 and had to get it from patchwork...
I'm not sure why.  I'm subscribed to linux-crypto but not linux-kernel.]

The proposed patch defines LZ4_MEMORY_USAGE to 10 which means that LZ4
compression will use a hash table of only 1024 bytes, containing only 256
entries, to find matches.  This differs from upstream LZ4 1.7.3, which uses
LZ4_MEMORY_USAGE of 14, as well as the previous LZ4 included in the Linux
kernel, both of which specify the hash table size to be 16384 bytes, containing
4096 entries.

Given that varying the hash table size is a trade-off between memory usage,
speed, and compression ratio, is this an intentional difference and has it been
benchmarked?

Also, in lz4defs.h:

> #if defined(__x86_64__)
>        typedef U64             reg_t;   /* 64-bits in x32 mode */
> #else
>        typedef size_t reg_t;    /* 32-bits in x32 mode */
> #endif

Are you sure this really needed over just always using size_t?

> #if LZ4_ARCH64
> #ifdef __BIG_ENDIAN__
> #define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
> #else
> #define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
> #endif
> #else
> #ifdef __BIG_ENDIAN__
> #define LZ4_NBCOMMONBYTES(val) (__builtin_clz(val) >> 3)
> #else
> #define LZ4_NBCOMMONBYTES(val) (__builtin_ctz(val) >> 3)
> #endif
> #endif

LZ4_NBCOMMONBYTES() is defined incorrectly for 64-bit little endian; it should
be using __builtin_ctzll().

Nit: can you also clean up the weird indentation (e.g. double tabs) in
lz4defs.h?

Thanks,

Eric

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

#1567452

On Thu, Jan 26, 2017 at 01:19:53AM -0800, Eric Biggers wrote:
> On Thu, Jan 26, 2017 at 08:57:30AM +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.
> > 
>

Hey Eric,
 
> Hi Sven,
> 
> [For some reason I didn't receive patch 1/5 and had to get it from patchwork...
> I'm not sure why.  I'm subscribed to linux-crypto but not linux-kernel.]

that's weird. I just experienced the first patch takes a little longer to get delivered because of its size.
Please let me know if the problem occurs again.
 
> The proposed patch defines LZ4_MEMORY_USAGE to 10 which means that LZ4
> compression will use a hash table of only 1024 bytes, containing only 256
> entries, to find matches.  This differs from upstream LZ4 1.7.3, which uses
> LZ4_MEMORY_USAGE of 14, as well as the previous LZ4 included in the Linux
> kernel, both of which specify the hash table size to be 16384 bytes, containing
> 4096 entries.
> 
> Given that varying the hash table size is a trade-off between memory usage,
> speed, and compression ratio, is this an intentional difference and has it been
> benchmarked?
> 

I believe I had some troubles with LZ4_MEMORY_USAGE of 14. But I may be wrong.
I will test that again and eventually adapt that value.

> Also, in lz4defs.h:
> 
> > #if defined(__x86_64__)
> >        typedef U64             reg_t;   /* 64-bits in x32 mode */
> > #else
> >        typedef size_t reg_t;    /* 32-bits in x32 mode */
> > #endif
> 
> Are you sure this really needed over just always using size_t?
>

No, actually there's just one use of that value and the upstream version uses size_t instead of reg_t 
in that particular place. So I will replace it with size_t.
 
> > #if LZ4_ARCH64
> > #ifdef __BIG_ENDIAN__
> > #define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
> > #else
> > #define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
> > #endif
> > #else
> > #ifdef __BIG_ENDIAN__
> > #define LZ4_NBCOMMONBYTES(val) (__builtin_clz(val) >> 3)
> > #else
> > #define LZ4_NBCOMMONBYTES(val) (__builtin_ctz(val) >> 3)
> > #endif
> > #endif
> 
> LZ4_NBCOMMONBYTES() is defined incorrectly for 64-bit little endian; it should
> be using __builtin_ctzll().
> 

Indeed! Using the same values in if and else does not make sense at all.
Thank you for pointing that one out. I will fix it.

> Nit: can you also clean up the weird indentation (e.g. double tabs) in
> lz4defs.h?
> 
> Thanks,
> 
> Eric
> 

I'm wondering why checkpatch does not point out this kind of styling problem?
I did fix that in the other files but I think I missed lz4defs.h. Will fix the indentation.

Thanks,

Sven

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web