Re: [PATCH] SLUB: use have_arch_cmpxchg()

["Pekka Enberg" <penberg@cs.helsinki.fi>]

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.

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