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
| ||
|
Date: Thu, 05 Jan 2012 21:22:48 +0100 From: Eric Dumazet <eric.dumazet@...il.com> To: Ian Campbell <Ian.Campbell@...rix.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 Le jeudi 05 janvier 2012 à 19:19 +0000, Ian Campbell a écrit : > On Thu, 2012-01-05 at 17:46 +0000, Eric Dumazet wrote: > > Le jeudi 05 janvier 2012 à 17:13 +0000, Ian Campbell a écrit : > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > > index da0c97f..b6de604 100644 > > > --- a/net/core/skbuff.c > > > +++ b/net/core/skbuff.c > > > @@ -189,8 +189,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > > > * aligned memory blocks, unless SLUB/SLAB debug is enabled. > > > * Both skb->head and skb_shared_info are cache line aligned. > > > */ > > > - size = SKB_DATA_ALIGN(size); > > > - size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > > > + size = SKB_DATA_ALIGN(size + sizeof(struct skb_shared_info)); > > > data = kmalloc_node_track_caller(size, gfp_mask, node); > > > if (!data) > > > goto nodata; > > > > Unfortunately this makes the frequently use part of skb_shared_info not > > any more in a single cache line. > > > > This will slow down many operations like kfree_skb() of cold skbs (TX > > completion for example) > > I've been toying with the following which moves the infrequently used > destructor_arg field to the front. With 32 or 64 byte cache lines this > is then the only field which ends up on a different cache line. With 128 > byte cache lines it is all already on the same line. > > Does that alleviate your concern? If so I can clean it up and include it > in the series. > > Cheers, > Ian. > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index cbc815c..533c2ea 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -249,6 +249,11 @@ struct ubuf_info { > * the end of the header data, ie. at skb->end. > */ > struct skb_shared_info { > + /* Intermediate layers must ensure that destructor_arg > + * remains valid until skb destructor */ > + void * destructor_arg; > + > + /* Warning all fields from here until dataref are cleared in __alloc_skb() */ > unsigned char nr_frags; > __u8 tx_flags; > unsigned short gso_size; > @@ -264,9 +269,6 @@ struct skb_shared_info { > */ > atomic_t dataref; > > - /* Intermediate layers must ensure that destructor_arg > - * remains valid until skb destructor */ > - void * destructor_arg; > > /* must be last field, see pskb_expand_head() */ > skb_frag_t frags[MAX_SKB_FRAGS]; Hmmm Take a look at kfree_skb() and please list all skb_shared_info needed fields, for a typical skb with nr_frags= 0 or 1. If all fit in a single cache line, its ok. If not, this adds a cache line miss in a critical path. Also in transmit path, when an skb is queued by cpu X on qdisc, and dequeued by another cpu Y to be delivered to device. -- 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
Powered by blists - more mailing lists