lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 6 Feb 2014 14:16:28 +0900
From:	Minchan Kim <>
To:	Sergey Senozhatsky <>
Cc:	Jerome Marchand <>,
	Nitin Gupta <>,
Subject: Re: [PATCHv2 1/2] zram: introduce compressing backend abstraction

Hello Sergey,

Sorry for late response.

On Thu, Jan 30, 2014 at 10:28:07PM +0300, Sergey Senozhatsky wrote:
> ZRAM performs direct LZO compression algorithm calls, making it the one
> and only option. Introduce struct zram_comp in order to support multiple
> compression algorithms. struct zram_comp defines the following set of
> compressing backend operations:
> 	.create
> 	.destroy
> 	.compress
> 	.decompress
> 	.workmem_get
> 	.workmem_put
> Schematically zram write() usually contains the following steps:
> 0) preparation (decompression of partioal IO, etc.)
> 1) lock buffer_lock mutex (protects meta compress buffers)
> 2) compress (using meta compress buffers)
> 3) alloc and map zs_pool object
> 4) copy compressed data (from meta compress buffers) to new object

                                                          object allocated by 3)
> 5) free previous pool page, assign a new one
> 6) unlock buffer_lock mutex
> As we can see, compressing buffers must remain untouched from 1) to 4),
> because, otherwise, concurrent write() will overwrite data. At the same
> time, zram_meta must be aware of a) specific compression algorithm
> memory requirements and b) necessary locking to protect compression
> buffers. Besides, zram holds buffer_lock almost through the whole write()
> function, making parallel compression impossible. To remove requirement
> a) new struct zcomp_workmem (workmem is a common term used by lzo, lz4
> and zlib) contains buffers required by compression algorithm, while new
> struct zcomp_wm_policy implements workmem handling and locking by means
> of get() and put() semantics and removes requirement b) from zram meta.
> In general, workmem_get() turns caller into exclusive user of workmem
> and workem_put() makes a particular workmem available.
> Each compressing backend may use a default workmem policy or provide
> custom implementation. Default workmem policy (struct zcomp_wm_policy)
> has a list of idle workmem buffers (at least 1 workmem), spinlock to
> protect idle list and wait queue, making it possible to have parallel
> compressions. Each time zram issues a workmem_get() call, the following
> set of operations performed:

I'm still really not sure why backend should know workmem policy.
I think it's matter of upper layer, not backend.
Yeb, surely, you have a reason but it's very hard for me to know it
by this patchset so I'd like to divide the patchset.
(You don't need to explain it in here and I expect it would be clear
if you separate it like I suggested below).
Pz, see below.

> - spin lock buffer_lock
> - if idle list is not empty, remove workmem from idle list, spin
>   unlock and return workmem pointer
> - if idle list is empty, current adds itself to wait queue. it will be
>   awaken by workmem_put() caller.
> workmem_put():
> - spin lock buffer_lock
> - add workmem to idle list
> - spin unlock, wake up sleeper (if any)


> zram_comp file contains array of supported compressing backends, which
> can be altered according to user selection.
> Usage examples. To initialize compressing backend:
> 	comp = zcomp_create(NAME) /* NAME e.g. lzo, lz4 */
> which performs NAME lookup in array of supported compressing backends
> and initialises compressing backend if requested algorithm is supported.
> Compressing:
> 	wm = comp->workmem_get(comp)
> 	comp->compress(...)
> 	[..] /* copy compressed data */
> 	comp->workmem_put(comp, wm)
> Free compessing backend and all ot its workmem buffers:
> 	zcomp_destroy(comp)
> The patch implements LZO and LZ4 backends. At this point zcomp_wm_policy
> keeps only one workmem in the idle list, support for multi workmem buffers
> will be introduced later.
> Signed-off-by: Sergey Senozhatsky <>
> ---
>  drivers/block/zram/zcomp_lz4.c |  49 ++++++++++
>  drivers/block/zram/zcomp_lz4.h |  18 ++++

Please don't include lz4 in this patch. It should be separated and
description of the patch surely should include the number to prove
lz4 is better than lzo in *what* workload so it should make
everybody easy to convince.

And let's separate this patchset following as

1. abstract compressor with zram_comp.
2. Support configurable compressor
3. support zcomp multi buffers
4. support lz4

Please don't add workmem policy in patch 1 because we still use only
a buffer until 3 so patch 1, 2 would be very simple.
Patch 3 might introduce wm policy. Then, it would be very clear
why we need it for zomp_multi so that it would make review easy.

If 1,2,3 have no problem and apparenlty lz4 has a benefit, patch 4
will be merged easily but If lz4 were rejected by some reason,
we could support another compression easily since patch 1,2,3 is


Kind regards,
Minchan Kim
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

Powered by blists - more mailing lists