On Sat, Dec 9, 2017 at 10:49 PM, Michael Chan <michael.c...@broadcom.com> wrote: > On Sat, Dec 9, 2017 at 2:37 PM, Alexander Duyck > <alexander.du...@gmail.com> wrote: >> On Sat, Dec 9, 2017 at 1:40 PM, Michael Chan <michael.c...@broadcom.com> >> wrote: >>> On Sat, Dec 9, 2017 at 10:56 AM, Alexander Duyck >>> <alexander.du...@gmail.com> wrote: >>>> I think these two lines are redundant in dev_disable_lro, since >>>> netdev_update_features should propagate the disable to all of the >>>> lower devices. >>> >>> Right. But for GRO_HW, there is no automatic propagation. >> >> Right, but that is also an issue since the automatic propagation is >> what prevents LRO from being re-enabled on the lower devices. >> >>>> Also this doesn't prevent the lower devices from >>>> re-enabling gro_hw. >>> >>> Right. You can re-enable LRO on the upper device as well. >> >> On the upper device yes, but not on the lower devices. That was what I >> was getting at. With LRO there is netdev_sync_upper_features() and >> that prevents you from enabling it if the upper device has it >> disabled. The problem is right now there is nothing that sets it for >> the upper devices when they are added to something like a bond so that >> is one of the pieces that still has to be worked on before we can just >> use the existing sync logic. > > I understand. But if the user really wants to re-enable LRO, he can > just re-enable LRO on the upper device first and then re-enable LRO on > the lower devices. > > To permanently disable a feature, I think additional infrastructure > may be required so that the feature can be cleared in hw_features and > then re-enabled later when it is allowed.
I'm not saying we have to permanently ban it. But we want to prevent any mix-ups. If someone re-enables LRO on the upper device and then goes down the chain manually re-enabling it then they definitely wanted to do that. However if they disable LRO on the bond they cannot re-enable it on one of the slaves in the bond. That is the kind of propagation I am looking at. Basically if I disable some feature in the Rx path I should not see any frames that make use of that feature come by after it has been disabled. That is why I am thinking GRO, LRO, Rx Checksum, and GRO_HW all need to handle this sort of propagation, but doing it right is going to take some time since we have to make certain to enable things like Rx checksum on an upper device then if the feature is present on a lower device and that logic doesn't exist now and will be needed across multiple drivers such as bond and team among others. So for now I would say don't worry about lower devices. Just focus on the immediate device and we can work on getting the synchronization between the upper and lower devices sorted out over the next couple of weeks.
