Re: [PATCH net-next] net: allocate skbs on local node

Re: [PATCH net-next] net: allocate skbs on local node

[Pekka Enberg]

On 10/12/10 10:58 AM, Andrew Morton wrote:
> On Tue, 12 Oct 2010 09:49:53 +0200 Eric Dumazet<eric.dumazet@gmail.com>  wrote:
>
>> Le mardi 12 octobre 2010 à 00:24 -0700, Andrew Morton a écrit :
>>
>>> I'd love to forget it, but it's faster for some things (I forget
>>> which).  Which is why it's still around.
>>
>> Yes, two years ago it was true on pathological/obscure cases.
>> Every time I did the comparison, SLUB won.
>> You asked me, I did yet another test this morning, and 40% is pretty
>> serious, I believe.
>>
>>> And the ghastly thing about this is that you're forced to care about it
>>> too because some people are, apparently, still using it.
>>
>> Yes, some people (in my company) still use linux 2.6.9 32bit on HP G6/G7
>> machines, I know...
>>
>> I am not saying we should not care, but for any serious network workload
>> on NUMA arches, SLUB is the best, and seeing Christoph recent work, it
>> might even get better.
>>
>> BTW, I believe all modern distros ship SLUB, dont they ?
>
> Dunno.
>
> Pekka, why haven't we deleted slab yet??

To make a long story short, we still have relevant performance 
regressions that need to be taken care of. The most interesting one is a 
regression in netperf TCP_RR that's been reported by David Rientjes a 
while back. There's bunch of SLUB cleanups queued for 2.6.37 that pave 
the way for Christoph's SLUB queueing work that should hopefully fix 
that particular issue for 2.6.38.

There's little point in discussing the removal of SLAB as long as there 
are performance regressions for real workloads from people who are 
willing to share results and test patches. I'm optimistic that we'll be 
able to try removing SLAB some time next year unless something 
interesting pops up...

			Pekka

Re: [PATCH net-next] net: allocate skbs on local node

[Christoph Lameter]

On Tue, 12 Oct 2010, Pekka Enberg wrote:

> There's little point in discussing the removal of SLAB as long as there are
> performance regressions for real workloads from people who are willing to
> share results and test patches. I'm optimistic that we'll be able to try
> removing SLAB some time next year unless something interesting pops up...

Hmmm. Given these effects I think we should be more cautious regarding the
unification work. May be the "unified allocator" should replace SLAB
instead and SLUB can stay unchanged? The unification patches go back to
the one lock per node SLAB thing because the queue maintenance overhead is
otherwise causing large regressions in hackbench because of lots of atomic
ops. The per node lock seem to be causing problems here in the network
stack,. Take the unified as a SLAB cleanup instead? Then at least we have
a large common code base and just differentiate through the locking
mechanism?

Re: [PATCH net-next] net: allocate skbs on local node

[David Rientjes]

On Tue, 12 Oct 2010, Christoph Lameter wrote:

> Hmmm. Given these effects I think we should be more cautious regarding the
> unification work. May be the "unified allocator" should replace SLAB
> instead and SLUB can stay unchanged?

Linus has said that he refuses to merge another allocator until one is 
removed or replaced, so that would force the unificiation patches to go 
into slab instead if you want to leave slub untouched.

> The unification patches go back to
> the one lock per node SLAB thing because the queue maintenance overhead is
> otherwise causing large regressions in hackbench because of lots of atomic
> ops. The per node lock seem to be causing problems here in the network
> stack,.

The TCP_RR regression on slub is because of what I described a couple 
years ago as "slab thrashing" where cpu slabs would be filled with 
allocations, then frees would occur to move those slabs from the full to 
partial list with only a few free objects, those partial slabs would 
quickly become full, etc.  Performance gets better if you change the 
per-node lock to a trylock when iterating the partial list and preallocate 
and have a substantially longer partial list than normal (and it still 
didn't rival slab's performance), so I don't think it's only a per-node 
lock that's the issue , it's all the slowpath overhead of swapping the cpu 
slab out for another slab.  The TCP_RR load would show slub stats that 
indicate certain caches, kmalloc-256 and kmalloc-2048, would have ~98% of 
allocations coming from the slowpath.

This gets better if you allocate higher order slabs (and kmalloc-2048 is 
already order-3 by default) but then allocating new slabs gets really slow 
if not impossible on smaller machines.  The overhead of even compaction 
will kill us.

> Take the unified as a SLAB cleanup instead? Then at least we have
> a large common code base and just differentiate through the locking
> mechanism?
> 

Will you be adding the extensive slub debugging to slab then?  It would be 
a shame to lose it because one allocator is chosen over another for 
performance reasons and then we need to recompile to debug issues as they 
arise.

