[PATCH 0/2] zram: lockmap tweaks

[PATCH 0/2] zram: lockmap tweaks

[Sebastian Andrzej Siewior]

Sergey, this should be a follow up on 
	https://lore.kernel.org/all/akza1qtgHm7zbjMW@google.com/

Could you try if this does what I claimed it does?

Sebastian Andrzej Siewior (2):
  zram: Move lockmap to be per-zram instead per table
  zram: Use a custom key for each zram object.

 drivers/block/zram/zram_drv.c | 26 +++++++++-----------------
 drivers/block/zram/zram_drv.h |  3 ++-
 2 files changed, 11 insertions(+), 18 deletions(-)

-- 
2.53.0

[PATCH 1/2] zram: Move lockmap to be per-zram instead per table

[Sebastian Andrzej Siewior]

The zram object contains an array zram_table_entry. Each one has a
`lock' variable and each has a matching struct lockdep_map. This mimics
a struct mutex.

It uses always the same key for all lockdep_map instances. This makes it
look like the same lock to lockdep. Therefore it could be reduced to
have one lockdep_map per struct zram.

Use only one struct lockdep_map per struct zram.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/block/zram/zram_drv.c | 21 +++++++++------------
 drivers/block/zram/zram_drv.h |  2 +-
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ace65c586072b..4223002d80b41 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -57,14 +57,12 @@ static size_t huge_class_size;
 static const struct block_device_operations zram_devops;
 
 static void slot_free(struct zram *zram, u32 index);
-#define slot_dep_map(zram, index) (&(zram)->table[(index)].dep_map)
 
-static void slot_lock_init(struct zram *zram, u32 index)
+static void slot_lock_init(struct zram *zram)
 {
 	static struct lock_class_key __key;
 
-	lockdep_init_map(slot_dep_map(zram, index), "zram->table[index].lock",
-			 &__key, 0);
+	lockdep_init_map(&zram->table_lock_map, "zram->table[index].lock", &__key, 0);
 }
 
 /*
@@ -84,8 +82,8 @@ static __must_check bool slot_trylock(struct zram *zram, u32 index)
 	unsigned long *lock = &zram->table[index].__lock;
 
 	if (!test_and_set_bit_lock(ZRAM_ENTRY_LOCK, lock)) {
-		mutex_acquire(slot_dep_map(zram, index), 0, 1, _RET_IP_);
-		lock_acquired(slot_dep_map(zram, index), _RET_IP_);
+		mutex_acquire(&zram->table_lock_map, 0, 1, _RET_IP_);
+		lock_acquired(&zram->table_lock_map, _RET_IP_);
 		return true;
 	}
 
@@ -96,16 +94,16 @@ static void slot_lock(struct zram *zram, u32 index)
 {
 	unsigned long *lock = &zram->table[index].__lock;
 
-	mutex_acquire(slot_dep_map(zram, index), 0, 0, _RET_IP_);
+	mutex_acquire(&zram->table_lock_map, 0, 0, _RET_IP_);
 	wait_on_bit_lock(lock, ZRAM_ENTRY_LOCK, TASK_UNINTERRUPTIBLE);
-	lock_acquired(slot_dep_map(zram, index), _RET_IP_);
+	lock_acquired(&zram->table_lock_map, _RET_IP_);
 }
 
 static void slot_unlock(struct zram *zram, u32 index)
 {
 	unsigned long *lock = &zram->table[index].__lock;
 
-	mutex_release(slot_dep_map(zram, index), _RET_IP_);
+	mutex_release(&zram->table_lock_map, _RET_IP_);
 	clear_and_wake_up_bit(ZRAM_ENTRY_LOCK, lock);
 }
 
@@ -1984,7 +1982,7 @@ static void zram_meta_free(struct zram *zram, u64 disksize)
 
 static bool zram_meta_alloc(struct zram *zram, u64 disksize)
 {
-	size_t num_pages, index;
+	size_t num_pages;
 
 	num_pages = disksize >> PAGE_SHIFT;
 	zram->table = vzalloc(array_size(num_pages, sizeof(*zram->table)));
@@ -2001,8 +1999,7 @@ static bool zram_meta_alloc(struct zram *zram, u64 disksize)
 	if (!huge_class_size)
 		huge_class_size = zs_huge_class_size(zram->mem_pool);
 
-	for (index = 0; index < num_pages; index++)
-		slot_lock_init(zram, index);
+	slot_lock_init(zram);
 
 	return true;
 }
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 08d1774c15db6..8290dc6e23352 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -74,7 +74,6 @@ struct zram_table_entry {
 #endif
 		} attr;
 	};
-	struct lockdep_map dep_map;
 };
 
 struct zram_stats {
@@ -107,6 +106,7 @@ struct zram_stats {
 
 struct zram {
 	struct zram_table_entry *table;
+	struct lockdep_map table_lock_map;
 	struct zs_pool *mem_pool;
 	struct zcomp *comps[ZRAM_MAX_COMPS];
 	struct zcomp_params params[ZRAM_MAX_COMPS];
-- 
2.53.0

Re: [PATCH 1/2] zram: Move lockmap to be per-zram instead per table

[Sergey Senozhatsky]

On (26/07/14 16:12), Sebastian Andrzej Siewior wrote:
> The zram object contains an array zram_table_entry. Each one has a
> `lock' variable and each has a matching struct lockdep_map. This mimics
> a struct mutex.
> 
> It uses always the same key for all lockdep_map instances. This makes it
> look like the same lock to lockdep. Therefore it could be reduced to
> have one lockdep_map per struct zram.
> 
> Use only one struct lockdep_map per struct zram.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>

