Message-ID: <537CD80F.2090909@intel.com>
Date:	Wed, 21 May 2014 09:45:03 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Alexander Duyck <alexander.duyck@...il.com>,
	David Miller <davem@...emloft.net>, amirv@...lanox.com,
	netdev@...r.kernel.org, idos@...lanox.com,
	jeffrey.t.kirsher@...el.com, jesse.brandeburg@...el.com,
	bruce.w.allan@...el.com, carolyn.wyborny@...el.com,
	donald.c.skidmore@...el.com, gregory.v.rose@...el.com,
	john.ronciak@...el.com, mitch.a.williams@...el.com,
	yevgenyp@...lanox.com, ogerlitz@...lanox.com
Subject: Re: [PATCH net-next 1/2] net: Expose header length compution function

On 05/21/2014 08:51 AM, Eric Dumazet wrote:
> On Wed, 2014-05-21 at 08:03 -0700, Alexander Duyck wrote:
>
>> So it looks like you did kind of what I expected you would, only you
>> allocated a temporary sk_buff on the stack and then pointed the head to
>> the start of the page.  I'm not really a fan of this approach though it
>> does give me a couple ideas.
>>
>> One thought I just had though, what if we were to do something like
>> create an eth_build_skb function?
> Well, it all depends if you use napi_get_frags() / napi_gro_frags(),
> which are normally the way to get very fast GRO processing, since
> you don't even have to allocate memory for the skbs at all, since
> skb will likely be recycled in napi_reuse_skb()

Another thought would be to possibly look into a GRO type approach. 
Something where we could place the length parsing functions in the
offload_callbacks.  If we could do that then we could just integrate the
functionality with GRO and make use of those callbacks.  Basically it
would require doing the parsing as a part of napi_frags_skb() so that
when we do the pull we get the full header length in one shot so that
the entire frame is linear right from the start.

Actually the more I think about this now the more it makes sense.  We
could probably pull out all the skb_gro_header_hard/skb_gro_header_slow
length bits from the existing gro_receive functions and place them in
another piece of the code.

>>   It would essentially be a cross
>> between eth_type_trans, your new eth_frame_headlen function, and
>> build_skb.  It would allow us to avoid the unnecessary allocation of an
>> skb on the stack and avoid any unnecessary data duplication since we
>> already would be doing a number of the eth_type_trans steps in your
>> eth_frame_headlen function.  The one limitation is that we would need to
>> allocate a block of memory for the head, but that would be done after we
>> figure out what the size of the header is.
> 'Allocating' an skb on stack has no cost. Exactly 0 added instructions.
>
> It only increases the size of stack, and at this point we are before all
> the networking stacks, so it is safe.
>
> Have you seen the eBPF stuff adding more stack usage than this ?
>
> #define MAX_BPF_STACK 512

We have had stack smashing issues in the past with the ixgbe interrupt
handlers and it wasn't consuming much memory on the stack as I recall. 
I prefer to err on the side of caution.

Also the more I think about it I am not really comfortable putting a
partially initialized sk_buff through any function calls.  It seems like
it is setting somebody up for a failure because if at some point the
code changes and needs some other field out of the skb it won't be
initialized here unless they catch this tricky bit of code.








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