On Fri, Dec 29, 2017 at 4:43 AM, Sabrina Dubroca <s...@queasysnail.net> wrote: > 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.
We are talking about GRO_HW. Which is basically aggregation being performed in hardware. The choice of name is unfortunate though since it implies this is GRO when what is actually happening is just GRO-like. Really the only difference between LRO and GRO_HW is that the frames are better formed in the final result. It is a matter of what is performed versus where it is performed. >> > 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 final frame output in the case of frames that are aggregated will be the same as GRO. However LRO could theoretically do the same thing if the hardware had been implemented correctly in the first place. >> 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? The only real piece I see as missing the propagation bits for GRO_HW to upper devices. The argument was that GRO doesn't have to do that, but I think we are going to have to get there at some point so that when we encounter the first piece of hardware that does this wrong we have a switch ready to allow us to disable it for debugging. If we are insistent on having a switch that binds together GRO and GRO_HW one thought I had was to change the meaning of GRO_HW for virtual devices to indicate that we allow GRO to happen on the lower devices associated with this device. My thought was that GRO_HW allows GRO to take place on the physical hardware below the netdev, so why couldn't we say that GRO_HW also applies to upper devices and them indicating they don't want GRO, GRO_HW, or LRO for lower devices. Thoughts? The only reason it occurred to me is that there is currently no way to propagate a disable of GRO so GRO_HW might be a way to do that for virtual devices. It would make GRO_HW more about the location of the GRO feature instead of the feature itself. Basically if you clear it then nothing below that point should be doing any sort of aggregation. Thanks. - Alex
