From: Mike Galbraith <[email protected]>
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 <[email protected]>
Cc: Nitin Gupta <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Signed-off-by: Mike Galbraith <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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(&comp->stream->lock);
+ return this_cpu_ptr(comp->stream);
}
void zcomp_stream_put(struct zcomp *comp)
{
- put_cpu_ptr(comp->stream);
+ local_unlock(&comp->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(&zstrm->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 <linux/local_lock.h>
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
On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> From: Mike Galbraith <[email protected]>
>
> 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 <[email protected]>
> Cc: Nitin Gupta <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Signed-off-by: Mike Galbraith <[email protected]>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Acked-by: Minchan Kim <[email protected]>
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 19f545b6e07f753c4dc639c2f0ab52345733b6a8
Gitweb: https://git.kernel.org/tip/19f545b6e07f753c4dc639c2f0ab52345733b6a8
Author: Mike Galbraith <[email protected]>
AuthorDate: Wed, 27 May 2020 22:11:19 +02:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Thu, 28 May 2020 10:31:10 +02:00
zram: Use local lock to protect per-CPU data
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]
Signed-off-by: Mike Galbraith <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 912e3e6..5ee8e3f 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(&comp->stream->lock);
+ return this_cpu_ptr(comp->stream);
}
void zcomp_stream_put(struct zcomp *comp)
{
- put_cpu_ptr(comp->stream);
+ local_unlock(&comp->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(&zstrm->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 72c2ee4..40f6420 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -5,8 +5,11 @@
#ifndef _ZCOMP_H_
#define _ZCOMP_H_
+#include <linux/local_lock.h>
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;
On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> From: Mike Galbraith <[email protected]>
>
> 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: G W
[ 40.051759] ------------------------------------------------------
[ 40.058685] swapon/586 is trying to acquire lock:
[ 40.063950] ffffe8ffffc0ee60 (&zstrm->lock){+.+.}-{2:2}, at: local_lock_acquire+0x5/0x70 [zram]
[ 40.073739]
[ 40.073739] but task is already holding lock:
[ 40.080277] ffff888101a1f438 (&zspage->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 (&zspage->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 (&zstrm->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] CPU0 CPU1
[ 40.327972] ---- ----
[ 40.333041] lock(&zspage->lock);
[ 40.336874] lock(&zstrm->lock);
[ 40.343424] lock(&zspage->lock);
[ 40.350071] lock(&zstrm->lock);
[ 40.353803]
[ 40.353803] *** DEADLOCK ***
On Sun, Oct 18, 2020 at 6:53 PM Yu Zhao <[email protected]> wrote:
>
> On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> > From: Mike Galbraith <[email protected]>
> >
> > 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/[email protected]/
>
> Thank you.
>
> [ 40.030778] ======================================================
> [ 40.037706] WARNING: possible circular locking dependency detected
> [ 40.044637] 5.9.0-74216-g5c9472ed6825 #1 Tainted: G W
> [ 40.051759] ------------------------------------------------------
> [ 40.058685] swapon/586 is trying to acquire lock:
> [ 40.063950] ffffe8ffffc0ee60 (&zstrm->lock){+.+.}-{2:2}, at: local_lock_acquire+0x5/0x70 [zram]
> [ 40.073739]
> [ 40.073739] but task is already holding lock:
> [ 40.080277] ffff888101a1f438 (&zspage->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 (&zspage->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 (&zstrm->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] CPU0 CPU1
> [ 40.327972] ---- ----
> [ 40.333041] lock(&zspage->lock);
> [ 40.336874] lock(&zstrm->lock);
> [ 40.343424] lock(&zspage->lock);
> [ 40.350071] lock(&zstrm->lock);
> [ 40.353803]
> [ 40.353803] *** DEADLOCK ***
>
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 <[email protected]>
> >
> > 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 <[email protected]>
---
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);