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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <irpjhnu7utkhf4dds5ghklsbdug6nf32ulsp52ibvym6t3wqfg@pqu7w6uvgbvw>
Date: Thu, 27 Feb 2025 21:42:25 +0900
From: Sergey Senozhatsky <senozhatsky@...omium.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Yosry Ahmed <yosry.ahmed@...ux.dev>, 
	Hillf Danton <hdanton@...a.com>, Kairui Song <ryncsn@...il.com>, Minchan Kim <minchan@...nel.org>, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 01/17] zram: sleepable entry locking

On (25/02/27 13:05), Sebastian Andrzej Siewior 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.

I see.  I thought that they key was "shared" between zram meta table
entries because the key is per-zram device, which sort of made sense
(we can have different zram devices in a system - one swap, a bunch
mounted with various file-systems on them).

I can do a 'static key', one for all zram devices.

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

Cool!