Re: [PATCHv2 1/2] zram: introduce compressing backend abstraction
On Thu, Feb 06, 2014 at 02:16:28PM +0900, Minchan Kim wrote: > 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 > > --- > > 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
Re: [PATCHv2 1/2] zram: introduce compressing backend abstraction
On Thu, Feb 06, 2014 at 02:16:28PM +0900, Minchan Kim wrote: 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.senozhat...@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. Hello Sergey, If I don't show my brain to you, I guess we need more iterations to discuss and it's really waste our time
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 > --- > 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 majord...@vger.kernel.org More majordomo info at
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.senozhat...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
