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: Windows password security audit tool. GUI, reports in PDF.

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

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