linux-kernel - Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw

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: <20150714104810.GB24369@quack.suse.cz>
Date:	Tue, 14 Jul 2015 12:48:10 +0200
From:	Jan Kara <jack@...e.cz>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Al Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Daniel Wagner <daniel.wagner@...-carit.de>,
	Davidlohr Bueso <dave@...olabs.net>,
	Ingo Molnar <mingo@...hat.com>, Tejun Heo <tj@...nel.org>,
	linux-kernel@...r.kernel.org,
	Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore

  Hello,

On Mon 13-07-15 23:25:36, Oleg Nesterov wrote:
> Al, Jan, could you comment? I mean the intent, the patches are
> obviously not for inclusion yet.

Thanks for the patches! Hum, what do people have with freeze protection
these days? Noone cared about it for years and sudddently two patch sets
within a month :) Anyway, have you seen the patch set from Dave Hansen?

It is here: https://lkml.org/lkml/2015/6/24/682
He modifies the freezing primitives in a different way. AFAICS the
resulting performance of the fast path should be about the same. So unless
I'm missing something and there is a significant performance advantage to
Dave's patches I'm all for using a generic primitive you suggest.

Can you perhaps work with Dave on some common resolution?

> We can remove everything from struct sb_writers except frozen
> (which can become a boolean, it seems) and add the array of

I think 'int' for 'frozen' needs to stay - we need to distinguish at least
three states - unfrozen, in the process of being frozen, frozen.

> percpu_rw_semaphore's instead.
> 
> __sb_start/end_write() can use percpu_down/up_read(), and
> freeze/thaw_super() can use percpu_down/up_write().
> 
> Why:
> 
> 	- Firstly, __sb_start_write() looks simply buggy. I does
> 	  __sb_end_write() if it sees ->frozen, but if it migrates
> 	  to another CPU before percpu_counter_dec() sb_wait_write()
> 	  can wrongly succeed if there is another task which holds
> 	  the same "semaphore": sb_wait_write() can miss the result
> 	  of the previous percpu_counter_inc() but see the result
> 	  of this percpu_counter_dec().

Agreed, that's a bug.
 
> 	- This code doesn't look simple. It would be better to rely
> 	  on the generic locking code.

Definitely agreed, I just didn't find anything usable (fast enough) at
the time...

> 	- __sb_start_write() will be a little bit faster, but this
> 	  is minor.

Actually Dave could measure the gain achieved by removing the barrier. It
would be good to verify that your patches achieve a similar gain.

> Todo:
> 
> 	- __sb_start_write(wait => false) always fail.
> 
> 	  Thivial, we already have percpu_down_read_trylock() just
> 	  this patch wasn't merged yet.
> 
> 	- sb_lockdep_release() and sb_lockdep_acquire() play with
> 	  percpu_rw_semaphore's internals.
> 
> 	  Trivial, we need a couple of new helper in percpu-rwsem.c.
> 
> 	- Fix get_super_thawed(), it will spin if MS_RDONLY...
> 
> 	  It is not clear to me what exactly should we do, but this
> 	  doesn't look hard. Perhaps it can just return if MS_RDONLY.

What's the exact problem here?

> 	- Most probably I missed something else, and I do not need
> 	  how to test.
> 
> Finally. freeze_super() calls synchronize_sched_expedited() 3 times in
> a row. This is bad and just stupid. But if we change percpu_rw_semaphore
> to use rcu_sync (see https://lkml.org/lkml/2015/7/11/211) we can avoid
> this and do synchronize_sched() only once. Just we need some more simple
> changes in percpu-rwsem.c, so that all sb_writers->rw_sem[] semaphores
> could use the single sb_writers->rss.
> 
> In this case destroy_super() needs some modifications too,
> percpu_free_rwsem() will be might_sleep(). But this looks simple too.

Otherwise the general conversion idea looks good to me.

									Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/