Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
On Sun, 2020-10-18 at 19:52 -0600, Yu Zhao wrote: > On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote: > > From: Mike Galbraith > > > > The zcomp driver uses per-CPU compression. The per-CPU data pointer is > > acquired with get_cpu_ptr() which implicitly disables preemption. > > It allocates memory inside the preempt disabled region which conflicts > > with the PREEMPT_RT semantics. > > > > Replace the implicit preemption control with an explicit local lock. > > This allows RT kernels to substitute it with a real per CPU lock, which > > serializes the access but keeps the code section preemptible. On non RT > > kernels this maps to preempt_disable() as before, i.e. no functional > > change. > > Hi, > > This change seems to have introduced a potential deadlock. Can you > please take a look? Hm, looks like I'm getting undeserved credit for uncovering a locking bug. In reality, Sebastian was generous with attribution of derivative work, so he should ge credit.. and it looks like peterz fixed it. Date: Fri, 16 Oct 2020 14:40:09 +0200 From: Peter Zijlstra --- diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 9100ac36670a..c1e2c2e1cde8 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1216,10 +1216,11 @@ static void zram_free_page(struct zram *zram, size_t index) static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index, struct bio *bio, bool partial_io) { - int ret; + struct zcomp_strm *zstrm; unsigned long handle; unsigned int size; void *src, *dst; + int ret; zram_slot_lock(zram, index); if (zram_test_flag(zram, index, ZRAM_WB)) { @@ -1250,6 +1251,9 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index, size = zram_get_obj_size(zram, index); + if (size != PAGE_SIZE) + zstrm = zcomp_stream_get(zram->comp); + src = zs_map_object(zram->mem_pool, handle, ZS_MM_RO); if (size == PAGE_SIZE) { dst = kmap_atomic(page); @@ -1257,8 +1261,6 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index, kunmap_atomic(dst); ret = 0; } else { - struct zcomp_strm *zstrm = zcomp_stream_get(zram->comp); - dst = kmap_atomic(page); ret = zcomp_decompress(zstrm, src, size, dst); kunmap_atomic(dst);
Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
On Sun, Oct 18, 2020 at 6:53 PM Yu Zhao wrote: > > On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote: > > From: Mike Galbraith > > > > The zcomp driver uses per-CPU compression. The per-CPU data pointer is > > acquired with get_cpu_ptr() which implicitly disables preemption. > > It allocates memory inside the preempt disabled region which conflicts > > with the PREEMPT_RT semantics. > > > > Replace the implicit preemption control with an explicit local lock. > > This allows RT kernels to substitute it with a real per CPU lock, which > > serializes the access but keeps the code section preemptible. On non RT > > kernels this maps to preempt_disable() as before, i.e. no functional > > change. > > Hi, > > This change seems to have introduced a potential deadlock. Can you > please take a look? Probably needs Peter's fix https://lore.kernel.org/lkml/20201016124009.gq2...@hirez.programming.kicks-ass.net/ > > Thank you. > > [ 40.030778] == > [ 40.037706] WARNING: possible circular locking dependency detected > [ 40.044637] 5.9.0-74216-g5c9472ed6825 #1 Tainted: GW > [ 40.051759] -- > [ 40.058685] swapon/586 is trying to acquire lock: > [ 40.063950] e8c0ee60 (>lock){+.+.}-{2:2}, at: > local_lock_acquire+0x5/0x70 [zram] > [ 40.073739] > [ 40.073739] but task is already holding lock: > [ 40.080277] 888101a1f438 (>lock){.+.+}-{2:2}, at: > zs_map_object+0x73/0x28d > [ 40.089182] > [ 40.089182] which lock already depends on the new lock. > [ 40.089182] > [ 40.098344] > [ 40.098344] the existing dependency chain (in reverse order) is: > [ 40.106715] > [ 40.106715] -> #1 (>lock){.+.+}-{2:2}: > [ 40.113386]lock_acquire+0x1cd/0x2c3 > [ 40.118083]_raw_read_lock+0x44/0x78 > [ 40.122781]zs_map_object+0x73/0x28d > [ 40.127479]zram_bvec_rw+0x42e/0x75d [zram] > [ 40.132855]zram_submit_bio+0x1fc/0x2d7 [zram] > [ 40.138526]submit_bio_noacct+0x11b/0x372 > [ 40.143709]submit_bio+0xfd/0x1b5 > [ 40.148113]__block_write_full_page+0x302/0x56f > [ 40.153877]__writepage+0x1e/0x74 > [ 40.158281]write_cache_pages+0x404/0x59a > [ 40.163461]generic_writepages+0x53/0x82 > [ 40.168545]do_writepages+0x33/0x74 > [ 40.173145]__filemap_fdatawrite_range+0x91/0xac > [ 40.179005]file_write_and_wait_range+0x39/0x87 > [ 40.184769]blkdev_fsync+0x19/0x3e > [ 40.189272]do_fsync+0x39/0x5c > [ 40.193384]__x64_sys_fsync+0x13/0x17 > [ 40.198178]do_syscall_64+0x37/0x45 > [ 40.202776]entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 40.209022] > [ 40.209022] -> #0 (>lock){+.+.}-{2:2}: > [ 40.215589]validate_chain+0x1966/0x21a8 > [ 40.220673]__lock_acquire+0x941/0xbba > [ 40.225552]lock_acquire+0x1cd/0x2c3 > [ 40.230250]local_lock_acquire+0x21/0x70 [zram] > [ 40.236015]zcomp_stream_get+0x33/0x4d [zram] > [ 40.241585]zram_bvec_rw+0x476/0x75d [zram] > [ 40.246963]zram_rw_page+0xd8/0x17c [zram] > [ 40.252240]bdev_read_page+0x7a/0x9d > [ 40.256933]do_mpage_readpage+0x6b2/0x860 > [ 40.262101]mpage_readahead+0x136/0x245 > [ 40.267089]read_pages+0x60/0x1f9 > [ 40.271492]page_cache_ra_unbounded+0x211/0x27b > [ 40.277251]generic_file_buffered_read+0x188/0xd4d > [ 40.283296]new_sync_read+0x10c/0x143 > [ 40.288088]vfs_read+0xf4/0x1a5 > [ 40.292285]ksys_read+0x73/0xd3 > [ 40.296483]do_syscall_64+0x37/0x45 > [ 40.301072]entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 40.307319] > [ 40.307319] other info that might help us debug this: > [ 40.307319] > [ 40.316285] Possible unsafe locking scenario: > [ 40.316285] > [ 40.322907]CPU0CPU1 > [ 40.327972] > [ 40.333041] lock(>lock); > [ 40.336874]lock(>lock); > [ 40.343424]lock(>lock); > [ 40.350071] lock(>lock); > [ 40.353803] > [ 40.353803] *** DEADLOCK *** >
Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote: > From: Mike Galbraith > > The zcomp driver uses per-CPU compression. The per-CPU data pointer is > acquired with get_cpu_ptr() which implicitly disables preemption. > It allocates memory inside the preempt disabled region which conflicts > with the PREEMPT_RT semantics. > > Replace the implicit preemption control with an explicit local lock. > This allows RT kernels to substitute it with a real per CPU lock, which > serializes the access but keeps the code section preemptible. On non RT > kernels this maps to preempt_disable() as before, i.e. no functional > change. Hi, This change seems to have introduced a potential deadlock. Can you please take a look? Thank you. [ 40.030778] == [ 40.037706] WARNING: possible circular locking dependency detected [ 40.044637] 5.9.0-74216-g5c9472ed6825 #1 Tainted: GW [ 40.051759] -- [ 40.058685] swapon/586 is trying to acquire lock: [ 40.063950] e8c0ee60 (>lock){+.+.}-{2:2}, at: local_lock_acquire+0x5/0x70 [zram] [ 40.073739] [ 40.073739] but task is already holding lock: [ 40.080277] 888101a1f438 (>lock){.+.+}-{2:2}, at: zs_map_object+0x73/0x28d [ 40.089182] [ 40.089182] which lock already depends on the new lock. [ 40.089182] [ 40.098344] [ 40.098344] the existing dependency chain (in reverse order) is: [ 40.106715] [ 40.106715] -> #1 (>lock){.+.+}-{2:2}: [ 40.113386]lock_acquire+0x1cd/0x2c3 [ 40.118083]_raw_read_lock+0x44/0x78 [ 40.122781]zs_map_object+0x73/0x28d [ 40.127479]zram_bvec_rw+0x42e/0x75d [zram] [ 40.132855]zram_submit_bio+0x1fc/0x2d7 [zram] [ 40.138526]submit_bio_noacct+0x11b/0x372 [ 40.143709]submit_bio+0xfd/0x1b5 [ 40.148113]__block_write_full_page+0x302/0x56f [ 40.153877]__writepage+0x1e/0x74 [ 40.158281]write_cache_pages+0x404/0x59a [ 40.163461]generic_writepages+0x53/0x82 [ 40.168545]do_writepages+0x33/0x74 [ 40.173145]__filemap_fdatawrite_range+0x91/0xac [ 40.179005]file_write_and_wait_range+0x39/0x87 [ 40.184769]blkdev_fsync+0x19/0x3e [ 40.189272]do_fsync+0x39/0x5c [ 40.193384]__x64_sys_fsync+0x13/0x17 [ 40.198178]do_syscall_64+0x37/0x45 [ 40.202776]entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 40.209022] [ 40.209022] -> #0 (>lock){+.+.}-{2:2}: [ 40.215589]validate_chain+0x1966/0x21a8 [ 40.220673]__lock_acquire+0x941/0xbba [ 40.225552]lock_acquire+0x1cd/0x2c3 [ 40.230250]local_lock_acquire+0x21/0x70 [zram] [ 40.236015]zcomp_stream_get+0x33/0x4d [zram] [ 40.241585]zram_bvec_rw+0x476/0x75d [zram] [ 40.246963]zram_rw_page+0xd8/0x17c [zram] [ 40.252240]bdev_read_page+0x7a/0x9d [ 40.256933]do_mpage_readpage+0x6b2/0x860 [ 40.262101]mpage_readahead+0x136/0x245 [ 40.267089]read_pages+0x60/0x1f9 [ 40.271492]page_cache_ra_unbounded+0x211/0x27b [ 40.277251]generic_file_buffered_read+0x188/0xd4d [ 40.283296]new_sync_read+0x10c/0x143 [ 40.288088]vfs_read+0xf4/0x1a5 [ 40.292285]ksys_read+0x73/0xd3 [ 40.296483]do_syscall_64+0x37/0x45 [ 40.301072]entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 40.307319] [ 40.307319] other info that might help us debug this: [ 40.307319] [ 40.316285] Possible unsafe locking scenario: [ 40.316285] [ 40.322907]CPU0CPU1 [ 40.327972] [ 40.333041] lock(>lock); [ 40.336874]lock(>lock); [ 40.343424]lock(>lock); [ 40.350071] lock(>lock); [ 40.353803] [ 40.353803] *** DEADLOCK ***
Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote: > From: Mike Galbraith > > The zcomp driver uses per-CPU compression. The per-CPU data pointer is > acquired with get_cpu_ptr() which implicitly disables preemption. > It allocates memory inside the preempt disabled region which conflicts > with the PREEMPT_RT semantics. > > Replace the implicit preemption control with an explicit local lock. > This allows RT kernels to substitute it with a real per CPU lock, which > serializes the access but keeps the code section preemptible. On non RT > kernels this maps to preempt_disable() as before, i.e. no functional > change. > > [bigeasy: Use local_lock(), description, drop reordering] > > Cc: Minchan Kim > Cc: Nitin Gupta > Cc: Sergey Senozhatsky > Signed-off-by: Mike Galbraith > Signed-off-by: Sebastian Andrzej Siewior Acked-by: Minchan Kim
[PATCH v3 7/7] zram: Use local lock to protect per-CPU data
From: Mike Galbraith The zcomp driver uses per-CPU compression. The per-CPU data pointer is acquired with get_cpu_ptr() which implicitly disables preemption. It allocates memory inside the preempt disabled region which conflicts with the PREEMPT_RT semantics. Replace the implicit preemption control with an explicit local lock. This allows RT kernels to substitute it with a real per CPU lock, which serializes the access but keeps the code section preemptible. On non RT kernels this maps to preempt_disable() as before, i.e. no functional change. [bigeasy: Use local_lock(), description, drop reordering] Cc: Minchan Kim Cc: Nitin Gupta Cc: Sergey Senozhatsky Signed-off-by: Mike Galbraith Signed-off-by: Sebastian Andrzej Siewior --- drivers/block/zram/zcomp.c | 7 +-- drivers/block/zram/zcomp.h | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c index 912e3e63d8a09..5ee8e3fae5516 100644 --- a/drivers/block/zram/zcomp.c +++ b/drivers/block/zram/zcomp.c @@ -110,12 +110,13 @@ ssize_t zcomp_available_show(const char *comp, char *buf) struct zcomp_strm *zcomp_stream_get(struct zcomp *comp) { - return get_cpu_ptr(comp->stream); + local_lock(>stream->lock); + return this_cpu_ptr(comp->stream); } void zcomp_stream_put(struct zcomp *comp) { - put_cpu_ptr(comp->stream); + local_unlock(>stream->lock); } int zcomp_compress(struct zcomp_strm *zstrm, @@ -159,6 +160,8 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node) int ret; zstrm = per_cpu_ptr(comp->stream, cpu); + local_lock_init(>lock); + ret = zcomp_strm_init(zstrm, comp); if (ret) pr_err("Can't allocate a compression stream\n"); diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h index 72c2ee4d843ed..40f6420f4b2e9 100644 --- a/drivers/block/zram/zcomp.h +++ b/drivers/block/zram/zcomp.h @@ -5,8 +5,11 @@ #ifndef _ZCOMP_H_ #define _ZCOMP_H_ +#include struct zcomp_strm { + /* The members ->buffer and ->tfm are protected by ->lock. */ + local_lock_t lock; /* compression/decompression buffer */ void *buffer; struct crypto_comp *tfm; -- 2.27.0.rc0