Re: [PATCH] SLUB: use have_arch_cmpxchg()

Mathieu Desnoyers
2007-08-27T14:56:39Z

* Pekka Enberg (penberg@cs.helsinki.fi) wrote:
> Hi Mathieu,
>
> On 8/22/07, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > Cons:
> > - Does not help code readability, i.e.:
> >
> > if (have_arch_cmpxchg())
> > preempt_disable();
> > else
> > local_irq_save(flags);
>
> Heh, that's an understatement, as now slub is starting to look a bit
> like slab... ;-) We need to hide that if-else maze into helper
> functions for sure.
>

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)

slow path:
both with and without local_cmpxchg():
preempt_disable()
preempt_enable()
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.

Mathieu

> On 8/22/07, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > --- slab.orig/mm/slub.c 2007-08-22 10:28:22.000000000 -0400
> > +++ slab/mm/slub.c 2007-08-22 10:51:53.000000000 -0400
> > @@ -1450,7 +1450,8 @@ static void *__slab_alloc(struct kmem_ca
> > void **freelist = NULL;
> > unsigned long flags;
> >
> > - local_irq_save(flags);
> > + if (have_arch_cmpxchg())
> > + local_irq_save(flags);
>
> I haven't been following on the cmpxchg_local() discussion too much so
> the obvious question is: why do we do local_irq_save() for the _has_
> cmpxchg() case here...
>
> On 8/22/07, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > @@ -1553,8 +1556,12 @@ static void __always_inline *slab_alloc(
> > {
> > void **object;
> > struct kmem_cache_cpu *c;
> > + unsigned long flags;
> >
> > - preempt_disable();
> > + if (have_arch_cmpxchg())
> > + preempt_disable();
> > + else
> > + local_irq_save(flags);
>
> ...and preempt_disable() here?
>
> On 8/22/07, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > + if (have_arch_cmpxchg()) {
> > + if (unlikely(cmpxchg_local(&c->freelist, object,
> > + object[c->offset]) != object))
> > + goto redo;
> > + preempt_enable();
> > + } else {
> > + c->freelist = object[c->offset];
> > + local_irq_restore(flags);
> > + }
>
> If you move the preempt_enable/local_irq_restore pair outside of the
> if-else block, you can make a static inline function slob_get_object()
> that does:
>
> static inline bool slob_get_object(struct kmem_cache *c, void **object)
> {
> if (have_arch_cmpxchg()) {
> if (unlikely(cmpxchg_local(&c->freelist, object,
> object[c->offset]) != object))
> return false;
> } else {
> c->freelist = object[c->offset];
> }
> return true;
> }
>
> And let the compiler optimize out the branch for the non-cmpxchg case.
> Same for the reverse case too (i.e. slob_put_object).
>
> Pekka