Messages in this thread | | | Date | Wed, 12 Jun 2013 14:17:47 -0700 | From | Tejun Heo <> | Subject | Re: [PATCH 2/2] percpu-refcount: implement percpu_tryget() along with percpu_ref_kill_and_confirm() |
| |
Hey, Kent.
On Wed, Jun 12, 2013 at 02:08:24PM -0700, Kent Overstreet wrote: > On Wed, Jun 12, 2013 at 01:46:27PM -0700, Tejun Heo wrote: > > From de3c0749e2c1960afcc433fc5da136b85c8bd896 Mon Sep 17 00:00:00 2001 > > From: Tejun Heo <tj@kernel.org> > > Date: Wed, 12 Jun 2013 13:37:42 -0700 > > > > Implement percpu_tryget() which succeeds iff the refcount hasn't been > > killed yet. Because the refcnt is per-cpu, different CPUs may have > > different perceptions on when the counter has been killed and tryget() > > may continue to succeed for a while after percpu_ref_kill() returns. > > I don't feel very comfortable with saying percpu_ref_tryget() succeeds > "iff the refcount hasn't been killed yet". That's something I would say
Yeah, the phrasing of the first sentence could be a bit misleading. It probably should emphasize that there's no synchronization by default from the beginning.
> about e.g. atomic_inc_not_zero(), but percpu_ref_tryget() doesn't do > that sort of synchronization which is what iff implies to me. > > If the user does need some kind of strict ordering between > percpu_ref_kill() and percpu_ref_tryget(), they'd have to insert some > memory barriers - tryget() certainly doesn't have any.
which is why percpu_ref_kill_and_confirm() has been added.
> > While this isn't the prettiest interface, it doesn't force synchronous > > wait and is much safer than requiring the caller to do its own > > call_rcu(). > > Yeah, this seems... icky to me. I'm going to withhold judgement until I > see how it's used, maybe there isn't any other way but I'd like to try > and find something prettier.
Yeap, this is icky. If you have any better ideas, I'm all ears.
> > -void percpu_ref_kill(struct percpu_ref *ref) > > +void percpu_ref_kill_and_confirm(struct percpu_ref *ref, > > + percpu_ref_func_t *confirm_kill) > > Passing release to percpu_ref_init() and confirm_kill to > percpu_ref_kill() is inconsistent. Can we pass them both to > percpu_ref_init()?
I don't know. Maybe. While they're stored in the same place, @confirm_kill is really an optional part of killing itself, so specifying it to kill *seems* like the better place and it also marks it clearly that something funky is going on during while killing the reference count.
> Also, given that confirm_kill is an optional thing I don't see why > you're renaming percpu_ref_kill() -> percpu_ref_kill_and_confirm(). Most > users (certainly aio, I think the module code too) don't have any use > for confirm kill, I don't want to rename it for an ugly optional thing.
Hmm? percpu_ref_kill() is still there. It now just calls the ugly thing with %NULL @confirm_kill.
Thanks.
-- tejun
| |