Re: [PATCHv2 1/2] zram: introduce compressing backend abstraction

From: Minchan Kim
Date: Thu Feb 06 2014 - 00:16:57 EST


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)

Good.

>
> 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 <sergey.senozhatsky@xxxxxxxxx>
> ---
> 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
merged.

Thanks.

--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/