Re: [PATCH] percpu: preemptless __per_cpu_counter_add

[Tejun Heo <tj@kernel.org>]

Hello, Christoph.

On Fri, Apr 15, 2011 at 02:43:15PM -0500, Christoph Lameter wrote:
> On Sat, 16 Apr 2011, Tejun Heo wrote:
> 
> > > +			new = 0;
> > > +		}
> > > +#ifdef CONFIG_PREEMPT
> > > +	} while (this_cpu_cmpxchg(*fbc->counters, count, new) != count);
> > > +#else
> > > +	} while (0);
> > > +	this_cpu_write(*fbc->counters, new);
> > > +#endif
> >
> > Eeeek, no.  If you want to do the above, please put it in a separate
> > inline function with sufficient comment.
> 
> That would not work well with the control flow.

It doesn't have to be that way.  ie.

	static inline bool pcnt_add_cmpxchg(counter, count, new)
	{
		/* blah blah */
	#ifdef PREEMPT
		return this_cpu_cmpxchg() == count;
	#else
		this_cpu_write();
		return true;
	#endif
	}

	void __percpu_counter_add(...)
	{
		...
		do {
			...
		} while (!pcnt_add_cmpxchg(counter, count, new))
		...
	}

It's the same thing but ifdef'd "} while()"'s are just too ugly.

> Just leave the cmpxchg for both cases? That would make the function
> irq safe as well!

Maybe, I don't know.  On x86, it shouldn't be a problem on both 32 and
64bit.  Even on archs which lack local cmpxchg, preemption flips are
cheap anyway so yeah maybe.

> > > +	if (unlikely(overflow)) {
> > >  		spin_lock(&fbc->lock);
> > > -		fbc->count += count;
> > > -		__this_cpu_write(*fbc->counters, 0);
> > > +		fbc->count += overflow;
> > >  		spin_unlock(&fbc->lock);
> >
> > Why put this outside and use yet another branch?
> 
> Because that way we do not need preempt enable/disable. The cmpxchg is
> used to update the per cpu counter in the slow case as well. All that is
> left then is to add the count to the global counter.
> 
> The branches are not an issue since they are forward branches over one
> (after converting to an atomic operation) or two instructions each. A
> possible stall is only possible in case of the cmpxchg failing.

It's slow path and IMHO it's needlessly complex.  I really don't care
whether the counter is reloaded once more or the task gets migrated to
another cpu before spin_lock() and ends up flushing local counter on a
cpu where it isn't strictly necessary.  Let's keep it simple.

Thank you.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>