[PATCH 2/2] zram: Use a custom key for each zram object.

[Sebastian Andrzej Siewior]

Each struct zram uses the same key for its struct lockdep_map which is
used for locking analysis.
According to Sergey the lock chains might be different if zram1 is used
for and zram2 is for ext4. This might lead to false dead lock reports if
it mixes a zram1 chain with a zram2. This can be avoided if each lockmap
gets its own unique key.c

Use a dynamic lock_class_key for the table_lock_map.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/block/zram/zram_drv.c | 11 +++--------
 drivers/block/zram/zram_drv.h |  1 +
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4223002d80b41..4bfe63a5225d8 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -58,13 +58,6 @@ static const struct block_device_operations zram_devops;
 
 static void slot_free(struct zram *zram, u32 index);
 
-static void slot_lock_init(struct zram *zram)
-{
-	static struct lock_class_key __key;
-
-	lockdep_init_map(&zram->table_lock_map, "zram->table[index].lock", &__key, 0);
-}
-
 /*
  * entry locking rules:
  *
@@ -1978,6 +1971,7 @@ static void zram_meta_free(struct zram *zram, u64 disksize)
 	zs_destroy_pool(zram->mem_pool);
 	vfree(zram->table);
 	zram->table = NULL;
+	lockdep_unregister_key(&zram->table_lock_key);
 }
 
 static bool zram_meta_alloc(struct zram *zram, u64 disksize)
@@ -1999,7 +1993,8 @@ static bool zram_meta_alloc(struct zram *zram, u64 disksize)
 	if (!huge_class_size)
 		huge_class_size = zs_huge_class_size(zram->mem_pool);
 
-	slot_lock_init(zram);
+	lockdep_register_key(&zram->table_lock_key);
+	lockdep_init_map(&zram->table_lock_map, "zram->table[index].lock", &zram->table_lock_key, 0);
 
 	return true;
 }
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 8290dc6e23352..4fddc582f3b82 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -107,6 +107,7 @@ struct zram_stats {
 struct zram {
 	struct zram_table_entry *table;
 	struct lockdep_map table_lock_map;
+	struct lock_class_key table_lock_key;
 	struct zs_pool *mem_pool;
 	struct zcomp *comps[ZRAM_MAX_COMPS];
 	struct zcomp_params params[ZRAM_MAX_COMPS];
-- 
2.53.0

Re: [PATCH 2/2] zram: Use a custom key for each zram object.

[Sergey Senozhatsky]

Hello, sorry for the delay

On (26/07/14 16:13), Sebastian Andrzej Siewior wrote:
> Each struct zram uses the same key for its struct lockdep_map which is
> used for locking analysis.
> According to Sergey the lock chains might be different if zram1 is used
> for and zram2 is for ext4. This might lead to false dead lock reports if

    ^^ swap  (a minor nit)

> it mixes a zram1 chain with a zram2. This can be avoided if each lockmap
> gets its own unique key.c
> 
> Use a dynamic lock_class_key for the table_lock_map.
> 

[..]

>  static bool zram_meta_alloc(struct zram *zram, u64 disksize)
> @@ -1999,7 +1993,8 @@ static bool zram_meta_alloc(struct zram *zram, u64 disksize)
>  	if (!huge_class_size)
>  		huge_class_size = zs_huge_class_size(zram->mem_pool);
>  
> -	slot_lock_init(zram);
> +	lockdep_register_key(&zram->table_lock_key);
> +	lockdep_init_map(&zram->table_lock_map, "zram->table[index].lock", &zram->table_lock_key, 0);

So I wonder if we can give distinct name, given that it's per-device
now: "zram%d->table[index].lock" and use zram->disk->first_minor (which
is a unique device id).

If you are too busy I can just send a trivial follow up patch.
Let me know what you prefer.


Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH 2/2] zram: Use a custom key for each zram object.

[Sergey Senozhatsky]

On (26/07/24 12:24), Sergey Senozhatsky wrote:
> >  static bool zram_meta_alloc(struct zram *zram, u64 disksize)
> > @@ -1999,7 +1993,8 @@ static bool zram_meta_alloc(struct zram *zram, u64 disksize)
> >  	if (!huge_class_size)
> >  		huge_class_size = zs_huge_class_size(zram->mem_pool);
> >  
> > -	slot_lock_init(zram);
> > +	lockdep_register_key(&zram->table_lock_key);
> > +	lockdep_init_map(&zram->table_lock_map, "zram->table[index].lock", &zram->table_lock_key, 0);
> 
> So I wonder if we can give distinct name, given that it's per-device
> now: "zram%d->table[index].lock" and use zram->disk->first_minor (which
> is a unique device id).

Probably (see below) is not worth it.  All those ifdef-s...  We used static
name before and didn't have problems with it.  So let's just keep doing so
(IOW let's keep not having problems ;))

// completely untested
----

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4bfe63a5225d..080012e83f86 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1956,6 +1956,31 @@ static ssize_t debug_stat_show(struct device *dev,
 	return ret;
 }
 
+static void zram_lockdep_map_release(struct zram *zram)
+{
+#ifdef CONFIG_LOCKDEP
+	lockdep_unregister_key(&zram->table_lock_key);
+	kfree(zram->table_lock_name);
+#endif
+}
+
+static int zram_lockdep_map_init(struct zram *zram)
+{
+#ifdef CONFIG_LOCKDEP
+	zram->table_lock_name = kasprintf(GFP_KERNEL,
+					  "zram%d->table[index].lock",
+					  zram->disk->first_minor);
+	if (!zram->table_lock_name)
+		return -ENOMEM;
+
+	lockdep_register_key(&zram->table_lock_key);
+	lockdep_init_map(&zram->table_lock_map, zram->table_lock_name,
+			 &zram->table_lock_key, 0);
+#endif
+	return 0;
+}
+
 static void zram_meta_free(struct zram *zram, u64 disksize)
 {
 	size_t num_pages = disksize >> PAGE_SHIFT;
@@ -1971,7 +1996,7 @@ static void zram_meta_free(struct zram *zram, u64 disksize)
 	zs_destroy_pool(zram->mem_pool);
 	vfree(zram->table);
 	zram->table = NULL;
-	lockdep_unregister_key(&zram->table_lock_key);
+	zram_lockdep_map_release(zram);
 }
 
 static bool zram_meta_alloc(struct zram *zram, u64 disksize)
@@ -1990,12 +2015,16 @@ static bool zram_meta_alloc(struct zram *zram, u64 disksize)
 		return false;
 	}
 
+	if (zram_lockdep_map_init(zram)) {
+		zs_destroy_pool(zram->mem_pool);
+		vfree(zram->table);
+		zram->table = NULL;
+		return false;
+	}
+
 	if (!huge_class_size)
 		huge_class_size = zs_huge_class_size(zram->mem_pool);
 
-	lockdep_register_key(&zram->table_lock_key);
-	lockdep_init_map(&zram->table_lock_map, "zram->table[index].lock", &zram->table_lock_key, 0);
-
 	return true;
 }
 
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 4fddc582f3b8..377832477588 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -106,8 +106,6 @@ struct zram_stats {
 
 struct zram {
 	struct zram_table_entry *table;
-	struct lockdep_map table_lock_map;
-	struct lock_class_key table_lock_key;
 	struct zs_pool *mem_pool;
 	struct zcomp *comps[ZRAM_MAX_COMPS];
 	struct zcomp_params params[ZRAM_MAX_COMPS];
@@ -143,5 +141,11 @@ struct zram {
 #ifdef CONFIG_ZRAM_MEMORY_TRACKING
 	struct dentry *debugfs_dir;
 #endif
+
+	struct lockdep_map table_lock_map;
+	struct lock_class_key table_lock_key;
+#ifdef CONFIG_LOCKDEP
+	char *table_lock_name;
+#endif
 };
 #endif

Re: [PATCH 0/2] zram: lockmap tweaks

[Sergey Senozhatsky]

On (26/07/14 16:12), Sebastian Andrzej Siewior wrote:
> Sergey, this should be a follow up on 
> 	https://lore.kernel.org/all/akza1qtgHm7zbjMW@google.com/
> 
> Could you try if this does what I claimed it does?

Thank you Sebastian!

I tested the patches.  Posted some minor comments for patch 2.

Re: [PATCH 0/2] zram: lockmap tweaks

[Sergey Senozhatsky]

On (26/07/14 16:12), Sebastian Andrzej Siewior wrote:
> Sergey, this should be a follow up on 
> 	https://lore.kernel.org/all/akza1qtgHm7zbjMW@google.com/
> 
> Could you try if this does what I claimed it does?
> 
> Sebastian Andrzej Siewior (2):
>   zram: Move lockmap to be per-zram instead per table
>   zram: Use a custom key for each zram object.

Seems that Sebastian is out-of-office (until Aug 13).

Andrew, can you please pick up these patches?  Patch 2 has a minor
commit message "issue" - missing word "swap".  As for unique name
for lockdep map comment - I will send a followup patch once this
series is in your tree.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>