Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

[Eric Dumazet <eric.dumazet@gmail.com>]

On Tue, 2013-01-15 at 09:28 +0200, Yuval Mintz wrote:
> On 01/14/2013 08:44 PM, Eric Dumazet wrote:
> > On Mon, 2013-01-14 at 09:22 -0800, Eric Dumazet wrote:
> > 
> > What is the value of gso_segs ?
> > 
> > The reason I am pointing this out is the recent change in commit
> > 1def9238d4aa2146924994aa4b7dc861f03b9362
> > (net_sched: more precise pkt_len computation)
> > 
> > bnx2x not setting gso_segs means that qdisc accounting on ingress is
> > completely wrong.
> 
> Hi Eric,
> 
> First I just want to state that you're totally correct about the gso_segs -
> bnx2x is not setting it correctly (it's currently totally omitted), and so
> it would incorrectly affect the accounting.
> 
> However, notice this behaviour was not introduced in this patch -
> Since 621b4d6 bnx2x is using FW GRO, overriding the kernel's GRO implementation:
> As the bnx2x driver is supplied with the aggregated packet from its FW,
> the bnx2x passes a value in the `gso_size' field of its skb, causing
> `skb_is_gso' to return `true'.
> This will cause the aggregated skb to override the GRO machinations
> (in `dev_gro_receive'), overriding all calls to `gro_receive' and thus also
> the call to `skb_gro_receive' which whould have incremented `gso_segs'.
> 
> This patch actually tries to correct said behaviour, obviously failing
> with the gso_segs but still improving the current state of bnx2x GRO
> in bridging scenarios.
> 
> >> This looks weird to me. This should be called by GRO stack only.
> 
> I think this is the main question - we could try and implement this
> inside the network-core itself, but as said behaviour is unique to the
> bnx2x driver (correct me if I'm wrong, but I'm unaware of any other
> driver which does GRO without the kernel GRO implementation), the
> solution is specially tailored for the bnx2x driver.
> 
> We could either:
>   1. Continue with this patch, later sending a patch correcting gso_segs,
>      as this is not a new issue.
>   2. Send a V2 patch-series which will also set gso_segs correctly.
>   3. Send a V2 patch-series which omits this patch, and later send an RFC
>      for some kernel implementation which fixes the issue.
> 
> Your thoughts on this matter will be greatly appreciated.

I am fine with any solution, as long as we fix the problem.

If GRO is done by the FW/driver instead of core network stack, we should
make sure :

- transport_header is set correctly (your patch seems to do that)
- gso_segs is computed (this could be done in core network, but this
adds yet another conditional test in th fast path, and it seems only
bnx2x would need this)

Thanks