Re: [PATCH] SLUB: use have_arch_cmpxchg()

Mathieu Desnoyers
2007-08-27T20:31:05Z

* Christoph Lameter (clameter@sgi.com) wrote:
> On Mon, 27 Aug 2007, Mathieu Desnoyers wrote:
>
> > Hrm, actually, I don't think such have_arch_cmpxchg() macro will be
> > required at all because of the small performance hit disabling
> > preemption will have on the slow and fast paths. Let's compare, for each
> > of the slow path and fast path, what locking looks like on architecture
> > with and without local cmpxchg:
> >
> > What we actually do here is:
> >
> > fast path:
> > with local_cmpxchg:
> > preempt_disable()
> > preempt_enable()
> > without local_cmpxchg:
> > preempt_disable()
> > local_irq_save
> > local_irq_restore
> > preempt_enable()
> > (we therefore disable preemption _and_ disable interrupts for
> > nothing)
>
> Hmmm..... This is a performance issue for preempt kernels.
>
> > slow path:
> > both with and without local_cmpxchg():
> > preempt_disable()
> > preempt_enable()
>
> Here we potentially loose our per cpu structure since the process may be
> rescheduled.
>

Yes, what you do here is more precisely:

preempt_disable()
local_irq_save()
preempt_enable_no_resched()
...
local_irq_restore()
preempt_check_resched()

> > local_irq_save()
> > local_irq_restore()
> >
> >
> > Therefore, we would add preempt disable/enable to the fast path of
> > architectures where local_cmpxchg is emulated with irqs off. But since
> > preempt disable/enable is just a check counter increment/decrement with
> > barrier() and thread flag check, I doubt it would hurt performances
> > enough to justify the added complexity of disabling interrupts for the
> > whole fast path in this case.
>
> One additional cacheline referenced in the hot path. Ok the counter may be
> hot as well if this is a preemptible kernel. Nevertheless a cause for
> concern.
>

If kernel is non preemptible, the macros preempt_*() are defined to
nothing. And as you say, on preemptible kernels, these variables (thread
flags and preempt count) are very likely to be in cache, since they are
in the thread info struct, but it should still be kept in mind.

Mathieu