Re: [PATCH v8 01/17] zram: sleepable entry locking

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (25/02/24 09:19), Sebastian Andrzej Siewior wrote:
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 9f5020b077c5..37c5651305c2 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -58,19 +58,62 @@ static void zram_free_page(struct zram *zram, size_t index);
> >  static int zram_read_from_zspool(struct zram *zram, struct page *page,
> >  				 u32 index);
> >  
> > -static int zram_slot_trylock(struct zram *zram, u32 index)
> > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > +#define slot_dep_map(zram, index) (&(zram)->table[(index)].dep_map)
> > +#define zram_lock_class(zram) (&(zram)->lock_class)
> > +#else
> > +#define slot_dep_map(zram, index) NULL
> > +#define zram_lock_class(zram) NULL
> > +#endif
> 
> That CONFIG_DEBUG_LOCK_ALLOC here is not needed because dep_map as well
> as lock_class goes away in !CONFIG_DEBUG_LOCK_ALLOC case.

Let me give it a try.

> > +static void zram_slot_lock_init(struct zram *zram, u32 index)
> >  {
> > -	return spin_trylock(&zram->table[index].lock);
> > +	lockdep_init_map(slot_dep_map(zram, index),
> > +			 "zram->table[index].lock",
> > +			 zram_lock_class(zram), 0);
> > +}
> Why do need zram_lock_class and slot_dep_map? As far as I can tell, you
> init both in the same place and you acquire both in the same place.
> Therefore it looks like you tell lockdep that you acquire two locks
> while it would be enough to do it with one.

Sorry, I'm not that familiar with lockdep, can you elaborate?
I don't think we can pass NULL as lock-class to lockdep_init_map(),
this should trigger `if (DEBUG_LOCKS_WARN_ON(!key))` as far as I
can tell.  I guess it's something else that you are suggesting?

> >  static void zram_slot_lock(struct zram *zram, u32 index)
> >  {
> > -	spin_lock(&zram->table[index].lock);
> > +	unsigned long *lock = &zram->table[index].flags;
> > +
> > +	mutex_acquire(slot_dep_map(zram, index), 0, 0, _RET_IP_);
> > +	wait_on_bit_lock(lock, ZRAM_ENTRY_LOCK, TASK_UNINTERRUPTIBLE);
> > +	lock_acquired(slot_dep_map(zram, index), _RET_IP_);
> 
> This looks odd. The first mutex_acquire() can be invoked twice by two
> threads, right? The first thread gets both (mutex_acquire() and
> lock_acquired()) while, the second gets mutex_acquire() and blocks on
> wait_on_bit_lock()).

Hmm why is this a problem?  ... and I'm pretty sure it was you who
suggested to put mutex_acquire() before wait_on_bit_lock() [1] ;)

[1] https://lore.kernel.org/all/20250206073803.c2tiyIq6@linutronix.de/