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

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

[Sebastian Andrzej Siewior]

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.

v3…v4: https://lore.kernel.org/linux-block/20240705125058.1564001-1-bigeasy@linutronix.de
  - Inline lock init into zram_meta_alloc().

v2…v3 https://lore.kernel.org/all/20240620153556.777272-1-bigeasy@linutronix.de/
  - Do "size_t index" within the for loop.

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

[PATCH v4 1/3] zram: Replace bit spinlocks with a spinlock_t.

[Sebastian Andrzej Siewior]

From: Mike Galbraith <umgwanakikbuti@gmail.com>

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 <umgwanakikbuti@gmail.com>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/block/zram/zram_drv.c | 18 ++++++++++++++----
 drivers/block/zram/zram_drv.h |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 1f1bf175a6c34..0f35e1f20b18e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -60,17 +60,24 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index,
 
 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)
@@ -1309,7 +1316,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;
+	size_t num_pages, index;
 
 	num_pages = disksize >> PAGE_SHIFT;
 	zram->table = vzalloc(array_size(num_pages, sizeof(*zram->table)));
@@ -1324,6 +1331,9 @@ 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++)
+		spin_lock_init(&zram->table[index].lock);
 	return true;
 }
 
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index b976824ead676..7aeff672b96f1 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

[PATCH v4 2/3] zram: Remove ZRAM_LOCK

[Sebastian Andrzej Siewior]

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.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
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 0f35e1f20b18e..812d4e7a6b7f0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -60,23 +60,16 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index,
 
 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);
 }
 
@@ -1391,7 +1384,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 7aeff672b96f1..d5eef65870380 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

[PATCH v4 3/3] zram: Shrink zram_table_entry::flags.

[Sebastian Andrzej Siewior]

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 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.

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
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 812d4e7a6b7f0..f8206ba6cbbba 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -2532,9 +2532,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 d5eef65870380..cfc8c059db636 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

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

[Jens Axboe]

On 9/6/24 8:14 AM, Sebastian Andrzej Siewior wrote:
> 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.

Looks good to me:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

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

[Sebastian Andrzej Siewior]

On 2024-09-06 08:31:23 [-0600], Jens Axboe wrote:
> On 9/6/24 8:14 AM, Sebastian Andrzej Siewior wrote:
> > 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.
> 
> Looks good to me:
> 
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
Thank you.
This is routed via your tree, right?

Sebastian

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

[Jens Axboe]

On 9/6/24 8:48 AM, Sebastian Andrzej Siewior wrote:
> On 2024-09-06 08:31:23 [-0600], Jens Axboe wrote:
>> On 9/6/24 8:14 AM, Sebastian Andrzej Siewior wrote:
>>> 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.
>>
>> Looks good to me:
>>
>> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> Thank you.
> This is routed via your tree, right?

I can certainly take it - Minchan let me know if you have concerns.

-- 
Jens Axboe

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

[Jens Axboe]

On Fri, 06 Sep 2024 16:14:42 +0200, 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.
> 
> v3…v4: https://lore.kernel.org/linux-block/20240705125058.1564001-1-bigeasy@linutronix.de
>   - Inline lock init into zram_meta_alloc().
> 
> [...]

Applied, thanks!

[1/3] zram: Replace bit spinlocks with a spinlock_t.
      commit: 9518e5bfaae19447d657983d0628062ab6712610
[2/3] zram: Remove ZRAM_LOCK
      commit: 6086aeb49e3d9e25165769b2a0a13ff67f98a1a2
[3/3] zram: Shrink zram_table_entry::flags.
      commit: 68d20eb60efbdc80662efedeb088353e9c4aa17f

Best regards,
-- 
Jens Axboe