linux-kernel - Re: [PATCH 1/7] rcu: Create rcu

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: <20150715184103.GA2101@redhat.com>
Date:	Wed, 15 Jul 2015 20:41:03 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	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
Subject: Re: [PATCH 1/7] rcu: Create rcu_sync infrastructure

On 07/15, Paul E. McKenney wrote:
>
> On Sun, Jul 12, 2015 at 01:35:48AM +0200, Oleg Nesterov wrote:
> > It is functionally equivalent to
> >
> >         struct rcu_sync_struct {
> >                 atomic_t counter;
> >         };
> >
> >         static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
> >         {
>
> If you add an smp_mb() here...

I don't think so, please see below...

> >         static inline void rcu_sync_exit(struct rcu_sync_struct *rss)
> >         {
> >                 synchronize_sched();
>
> You should be able to demote the above synchronize_sched() to an
> smp_mb__before_atomic().  Even rare writes should make this tradeoff
> worthwhile.

This is irrelevant I think, this (pseudo) code just tries to explain
what this interface does.

> > +static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
> > +{
>
> 	smp_mb();  /* A: Ensure that reader sees last update. */
> 		   /* Pairs with B. */
>

Let me remind you about your f0a0e6f282c72247e7c8ec "rcu: Clarify
memory-ordering properties of grace-period primitives" documentation
patch ;)

We do not need any barrier, assuming that this is called under
preempt_disable/etc.

rcu_sync_is_idle() becomes true after another gp pass. The reader
should see all updates after that.

> > +void rcu_sync_exit(struct rcu_sync_struct *rss)
> > +{
> > +	spin_lock_irq(&rss->rss_lock);
>
> 	smp_mb(); /* B: Make sure next readers see critical section. */
> 		  /* Pairs with A. */
>
> > +	if (!--rss->gp_count) {
>
> At which point, I believe you can ditch the callback entirely, along
> with ->cb_state.
>
> So, what am I missing here?

Please see above. We start anothe gp before "unlock" to avoid mb's in
the reader's code.

> Are readers really so frequent that the
> added read-side memory barrier is visible?

But this code is heavily optimized for the readers. And please see
another discussion about sb_writers and percpu_rw_semaphore. I was
suprized, but mb() in sb_start_write() is actually noticeable.

> Given that the current
> code forces the readers to grab ->rss_lock

Where? the readers never take this lock.

Oleg.

--
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/