netdev - Re: [PATCH 2/6] net: pad skb data and shinfo as a whole rather than individually

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 6 Jan 2012 13:49:02 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH 2/6] net: pad skb data and shinfo as a whole rather than
 individually

On Fri, 2012-01-06 at 13:37 +0000, Eric Dumazet wrote:
> Le vendredi 06 janvier 2012 à 13:20 +0000, Ian Campbell a écrit :
> > On Fri, 2012-01-06 at 12:33 +0000, Eric Dumazet wrote:
> > > Le vendredi 06 janvier 2012 à 11:20 +0000, Ian Campbell a écrit :
> > > 
> > > > 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.
> 
> 
> 
> > (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
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html