netdev - Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

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]

Message-ID: <1335597403.2900.29.camel@edumazet-glaptop>
Date:	Sat, 28 Apr 2012 09:16:43 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Alexander Duyck <alexander.h.duyck@...el.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>,
	Neal Cardwell <ncardwell@...gle.com>,
	Tom Herbert <therbert@...gle.com>,
	Maciej Żenczykowski <maze@...gle.com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Ben Hutchings <bhutchings@...arflare.com>,
	Matt Carlson <mcarlson@...adcom.com>,
	Michael Chan <mchan@...adcom.com>,
	Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

On Fri, 2012-04-27 at 17:27 -0700, Alexander Duyck wrote:
> On 04/27/2012 03:33 AM, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@...gle.com>
> >

> > +struct sk_buff *build_skb(void *data, unsigned int frag_size)
> >  {
> >  	struct skb_shared_info *shinfo;
> >  	struct sk_buff *skb;
> > -	unsigned int size;
> > +	unsigned int size = frag_size ? : ksize(data);
> >  
> >  	skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
> >  	if (!skb)
> >  		return NULL;
> >  
> > -	size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > +	size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >  
> >  	memset(skb, 0, offsetof(struct sk_buff, tail));
> >  	skb->truesize = SKB_TRUESIZE(size);
> > +	skb->head_frag = frag_size != 0;
> >  	atomic_set(&skb->users, 1);
> >  	skb->head = data;
> >  	skb->data = data;
> 
> This doesn't seem right to me.  You are only counting the piece of the
> page that got filled with data and the piece that will get overwritten
> with the shared info.  What about the rest of the page?  It looks like
> in the tg3 patch you have the driver using a half page.  Based on this
> function I suspect the resultant truesize would be something like 64 +
> 256 + 320 for an ack.  Shouldn't your truesize in that case be 2048 + 256?

Re-reading your mail, I think you missed fact that tg3 driver currently
uses a kmalloc(64+1500+14+SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)) , so basically does a kmalloc(2048). But this is done
before NIC fills the frame in it, so we cant know what will be the frame
length...

So build_skb() does later a ksize(data) and this gives 2048, even for a
small ACK packet ...

So the spirit of this patch is not to change any truesize.

tg3 for example splits a PAGE_SIZE into 2048 bytes frags (2 frags on x86
for example). Its done about the same in Intel IGB driver (IGB assumes
PAGE_SIZE is 4096 since it uses PAGE_SIZE/2)

The only thing that is changed here is where skb->head is allocated
from : kmalloc() caches or a frag from a page (and one reference to
page->_count)

Hope this clears your concern ?


--
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