| Message ID | 20240620153556.777272-2-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | zram: Replace bit spinlocks with a spinlock_t. | expand |
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: Thu, 20 Jun 2024 17:28:50 +0200 > 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> > 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++) 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: > > 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. > > > + 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: >>> 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. > >> >>> + 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 > > > On 2024-07-04 13:38:04 [+0200], Alexander Lobakin wrote: > >>> 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: > > On 2024-07-04 13:38:04 [+0200], Alexander Lobakin wrote: [..] > >>> +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: [..] > > 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.
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