linux-kernel - Re: [PATCH RFC tip/core/rcu 24/28] rcu: Introduce bulk reference count

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: <1322504279.2921.154.camel@twins>
Date:	Mon, 28 Nov 2011 19:17:59 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	paulmck@...ux.vnet.ibm.com
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, laijs@...fujitsu.com,
	dipankar@...ibm.com, akpm@...ux-foundation.org,
	mathieu.desnoyers@...ymtl.ca, josh@...htriplett.org,
	niv@...ibm.com, tglx@...utronix.de, rostedt@...dmis.org,
	Valdis.Kletnieks@...edu, dhowells@...hat.com,
	eric.dumazet@...il.com, darren@...art.com, patches@...aro.org
Subject: Re: [PATCH RFC tip/core/rcu 24/28] rcu: Introduce bulk reference
 count

On Mon, 2011-11-28 at 09:15 -0800, Paul E. McKenney wrote:

> > I'm having trouble with the naming as well as the need for an explicit
> > new API.
> > 
> > To me this looks like a regular (S)RCU variant, nothing to do with
> > references per-se (aside from the fact that SRCU is a refcounted rcu
> > variant). Also WTF is this bulk stuff about? Its still a single ref at a
> > time, not 10s or 100s or whatnot.
> 
> It is a bulk reference in comparison to a conventional atomic_inc()-style
> reference count, which is normally associated with a specific structure.
> In contrast, doing a bulkref_get() normally protects a group of structures,
> everything covered by the bulkref_t.
> 
> Yes, in theory you could have a global reference counter that protected
> a group of structures, but in practice we both know that this would not
> end well.  ;-)

Well, all the counter based RCUs are basically that. And yes, making
them scale is 'interesting', however you've done pretty well so far ;-)

I just hate the name in that it totally obscures the fact that its
regular SRCU.

> > > +static inline int bulkref_get(bulkref_t *brp)
> > > +{
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	local_irq_save(flags);
> > > +	ret =  __srcu_read_lock(brp);
> > > +	local_irq_restore(flags);
> > > +	return ret;
> > > +}
> > > +
> > > +static inline void bulkref_put(bulkref_t *brp, int idx)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	local_irq_save(flags);
> > > +	__srcu_read_unlock(brp, idx);
> > > +	local_irq_restore(flags);
> > > +}
> > 
> > This seems to be the main gist of the patch, which to me sounds utterly
> > ridiculous. Why not document that srcu_read_{un,}lock() aren't IRQ safe
> > and if you want to use it from those contexts you have to fix it up
> > yourself.
> 
> I thought I had documented this, but I guess not.  I will add that.

Oh, I hadn't checked, it could be.

> I lost you on the "fix it up yourself" -- what are you suggesting that
> someone needing to use RCU in this manner actually do?

  local_irq_save(flags);
  srcu_read_lock(&my_srcu_domain);
  local_irq_restore(flags);

and

  local_irq_save(flags);
  srcu_read_unlock(&my_srcu_domain);
  local_irq_restore(flags)

Doesn't look to be too hard, or confusing.

> > RCU lockdep doesn't do the full validation so it won't actually catch it
> > if you mess up the irq states, but I guess if you want we could look at
> > adding that.
> 
> Ah, I had missed that.  Yes, it would be very good if that could be added.
> The vast majority of the uses exit the RCU read-side critical section in
> the same context that they enter it, so it would be good to check.

/me adds to TODO list.

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