2017-12-22, 10:14:32 -0800, Alexander Duyck wrote: > On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <s...@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