Re: [PATCH] generic dynamic per cpu refcounting

[Kent Overstreet <koverstreet@google.com>]

On Fri, Jan 25, 2013 at 07:09:41PM +0100, Oleg Nesterov wrote:
> (add lkml)
> 
> On 01/24, Kent Overstreet wrote:
> >
> > This has already been on lkml and is in Andrew's tree, Tejun just asked
> > me to send it out again:
> 
> I'll try to read this code later, just a couple of questions after a quick
> glance. Sorry this was already discussed...

No worries, it wasn't that widely circulated.

> > +struct percpu_ref {
> > +	atomic64_t		count;
> > +	unsigned long		pcpu_count;
> > +};
> 
> The code looks a bit tricky mostly because you pack state/pointer/jiffies
> into ->pcpu_count. The same for ->count.

Yes, it is.

> I assume that you have a good reason to shrink the sizeof(percpu_ref), but
> I am curious: who is the user of this thing?

Right now - just the aio code, but the idea was to make it as close to a
drop in replacement for atomic_t + atomic_get()/atomic_dec_and_test() as
possible.

> > + * percpu_ref_get - increment a dynamic percpu refcount
> > + *
> > + * Increments @ref and possibly converts it to percpu counters. Must be called
> > + * with rcu_read_lock() held, and may potentially drop/reacquire rcu_read_lock()
> > + * to allocate percpu counters - if sleeping/allocation isn't safe for some
> > + * other reason (e.g. a spinlock), see percpu_ref_get_noalloc().
> 
> And this looks strange. It must be called under rcu_read_lock(), but
> ->rcu_read_lock_nesting must be == 1. Otherwise rcu_read_unlock() in
> percpu_ref_alloc() won't work.
> 
> Again, I think you have a reason, but could you explain? IOW, why we
> can't make it might_sleep() instead? The fast path can do rcu_read_lock()
> itself.

It's stupid and contorted because I didn't have any better ideas when I
first wrote it and haven't fixed it yet.

> > +static inline void percpu_ref_get_noalloc(struct percpu_ref *ref)
> > +{
> > +	__percpu_ref_get(ref, false);
> > +}
> 
> and this could be percpu_ref_get_atomic().
> 
> Once again, I am not arguing, just can't understand.

Same deal, I'm going to get rid of the two different versions.

> > +void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
> > +{
> > +	unsigned long pcpu_count;
> > +	uint64_t v;
> > +
> > +	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
> > +
> > +	if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
> > +		/* for rcu - we're not using rcu_dereference() */
> > +		smp_read_barrier_depends();
> > +		__this_cpu_inc(*((unsigned __percpu *) pcpu_count));
> 
> The comment looks confusing a bit... smp_read_barrier_depends() is not
> for rcu, we obviously need it to access (unsigned __percpu *) pcpu_count.
> But yes, since we didn't use rcu_dereference() we have to add it by hand.

Yeah - originally I was using rcu_dereference(), but sparse hated
combining __percpu and __rcu and I couldn't get it to stop complaining.

> 
> > +int percpu_ref_kill(struct percpu_ref *ref)
> > +{
> > ...
> > +	if (status == PCPU_REF_PTR) {
> > +		unsigned count = 0, cpu;
> > +
> > +		synchronize_rcu();
> > +
> > +		for_each_possible_cpu(cpu)
> > +			count += *per_cpu_ptr((unsigned __percpu *) pcpu_count, cpu);
> > +
> > +		pr_debug("global %lli pcpu %i",
> > +			 atomic64_read(&ref->count) & PCPU_COUNT_MASK,
> > +			 (int) count);
> > +
> > +		atomic64_add((int) count, &ref->count);
> > +		smp_wmb();
> > +		/* Between setting global count and setting PCPU_REF_DEAD */
> > +		ref->pcpu_count = PCPU_REF_DEAD;
> 
> The coment explains what the code does, but not why ;)

That seems like a more straightforward barrier than most... we need the
refcount to be consistent before setting the state to dead :P

> I guess this is for percpu_ref_put(), and this wmb() pairs with implicit
> mb() implied by atomic64_dec_return().

Yeah. I expanded the comment there a bit...

> 
> > +		free_percpu((unsigned __percpu *) pcpu_count);
> 
> I guess it could be freed right after for_each_possible_cpu() above, but
> this doesn't matter.

I think that'd be better though, I'll switch it.