Messages in this thread | | | Date | Thu, 6 Feb 2014 14:16:28 +0900 | From | Minchan Kim <> | 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)
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@gmail.com> > --- > 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
| |