Re: [PATCH net-next] net: allocate skbs on local node

[Pekka Enberg]

On Tue, 12 Oct 2010, Christoph Lameter wrote:
>> Hmmm. Given these effects I think we should be more cautious regarding the
>> unification work. May be the "unified allocator" should replace SLAB
>> instead and SLUB can stay unchanged?

On 10/12/10 10:43 PM, David Rientjes wrote:
> Linus has said that he refuses to merge another allocator until one is
> removed or replaced, so that would force the unificiation patches to go
> into slab instead if you want to leave slub untouched.

Yes, and quite frankly, I'm not interested in introducing a new one either.

On Tue, 12 Oct 2010, Christoph Lameter wrote:
>> The unification patches go back to
>> the one lock per node SLAB thing because the queue maintenance overhead is
>> otherwise causing large regressions in hackbench because of lots of atomic
>> ops. The per node lock seem to be causing problems here in the network
>> stack,.

On Tue, 12 Oct 2010, Christoph Lameter wrote:
>> Take the unified as a SLAB cleanup instead? Then at least we have
>> a large common code base and just differentiate through the locking
>> mechanism?

On 10/12/10 10:43 PM, David Rientjes wrote:
> Will you be adding the extensive slub debugging to slab then?  It would be
> a shame to lose it because one allocator is chosen over another for
> performance reasons and then we need to recompile to debug issues as they
> arise.

I think Christoph is saying that we'd remove SLAB and make the unified 
allocator the new SLAB while keeping SLUB in place. In any case, yes, 
the debugging support in SLUB is something that we want to keep.

			Pekka

Re: [PATCH net-next] net: allocate skbs on local node

[David Rientjes]

On Wed, 13 Oct 2010, Pekka Enberg wrote:

> I think Christoph is saying that we'd remove SLAB and make the unified
> allocator the new SLAB while keeping SLUB in place.

Right, so his unification patches would be against slab instead of slub 
which is a pretty major change from the current state of the patchset, 
although it might actually be smaller?

Re: [PATCH net-next] net: allocate skbs on local node

[Pekka Enberg]

  On Wed, 13 Oct 2010, Pekka Enberg wrote:
>> I think Christoph is saying that we'd remove SLAB and make the unified
>> allocator the new SLAB while keeping SLUB in place.

On 10/13/10 9:31 AM, David Rientjes wrote:
>> Right, so his unification patches would be against slab instead of slub
>> which is a pretty major change from the current state of the patchset,
>> although it might actually be smaller?

No, the way I understood it was:

   rm mm/slab.c
   cp mm/slub.c mm/slab.c
<apply patches on top of slab.c>

             Pekka

Re: [PATCH net-next] net: allocate skbs on local node

[Christoph Lameter]

On Tue, 12 Oct 2010, David Rientjes wrote:

> > Take the unified as a SLAB cleanup instead? Then at least we have
> > a large common code base and just differentiate through the locking
> > mechanism?
> >
>
> Will you be adding the extensive slub debugging to slab then?  It would be
> a shame to lose it because one allocator is chosen over another for
> performance reasons and then we need to recompile to debug issues as they
> arise.

Well basically we would copy SLUB to SLAB apply unification patches to
SLAB instead of SLUBB. We first have to make sure that the unified patches
have the same performance as SLAB.

It maybe much better to isolate the debug features and general bootstrap
from the particulars of the allocation strategy of either SLUB or SLAB.
That way a common code base exists and it would be easier to add different
allocation strategies.

Basically have slab.c with the basic functions and then slab_queueing.c
and slab_noqueue.c for SLAB/SLUB with the particulars of the allocation
strategy?

Re: [PATCH net-next] net: allocate skbs on local node

[David Rientjes]

On Wed, 13 Oct 2010, Christoph Lameter wrote:

> > Will you be adding the extensive slub debugging to slab then?  It would be
> > a shame to lose it because one allocator is chosen over another for
> > performance reasons and then we need to recompile to debug issues as they
> > arise.
> 
> Well basically we would copy SLUB to SLAB apply unification patches to
> SLAB instead of SLUBB. We first have to make sure that the unified patches
> have the same performance as SLAB.
> 

I see, so all of the development will be done in Pekka's tree on mm/slub.c 
and then when we can see no performance regression compared to the slab 
baseline, merge it into Linus' tree as mm/slab.c.  I'm not exactly sure 
how that set of diffs being sent to Linus would look.

Are the changes to slub in the unification patchset so intrusive that it 
wouldn't be possible to isolate many of the features under #ifdef or 
boot-time options in a single, truly unified, allocator?  It seems like a 
shame that we'll have two allocators where the base is the same and much 
of the debugging code is the same.

