Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Sabrina Dubroca <sd@queasysnail.net>]

2017-12-22, 10:14:32 -0800, Alexander Duyck wrote:
> On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> > IIUC, with the patches that were applied, each driver can define
> > whether GRO_HW depends on GRO? From a user's perspective, this
> > inconsistent behavior is going to be quite confusing.
> >
> > Worse than inconsistent behavior, it looks like a driver deciding that
> > GRO_HW doesn't depend on GRO is going to introduce a change of
> > behavior.  Previously, when GRO was disabled, there wouldn't be any
> > packet over MTU handed to the network stack.  Now, even if GRO is
> > disabled, GRO_HW might still be enabled, so we might get over-MTU
> > packets because of hardware GRO.
> 
> This isn't actually true. LRO was still handling packets larger than
> MTU over even when GRO was disabled.

Sure, LRO will also cause that, but we're speaking in the context of
GRO here, which means no LRO.


> > I don't think drivers should be allowed to say "GRO_HW doesn't depend
> > on GRO".
> 
> Why not, it doesn't. In my mind GRO_HW is closer to LRO than it is to
> GRO.

Why do you say that? It looks like GRO to me. These drivers are
calling tcp_gro_complete() for example.

> The only ugly bit as I see it is that these devices were exposing
> the feature via the GRO flag in the first place. So for the sake of
> legacy they might want to carry around the dependency.
> 
> > I think it's reasonable to be able to disable software GRO even if
> > hardware GRO is enabled. Thus, I would propose:
> > - keep the current GRO flag
> > - add a GRO_HW flag, depending on GRO, enforced by the core as in
> >   earlier versions of these patches
> > - add a GRO_SW flag, also depending on GRO
> 
> This seems like a bunch of extra overhead for not much gain. Do we
> really need to fork GRO into 3 bits? I would argue that GRO_HW really
> should have been branded something like FORWARDABLE_LRO, but nobody
> wanted to touch the name LRO since it apparently has some negative
> stigma to it. If we had used a name like that we probably wouldn't be
> going through all these extra hoops. The only real reason why this is
> even being associated with GRO in the first place is that is how this
> feature was hidden by the drivers so they got around having to deal
> with the LRO being disabled for routing/forwarding issue. Those are
> the parts that want to keep it associated with GRO since that is how
> they exposed it in their devices originally.

I think it wouldn't have hidden behind GRO if it wasn't GRO. Again,
why do you think it's not GRO?

-- 
Sabrina