[PATCH v2 0/3] zram: Replace bit spinlocks with a spinlock_t.

Hi,

this is follow up to the previous posting, making the lock
unconditionally. The original problem with bit spinlock is that it
disabled preemption and the following operations (within the atomic
section) perform operations that may sleep on PREEMPT_RT. Mike expressed
that he would like to keep using zram on PREEMPT_RT.

v1…v2: https://lore.kernel.org/all/20240619150814.BRAvaziM@linutronix.de/:
  - Add the spinlock_t unconditionally
  - Remove ZRAM_LOCK since it has no user after the lock has been added.
  - Make zram_table_entry::flags an integer so struct zram_table_entry
    does not gain additional weight.

Sebastian

From: Mike Galbraith <redacted>

The bit spinlock disables preemption. The spinlock_t lock becomes a sleeping
lock on PREEMPT_RT and it can not be acquired in this context. In this locked
section, zs_free() acquires a zs_pool::lock, and there is access to
zram::wb_limit_lock.

Add a spinlock_t for locking. Keep the set/ clear ZRAM_LOCK bit after
the lock has been acquired/ dropped. The size of struct zram_table_entry
increases by 4 bytes due to lock and additional 4 bytes padding with
CONFIG_ZRAM_TRACK_ENTRY_ACTIME enabled.

Signed-off-by: Mike Galbraith <redacted>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/block/zram/zram_drv.c | 22 +++++++++++++++++++---
 drivers/block/zram/zram_drv.h |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 3acd7006ad2cc..036845cd4f25e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -57,19 +57,34 @@ static void zram_free_page(struct zram *zram, size_t index);
 static int zram_read_page(struct zram *zram, struct page *page, u32 index,
 			  struct bio *parent);
 
+static void zram_meta_init_table_locks(struct zram *zram, size_t num_pages)
+{
+	size_t index;
+
+	for (index = 0; index < num_pages; index++)
+		spin_lock_init(&zram->table[index].lock);
+}
+
 static int zram_slot_trylock(struct zram *zram, u32 index)
 {
-	return bit_spin_trylock(ZRAM_LOCK, &zram->table[index].flags);
+	int ret;
+
+	ret = spin_trylock(&zram->table[index].lock);
+	if (ret)
+		__set_bit(ZRAM_LOCK, &zram->table[index].flags);
+	return ret;
 }
 
 static void zram_slot_lock(struct zram *zram, u32 index)
 {
-	bit_spin_lock(ZRAM_LOCK, &zram->table[index].flags);
+	spin_lock(&zram->table[index].lock);
+	__set_bit(ZRAM_LOCK, &zram->table[index].flags);
 }
 
 static void zram_slot_unlock(struct zram *zram, u32 index)
 {
-	bit_spin_unlock(ZRAM_LOCK, &zram->table[index].flags);
+	__clear_bit(ZRAM_LOCK, &zram->table[index].flags);
+	spin_unlock(&zram->table[index].lock);
 }
 
 static inline bool init_done(struct zram *zram)

@@ -1226,6 +1241,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);
+	zram_meta_init_table_locks(zram, num_pages);
 	return true;
 }
 

diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 35e3221446292..dcc3c106ce713 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h

@@ -69,6 +69,7 @@ struct zram_table_entry {
 		unsigned long element;
 	};
 	unsigned long flags;
+	spinlock_t lock;
 #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
 	ktime_t ac_time;
 #endif

-- 
2.45.2

The ZRAM_LOCK was used for locking and after the addition of spinlock_t
the bit set and cleared but there no reader of it.

Remove the ZRAM_LOCK bit.

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

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 036845cd4f25e..659966e00c300 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -67,23 +67,16 @@ static void zram_meta_init_table_locks(struct zram *zram, size_t num_pages)
 
 static int zram_slot_trylock(struct zram *zram, u32 index)
 {
-	int ret;
-
-	ret = spin_trylock(&zram->table[index].lock);
-	if (ret)
-		__set_bit(ZRAM_LOCK, &zram->table[index].flags);
-	return ret;
+	return spin_trylock(&zram->table[index].lock);
 }
 
 static void zram_slot_lock(struct zram *zram, u32 index)
 {
 	spin_lock(&zram->table[index].lock);
-	__set_bit(ZRAM_LOCK, &zram->table[index].flags);
 }
 
 static void zram_slot_unlock(struct zram *zram, u32 index)
 {
-	__clear_bit(ZRAM_LOCK, &zram->table[index].flags);
 	spin_unlock(&zram->table[index].lock);
 }
 

