On Fri, 2012-01-06 at 13:37 +0000, Eric Dumazet wrote:

Le vendredi 06 janvier 2012 à 13:20 +0000, Ian Campbell a écrit :

quoted

On Fri, 2012-01-06 at 12:33 +0000, Eric Dumazet wrote:

quoted

Le vendredi 06 janvier 2012 à 11:20 +0000, Ian Campbell a écrit :

quoted

It doesn't fit in a single cache line today.

It really does, thanks to your (net: pack skb_shared_info more
efficiently) previous patch.

I dont understand your numbers, very hard to read.

Current net-next :

offsetof(struct skb_shared_info, nr_frags)=0x0
offsetof(struct skb_shared_info, frags[1])=0x40   (0x30 on 32bit arches)

I see 0x48 here at cset 6386994e03ebbe60338ded3d586308a41e81c0dc:

If you read my mail, I said "current net-next" 

Please "git pull" again ?

Oh, oops, I thought I'd checked I was up to date.

Here I really have your commit 9f42f126154786e6e76df513004800c8c633f020
in.

quoted

(gdb) print &((struct skb_shared_info *)0)->nr_frags
$1 = (short unsigned int *) 0x0
(gdb) print &((struct skb_shared_info *)0)->frags[1]
$2 = (skb_frag_t *) 0x48
(gdb) print &((struct skb_shared_info *)0)->frags[0]
$3 = (skb_frag_t *) 0x38

(it's 0x34 and 0x2c on 32 bit) and these numbers match what I posted for
v3.1 (and I imagine earlier since this stuff doesn't seem to change very
often).

I provided the offsets of each field in struct skb_shared_info, which
one do you think is wrong?

Remember that the end shared info is explicitly aligned (to the end of
the allocation == a cache line) while the front just ends up at wherever
the size dictates so you can't measure the alignment of the fields from
the beginning  of the struct, you need to measure from the end.

You're mistaken. Really.

Current code makes SURE skb->end starts at a cache line boundary
(assuminf kmalloc() returned an aligned bloc, this might be not true
with SLAB debugging)

size = SKB_WITH_OVERHEAD(ksize(data));
...
skb->end = skb->tail + size;

So there is possibility of padding (less than 64 bytes) _after_
skb_shared_info and before the end of allocated area.

Oh, I somehow missed the SKB_DATA_ALIGN inside SKB_WITH_OVERHEAD when I
was reasoning about the before case (even though I touched it in my
patch!) thanks for prodding me sufficiently...

I guess the comment preceeding "size =" is therefore slightly misleading
since shinfo isn't "exactly at the end" but rather "at the end but still
aligned"?
	/* kmalloc(size) might give us more room than requested.
	 * Put skb_shared_info exactly at the end of allocated zone,
	 * to allow max possible filling before reallocation.
	 */

Anyhow, despite this I think with the patch to move destructor_arg to
the front ontop of this series the hot fields are all in a single cache
line, including the first frag. is that sufficient to alleviate your
concerns?

Ian.

After your 9f42f1261547 commit on 64bit, we have no padding anymore
since sizeof(struct skb_shared_info)=0x140