Messages in this thread | | | Subject | Re: [PATCH RFC tip/core/rcu 24/28] rcu: Introduce bulk reference count | From | Peter Zijlstra <> | Date | Mon, 28 Nov 2011 19:17:59 +0100 |
| |
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.
| |