Re: 2.6.24-rc2: Network commit causes SLUB performance regression with tbench

[Nick Piggin <nickpiggin@yahoo.com.au>]

cc'ed linux-netdev

On Saturday 10 November 2007 10:46, Christoph Lameter wrote:
> commit deea84b0ae3d26b41502ae0a39fe7fe134e703d0 seems to cause a drop
> in SLUB tbench performance:
>
> 8p x86_64 system:
>
> 2.6.24-rc2:
> 	1260.80 MB/sec
>
> After reverting the patch:
> 	2350.04 MB/sec
>
> SLAB performance (which is at 2435.58 MB/sec, ~3% better than SLUB) is not
> affected by the patch.

Ah, I didn't realise this was a regression. Thanks for bisecting it.


> Since this is an alignment change it seems that tbench performance is
> sensitive to the data layout? SLUB packs data more tightly than SLAB. So
> 8 byte allocations could result in cacheline contention if adjacent
> objects are allocated from different cpus. SLABs minimum size is 32
> bytes so the cacheline contention is likely more limited.

> Maybe we need to allocate a mininum of one cacheline to the skb head? Or
> padd it out to a full cacheline?

The data should already be cacheline aligned. It is kmalloced, and
with a minimum size of somewhere around 200 bytes on a 64-bit machine.
So it will hit a cacheline aligned kmalloc slab AFAIKS  -- cacheline
interference is probably not the problem. (To verify, I built slub with
minimum kmalloc size set to 32 like slab and it's no real difference)

But I can't see why restricting the allocation to PAGE_SIZE would help
either. Maybe the macros are used in some other areas.

BTW. your size-2048 kmalloc cache is order-1 in the default setup,
wheras kmalloc(1024) or kmalloc(4096) will be order-0 allocations. And
SLAB also uses order-0 for size-2048. It would be nice if SLUB did the
same...


> commit deea84b0ae3d26b41502ae0a39fe7fe134e703d0
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Sun Oct 21 16:27:46 2007 -0700
>
>     [NET]: Fix SKB_WITH_OVERHEAD calculation
>
>     The calculation in SKB_WITH_OVERHEAD is incorrect in that it can cause
>     an overflow across a page boundary which is what it's meant to prevent.
>     In particular, the header length (X) should not be lumped together with
>     skb_shared_info.  The latter needs to be aligned properly while the
> header has no choice but to sit in front of wherever the payload is.
>
>     Therefore the correct calculation is to take away the aligned size of
>     skb_shared_info, and then subtract the header length.  The resulting
>     quantity L satisfies the following inequality:
>
>         SKB_DATA_ALIGN(L + X) + sizeof(struct skb_shared_info) <= PAGE_SIZE
>
>     This is the quantity used by alloc_skb to do the actual allocation.
>     Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f93f22b..369f60a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -41,8 +41,7 @@
>  #define SKB_DATA_ALIGN(X)      (((X) + (SMP_CACHE_BYTES - 1)) & \
>                                  ~(SMP_CACHE_BYTES - 1))
>  #define SKB_WITH_OVERHEAD(X)   \
> -       (((X) - sizeof(struct skb_shared_info)) & \
> -        ~(SMP_CACHE_BYTES - 1))
> +       ((X) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
>  #define SKB_MAX_ORDER(X, ORDER) \
>         SKB_WITH_OVERHEAD((PAGE_SIZE << (ORDER)) - (X))
>  #define SKB_MAX_HEAD(X)                (SKB_MAX_ORDER((X), 0))