@@ -1299,7 +1292,7 @@ static void zram_free_page(struct zram *zram, size_t index)
 	zram_set_handle(zram, index, 0);
 	zram_set_obj_size(zram, index, 0);
 	WARN_ON_ONCE(zram->table[index].flags &
-		~(1UL << ZRAM_LOCK | 1UL << ZRAM_UNDER_WB));
+		~(1UL << ZRAM_UNDER_WB));
 }
 
 /*

diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index dcc3c106ce713..262fa960a0783 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h

@@ -45,9 +45,7 @@
 
 /* Flags for zram pages (table[page_no].flags) */
 enum zram_pageflags {
-	/* zram slot is locked */
-	ZRAM_LOCK = ZRAM_FLAG_SHIFT,
-	ZRAM_SAME,	/* Page consists the same element */
+	ZRAM_SAME = ZRAM_FLAG_SHIFT,	/* Page consists the same element */
 	ZRAM_WB,	/* page is stored on backing_device */
 	ZRAM_UNDER_WB,	/* page is under writeback */
 	ZRAM_HUGE,	/* Incompressible page */

-- 
2.45.2

The zram_table_entry::flags member is of type long and uses 8 bytes on a
64bit architecture. With a PAGE_SIZE of 256KiB we have PAGE_SHIFT of 18
which in turn leads to __NR_ZRAM_PAGEFLAGS = 27. This still fits in an
ordinary integer.
By reducing it the size of `flags' to four bytes, the size of the struct
goes back to 16 bytes. The padding between the lock and ac_time (if
enabled) is also gone.

Make zram_table_entry::flags an unsigned int and update the build test
to reflect the change.

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

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 659966e00c300..a35d4bd2e60ef 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -2412,9 +2412,10 @@ static void destroy_devices(void)
 
 static int __init zram_init(void)
 {
+	struct zram_table_entry zram_te;
 	int ret;
 
-	BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > BITS_PER_LONG);
+	BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > sizeof(zram_te.flags) * 8);
 
 	ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
 				      zcomp_cpu_up_prepare, zcomp_cpu_dead);

diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 262fa960a0783..531cefc666682 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h

@@ -66,7 +66,7 @@ struct zram_table_entry {
 		unsigned long handle;
 		unsigned long element;
 	};
-	unsigned long flags;
+	unsigned int flags;
 	spinlock_t lock;
 #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
 	ktime_t ac_time;

-- 
2.45.2

On (24/06/20 17:28), Sebastian Andrzej Siewior wrote:

this is follow up to the previous posting, making the lock
unconditionally. The original problem with bit spinlock is that it
disabled preemption and the following operations (within the atomic
section) perform operations that may sleep on PREEMPT_RT. Mike expressed
that he would like to keep using zram on PREEMPT_RT.

Sorry for the delay.

I guess this works for me, FWIW:
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Minchan, any objections?

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Thu, 20 Jun 2024 17:28:50 +0200

Maybe declare @index right here?

+		spin_lock_init(&zram->table[index].lock);
+}

[...]

Thanks,
Olek

On 2024-07-04 13:38:04 [+0200], Alexander Lobakin wrote:

quoted

index 3acd7006ad2cc..036845cd4f25e 100644

--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -57,19 +57,34 @@ static void zram_free_page(struct zram *zram, size_t index);
 static int zram_read_page(struct zram *zram, struct page *page, u32 index,
 			  struct bio *parent);
 
+static void zram_meta_init_table_locks(struct zram *zram, size_t num_pages)
+{
+	size_t index;
+
+	for (index = 0; index < num_pages; index++)

Maybe declare @index right here?

But why? Declarations at the top followed by code. 

quoted

+		spin_lock_init(&zram->table[index].lock);
+}

[...]

Thanks,
Olek

Sebastian

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Thu, 4 Jul 2024 14:19:08 +0200

On 2024-07-04 13:38:04 [+0200], Alexander Lobakin wrote:

quoted

quoted

index 3acd7006ad2cc..036845cd4f25e 100644

--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -57,19 +57,34 @@ static void zram_free_page(struct zram *zram, size_t index);
 static int zram_read_page(struct zram *zram, struct page *page, u32 index,
 			  struct bio *parent);
 
+static void zram_meta_init_table_locks(struct zram *zram, size_t num_pages)
+{
+	size_t index;
+
+	for (index = 0; index < num_pages; index++)

Maybe declare @index right here?

But why? Declarations at the top followed by code. 

I meant

	for (size_t index = 0; index < num_pages; index++)

It's allowed and even recommended for a couple years already.

quoted

quoted

+		spin_lock_init(&zram->table[index].lock);
+}

[...]

Thanks,
Olek

Sebastian

Thanks,
Olek

On 2024-07-05 14:02:22 [+0200], Alexander Lobakin wrote:

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Thu, 4 Jul 2024 14:19:08 +0200

quoted

On 2024-07-04 13:38:04 [+0200], Alexander Lobakin wrote:

quoted

quoted

index 3acd7006ad2cc..036845cd4f25e 100644

--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -57,19 +57,34 @@ static void zram_free_page(struct zram *zram, size_t index);
 static int zram_read_page(struct zram *zram, struct page *page, u32 index,
 			  struct bio *parent);
 
+static void zram_meta_init_table_locks(struct zram *zram, size_t num_pages)
+{
+	size_t index;
+
+	for (index = 0; index < num_pages; index++)

Maybe declare @index right here?

But why? Declarations at the top followed by code. 

I meant

	for (size_t index = 0; index < num_pages; index++)

It's allowed and even recommended for a couple years already.

I can't believe this…

Thanks,
Olek

Sebastian

On (24/07/05 14:02), Alexander Lobakin wrote:

quoted

On 2024-07-04 13:38:04 [+0200], Alexander Lobakin wrote:

[..]

quoted

quoted

quoted

+static void zram_meta_init_table_locks(struct zram *zram, size_t num_pages)
+{
+	size_t index;
+
+	for (index = 0; index < num_pages; index++)

Maybe declare @index right here?

But why? Declarations at the top followed by code. 

I meant

	for (size_t index = 0; index < num_pages; index++)

It's allowed and even recommended for a couple years already.

I wonder since when?  Do gcc 5.1 and clang 13.0.1 support this?

On (24/07/08 12:03), Sergey Senozhatsky wrote:
[..]

quoted

I meant

	for (size_t index = 0; index < num_pages; index++)

It's allowed and even recommended for a couple years already.

I wonder since when?  Do gcc 5.1 and clang 13.0.1 support this?

Since C99.  gcc 5.1/clang 13.0.1 are fine with that.  TIL.