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@xxxxxxxxxx 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@xxxxxxxxx";> email@xxxxxxxxx </a>