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

[Sebastian Andrzej Siewior]

On 2025-02-25 13:51:31 [+0900], Sergey Senozhatsky wrote:
> > > +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?

ach. Got it. What about

| static void zram_slot_lock_init(struct zram *zram, u32 index)
| {
| 	static struct lock_class_key __key;
| 
| 	lockdep_init_map(slot_dep_map(zram, index),
| 			 "zram->table[index].lock",
| 			 &__key, 0);
| }

So every lock coming from zram belongs to the same class. Otherwise each
lock coming from zram_slot_lock_init() would belong to a different class
and for lockdep it would look like they are different locks. But they
are used always in the same way.

> > >  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] ;)

Sure. I was confused that you issue it twice. I didn't noticed the d in
lock_acquired(). So you have one for lockdep and one for lockstat. That
is okay ;)

Sebastian