lkml.org 
[lkml]   [2007]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] SLUB: use have_arch_cmpxchg()
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-08-22 18:27    [W:0.094 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site