> It maybe much better to isolate the debug features and general bootstrap
> from the particulars of the allocation strategy of either SLUB or SLAB.
> That way a common code base exists and it would be easier to add different
> allocation strategies.
> 
> Basically have slab.c with the basic functions and then slab_queueing.c
> and slab_noqueue.c for SLAB/SLUB with the particulars of the allocation
> strategy?
> 

I was going to mention that as an idea, but I thought storing the metadata 
for certain debugging features might differ from the two allocators so 
substantially that it would be even more convoluted and difficult to 
maintain?

Re: [PATCH net-next] net: allocate skbs on local node

[Christoph Lameter]

On Wed, 13 Oct 2010, David Rientjes wrote:

> > Basically have slab.c with the basic functions and then slab_queueing.c
> > and slab_noqueue.c for SLAB/SLUB with the particulars of the allocation
> > strategy?
> >
>
> I was going to mention that as an idea, but I thought storing the metadata
> for certain debugging features might differ from the two allocators so
> substantially that it would be even more convoluted and difficult to
> maintain?

We could have some callbacks to store allocator specific metadata?

Re: [PATCH net-next] net: allocate skbs on local node

[David Rientjes]

On Wed, 13 Oct 2010, Christoph Lameter wrote:

> > I was going to mention that as an idea, but I thought storing the metadata
> > for certain debugging features might differ from the two allocators so
> > substantially that it would be even more convoluted and difficult to
> > maintain?
> 
> We could have some callbacks to store allocator specific metadata?
> 

It depends on whether we could share the same base for both slab (unified 
allocator) and slub, which you snipped from your reply, that would make 
this cleaner.

Re: [PATCH net-next] net: allocate skbs on local node

[Pekka Enberg]

On Wed, 13 Oct 2010, Christoph Lameter wrote:
>>> I was going to mention that as an idea, but I thought storing the metadata
>>> for certain debugging features might differ from the two allocators so
>>> substantially that it would be even more convoluted and difficult to
>>> maintain?
>>
>> We could have some callbacks to store allocator specific metadata?

On 10/14/10 1:41 AM, David Rientjes wrote:
> It depends on whether we could share the same base for both slab (unified
> allocator) and slub, which you snipped from your reply, that would make
> this cleaner.

Argh. Why would we want to introduce something that's effectively a new 
allocator based on SLUB? If there's something controversial in the 
current patch series, lets just keep it out of mainline. A "rewrite" is 
the reason we're in this mess so lets not repeat the same mistake again!

			Pekka

Re: [PATCH net-next] net: allocate skbs on local node

[David Rientjes]

On Thu, 14 Oct 2010, Pekka Enberg wrote:

> Argh. Why would we want to introduce something that's effectively a new
> allocator based on SLUB? If there's something controversial in the current
> patch series, lets just keep it out of mainline. A "rewrite" is the reason
> we're in this mess so lets not repeat the same mistake again!
> 

SLUB is a good base framework for developing just about any slab allocator 
you can imagine, in part because of its enhanced debugging facilities.  
Nick originally developed SLQB with much of the same SLUB framework and 
the queueing changes that Christoph is proposing in his new unified 
allocator builds upon SLUB.

Instead of the slab.c, slab_queue.c, and slab_nonqueue.c trifecta, I 
suggested building as much of the core allocator into a single file as 
possible and then extending that with a config option such as 
CONFIG_SLAB_QUEUEING, if possible.  Christoph knows his allocator better 
than anybody so he'd be the person to ask if this was indeed feasible and, 
if so, I think it's in the best interest of a long-term maintainable 
kernel.

I care about how this is organized because I think the current config 
option demanding users select between SLAB and SLUB without really 
understanding the differences (especially for users who run a very wide 
range of applications and the pros and cons of better microbenchmark 
results for one allocator over another isn't at all convincing) is 
detrimental.

Re: [PATCH net-next] net: allocate skbs on local node

[Christoph Lameter]

On Wed, 13 Oct 2010, David Rientjes wrote:

> On Wed, 13 Oct 2010, Christoph Lameter wrote:
>
> > > I was going to mention that as an idea, but I thought storing the metadata
> > > for certain debugging features might differ from the two allocators so
> > > substantially that it would be even more convoluted and difficult to
> > > maintain?
> >
> > We could have some callbacks to store allocator specific metadata?
> >
>
> It depends on whether we could share the same base for both slab (unified
> allocator) and slub, which you snipped from your reply, that would make
> this cleaner.

I already said before that we should consider having a common base so I
thought that was not necessary.