lkml.org 
[lkml]   [2009]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [this_cpu_xx V5 19/19] SLUB: Experimental new fastpath w/o interrupt disable
* Christoph Lameter (cl@linux-foundation.org) wrote:
> On Thu, 8 Oct 2009, Mathieu Desnoyers wrote:
>
> > > Index: linux-2.6/mm/slub.c
> > > ===================================================================
> > > --- linux-2.6.orig/mm/slub.c 2009-10-08 11:35:59.000000000 -0500
> > > +++ linux-2.6/mm/slub.c 2009-10-08 14:03:22.000000000 -0500
> > > @@ -1606,7 +1606,14 @@ static void *__slab_alloc(struct kmem_ca
> > > unsigned long addr)
> > > {
> > > void **object;
> > > - struct page *page = __this_cpu_read(s->cpu_slab->page);
> > > + struct page *page;
> > > + unsigned long flags;
> > > + int hotpath;
> > > +
> > > + local_irq_save(flags);
> >
> > (Recommend adding)
> >
> > preempt_enable_no_resched();
> >
> >
> > The preempt enable right in the middle of a big function is adding an
> > unnecessary barrier(), which will restrain gcc from doing its
> > optimizations. This might hurt performances.
>
> In the middle of the function we have determine that we have to go to the
> page allocator to get more memory. There is not much the compiler can do
> to speed that up.

Indeed, the compiler cannot do much about it. However, the programer
(you) can move the preempt_enable_no_resched() part of the
preempt_enable() to the beginning of the function.

>
> > I still recommend the preempt_enable_no_resched() at the beginning of
> > __slab_alloc(), and simply putting a check_resched() here (which saves
> > us the odd compiler barrier in the middle of function).
>
> Then preemption would be unnecessarily disabled for the page allocator
> call?

No ?
preempt_enable_no_resched() enables preemption.

>
> > > if (gfpflags & __GFP_WAIT)
> > > local_irq_enable();
> > >
> > > + preempt_enable();
> >
> > We could replace the above by:
> >
> > if (gfpflags & __GFP_WAIT) {
> > local_irq_enable();
> > preempt_check_resched();
> > }
>
> Which would leave preempt off for the page allocator.

Not if you do preempt_enable_no_resched() at the beginnig of the
function, after disabling interrupts.

>
> > > + irqsafe_cpu_inc(s->cpu_slab->active);
> > > + barrier();
> > > object = __this_cpu_read(s->cpu_slab->freelist);
> > > - if (unlikely(!object || !node_match(s, node)))
> > > + if (unlikely(!object || !node_match(s, node) ||
> > > + __this_cpu_read(s->cpu_slab->active)))
> >
> > Missing a barrier() here ?
>
> The modifications of the s->cpu_slab->freelist in __slab_alloc() are only
> done after interrupts have been disabled and after the slab has been locked.

I was concerned about a potential race between
cpu_slab->active/cpu_slab->freelist if an interrupt came in. I
understand that as soon as you get a hint that you must hit the slow
path, you don't care about the order in which these operations have been
done.

>
> > The idea is to let gcc know that "active" inc/dec and "freelist" reads
> > must never be reordered. Even when the decrement is done in the slow
> > path branch.
>
> Right. How could that occur with this code?
>

__slab_alloc calls __this_cpu_dec(s->cpu_slab->active); without any
compiler barrier. But I get that when __slab_alloc is executed, we don't
care about "active" dec to be reordered, because we're not altering fast
path data anymore.

> > > + preempt_enable();
> > > stat(s, FREE_FASTPATH);
> > > - } else
> > > + } else {
> >
> > Perhaps missing a barrier() in the else ?
>
> Not sure why that would be necessary. __slab_free() does not even touch
> s->cpu_slab->freelist if you have the same reasons as in the alloc path.

My intent was to order __this_cpu_read(s->cpu_slab->page) and
irqsafe_cpu_dec(s->cpu_slab->active), but I get that if you run the slow
path, you don't care about some spilling of the slow path over the slab
active critical section.

Thanks,

Mathieu

>
>

--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


\
 
 \ /
  Last update: 2009-10-12 16:07    [W:0.141 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site