On Thu, Dec 7, 2017 at 6:36 PM, Alexander Duyck <alexander.du...@gmail.com> wrote: > On Thu, Dec 7, 2017 at 4:05 PM, Michael Chan <michael.c...@broadcom.com> > wrote: >> I see. But this won't happen. Because the bonding driver is not >> advertising NETIF_F_GRO_HW in its hw_features. It is not advertising >> NETIF_F_GRO either but I think it gets added automatically since it is >> a software feature. So LRO won't get disabled on the bond when a >> unrelated feature is changed. >> >> But I think I see your point. I can make it so that it is up to >> individual driver's .ndo_fix_features() to drop LRO/GRO_HW as it sees >> fit, instead of doing it in the common netdev_fix_features(). That >> way, it is more flexible at least. > > Thank you.
OK. I will make this change for V3. >> >> I don't think that things are necessarily broken today. LRO truly >> needs to be propagated. It's debatable whether other features like >> GRO/RXCSUM/NTUPLE should be centrally set by the upper device or not. > > So I can agree with the NTUPLE not being propagated since it doesn't > actually effect upper devices. Really the functionality only really > has effects locally since the functionality consists of route to a > specific queue/device or drop the packet. > > I'm not sure why RXCSUM isn't being propagated. It seems like that is > something that would make sense to have passed all the way down to the > lower devices since a single device that is doing bad Rx checksum > offloads could potentially corrupt all traffic in a bond. Seems like > that one should definitely be included. This is a separate discussion that goes beyond GRO. There are pros and cons for the upper device to propagate every single feature flag. >> >> GRO kicks in at the lower device before it gets to the bond if the >> lower device calls napi_gro_receive() and GRO is enabled. > > I get that. I assume the reason why the bond doesn't have it enabled > is because we don't want it to kick in at every given netdev, there > isn't any point to do GRO more than once. The problem is GRO_HW isn't > a pure software offload like GRO is. Call me a pessimist, but when we > end up encountering a buggy implementation that has to be disabled we > will want the right infrastructure in place to handle it. It becomes > another argument for why we might want to split GRO_HW and GRO without > tying them together. It would make sense to expose GRO_HW in a bond, > but not GRO. It might be something where we want to do any close tying > together of the GRO flag and GRO_HW at the driver as well. Basically > the legacy devices that transition over to GRO_HW from using just the > GRO flag could do that to maintain existing functionality, and new > drivers that implement it could opt in to the same behavior or just > handle GRO_HW as a separate flag. To me, making GRO_HW dependent on GRO makes the most intuitive sense. Separating them is just confusing. The possibility of GRO_HW being enabled without GRO enabled makes no sense to me. > > Actually I just had a thought. What if we consider this a separate GRO > stage instead of just a hardware offload? Our standard GRO is a post > receive from the driver perspective, basically the packet is assembled > after we have handed it to the stack. What you are doing with GRO_HW > is essentially providing an early reassembly before it is handed to > the stack. What if we were to rename GRO_HW to something like > GRO_LOWER, GRO_EARLY, GRO_PRE, or pick your name (I'm lousy at > naming), and used it as a way to indicate that we want to perform GRO > before we begin receive processing on the frame in our driver? Then > for stacked devices you could use this new flag to indicate you don't > want to perform GRO on the lower levels below this device, and could > then use the regular GRO flag to control if we do it ourselves. Doing > that should provide stacked devices with a good way to control GRO on > the lower devices and would resolve what you need to indicate as well. > The only real changes needed might be a rename and to add the > necessary bit shifting for the upper and lower dev sync code. If you > aren't interested in the idea I can probably spend a couple of hours > getting to it tomorrow since I think this might be a much better way > to go as it solves multiple issues. I really don't see what a different name will buy us. If you want to propagate GRO/GRO_HW, we can do that if others agree. I only feel strongly that GRO/GRO_HW should be tied. I don't feel strongly whether GRO/GRO_HW should be propagated or not propagated. Again, LRO needs to be propagated out of necessity (e.g. when a bond is added to a bridge).
