Re: PATCH: kmalloc packet slab

Re: PATCH: kmalloc packet slab

[Patrick McHardy]

Alan Cox wrote:
> The networking world runs in 1514 byte packets pretty much all the time.
> This adds a 1620 byte slab for such objects and is one of the internally
> generated Red Hat patches we use on things like Fedora Core 3. Original:
> Arjan van de Ven.
> 
> Signed-off-by: Alan Cox <alan@redhat.com>

Why 1620 bytes ? Most drivers allocate packet_size + 2 bytes.
dev_alloc_skb adds another 16 bytes, finally alloc_skb adds
sizeof(struct skb_shared_info). So we get:

(32bit): 1514b + 2b + 16b + 160b = 1692b
(64bit): 1514b + 2b + 16b + 312b = 1844b

On paths using alloc_skb instead of dev_alloc_skb it's 16 bytes
less, but 1620 bytes is still too small for full-sized packets.

Regards
Patrick

Re: PATCH: kmalloc packet slab

[David S. Miller]

On Mon, 27 Dec 2004 18:17:32 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Alan Cox wrote:
> > The networking world runs in 1514 byte packets pretty much all the time.
> > This adds a 1620 byte slab for such objects and is one of the internally
> > generated Red Hat patches we use on things like Fedora Core 3. Original:
> > Arjan van de Ven.
> > 
> > Signed-off-by: Alan Cox <alan@redhat.com>
> 
> Why 1620 bytes ? Most drivers allocate packet_size + 2 bytes.
> dev_alloc_skb adds another 16 bytes, finally alloc_skb adds
> sizeof(struct skb_shared_info). So we get:
> 
> (32bit): 1514b + 2b + 16b + 160b = 1692b
> (64bit): 1514b + 2b + 16b + 312b = 1844b
> 
> On paths using alloc_skb instead of dev_alloc_skb it's 16 bytes
> less, but 1620 bytes is still too small for full-sized packets.

Absolutely, there is no way this patch actually helps for
full sized frames.  Another thing in the above equations is
that on output you have to add in MAX_TCP_HEADER which is
128 + MAX_HEADER. MAX_HEADER is variable sized based upon
which link layer support is built into the kernel.

Even on input, many ethernet device drivers add in their
own amounts to the size for DMA and cache-line alignment.

So this special slab would never be used on output even
if it got the base equations correct.

If we are really going to do something like this, it should
be calculated properly and be determined per-interface
type as netdevs are registered.

Special casing ethernet is just rediculious.

Re: PATCH: kmalloc packet slab

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

On Mon, 27 Dec 2004 14:23:50 PST, "David S. Miller" said:

> If we are really going to do something like this, it should
> be calculated properly and be determined per-interface
> type as netdevs are registered.

Would you prefer to see this done for all interface types if we do it
at all, or would a special-case for 1 or 2 types that can use a slab
without being wasteful be an acceptable solution? (Let's face it - if
3.95 objects fit in each slab, we may not want to do it...)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: PATCH: kmalloc packet slab

[David S. Miller]

On Mon, 27 Dec 2004 17:50:01 -0500
Valdis.Kletnieks@vt.edu wrote:

> On Mon, 27 Dec 2004 14:23:50 PST, "David S. Miller" said:
> 
> > If we are really going to do something like this, it should
> > be calculated properly and be determined per-interface
> > type as netdevs are registered.
> 
> Would you prefer to see this done for all interface types if we do it
> at all, or would a special-case for 1 or 2 types that can use a slab
> without being wasteful be an acceptable solution? (Let's face it - if
> 3.95 objects fit in each slab, we may not want to do it...)

It's not even just device MTU based (which can change dynamically
at run time), it's also based upon the PMTU for various paths.

As for wastefulness, that's a good question.  Adding a mechanism
to do kmalloc slabs dynamically doesn't sound all that wise.  That
would undo all the inlining tricks.

Probably a better idea is to provide a way to attach a slab to
an SKB's data area so that we can have per-device SLABs for this
kind of stuff (and if all "ethernet" devices want to share the
same SLAB, that's fine too, but it won't help all ethernet drivers
for reasons outlined in my previous email).

We added something similar for the Xen folks, and it's in Linus's
BK tree right now.  It's named alloc_skb_from_cache().

What I'd really like to see is device based determination of the
correct slab to use.  Unfortunately, dev_alloc_skb() doesn't take
a netdev argument, which is truly offensive.  Otherwise we could
just stick the necessary logic there.

Re: PATCH: kmalloc packet slab

[Alan Cox]

On Llu, 2004-12-27 at 22:23, David S. Miller wrote:
> If we are really going to do something like this, it should
> be calculated properly and be determined per-interface
> type as netdevs are registered.

Fine by me, I'm just going through plausible looking changes in the Red
Hat tree. You might want to slightly injure someone internally until
they drop that too 8)

Alan

Re: PATCH: kmalloc packet slab

[Dave Jones]

On Tue, Dec 28, 2004 at 12:51:28AM +0000, Alan Cox wrote:
 > On Llu, 2004-12-27 at 22:23, David S. Miller wrote:
 > > If we are really going to do something like this, it should
 > > be calculated properly and be determined per-interface
 > > type as netdevs are registered.
 > 
 > Fine by me, I'm just going through plausible looking changes in the Red
 > Hat tree. You might want to slightly injure someone internally until
 > they drop that too 8)

Internal injuries unnecessary. Regardless of outcome of this patch,
Fedora will pick up whatever happens upstream instead of carrying
this any longer. This and a few other patches have been stagnating
in our tree for far longer than they should have been.

		Dave

Re: PATCH: kmalloc packet slab

[Andi Kleen]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> The networking world runs in 1514 byte packets pretty much all the time.
> This adds a 1620 byte slab for such objects and is one of the internally
> generated Red Hat patches we use on things like Fedora Core 3. Original:
> Arjan van de Ven.

Doesnt this clash a bit with yours and Arjans no-prisoners-taken 
quest to get rid of order>0 allocations? (4K stacks). 

I implemented this long ago (in 2.1 - bonus points if you still find
the leftover hook), but then gave up on it. I realized that to
use it you would need order>0 allocations. In a single 4K page only 2
1.5K slabs fit, but 2 2K slabs fit as well. And there is already a handy
2K slab that works perfect well.

IMHO it is useless except for architectures with PAGE_SIZE>4K or if 
you fix the VM to handle order>0 allocations really well. If you want
to add it for sparc64/ia64/alpha etc. I would do it with an ifdef
at least. 

-Andi