Message-ID: <CAKgT0UdCRhiMHx4MS1zWQhBgMOYd0+9Go_a8753WW=-2c58L2Q@mail.gmail.com>
Date:   Thu, 7 Dec 2017 14:43:51 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        everest-linux-l2@...ium.com
Subject: Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO_HW.

On Thu, Dec 7, 2017 at 2:08 PM, Michael Chan <michael.chan@...adcom.com> wrote:
> On Thu, Dec 7, 2017 at 1:35 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Thu, Dec 7, 2017 at 10:44 AM, Michael Chan <michael.chan@...adcom.com> wrote:
>>> On Thu, Dec 7, 2017 at 10:13 AM, Alexander Duyck
>>> <alexander.duyck@...il.com> wrote:
>>>> On Thu, Dec 7, 2017 at 12:03 AM, Michael Chan <michael.chan@...adcom.com> wrote:
>>>>> @@ -7405,6 +7405,23 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>>>                 features &= ~dev->gso_partial_features;
>>>>>         }
>>>>>
>>>>> +       if (features & NETIF_F_GRO_HW) {
>>>>> +               /* Hardware GRO depends on GRO and RXCSUM. */
>>>>> +               if (!(features & NETIF_F_GRO)) {
>>>>> +                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
>>>>> +                       features &= ~NETIF_F_GRO_HW;
>>>>> +               }
>>>>
>>>> I'm not sure I agree with this part. The hardware offload should not
>>>> be dependent on the software offload.
>>>
>>> As I said before, GRO_HW is basically hardware accelerated GRO. To me,
>>> it makes perfect sense that GRO_HW depends on GRO.
>>>
>>>> If you are concerned with the
>>>> XDP case and such you should probably just look at handling it more
>>>> like how TSO is handled with a group of flags that aggregate GRO, LRO,
>>>> and GRO_HW into a group of features that must be disabled to support
>>>> XDP.
>>>>
>>>>> +               if (!(features & NETIF_F_RXCSUM)) {
>>>>> +                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
>>>>> +                       features &= ~NETIF_F_GRO_HW;
>>>>> +               }
>>>>> +               /* Hardware GRO and LRO are mutually exclusive. */
>>>>> +               if (features & NETIF_F_LRO) {
>>>>> +                       netdev_dbg(dev, "Dropping NETIF_F_LRO since GRO_HW is set.\n");
>>>>> +                       features &= ~NETIF_F_LRO;
>>>>> +               }
>>>>
>>>> This is going to be problematic. What if you bond one interface that
>>>> has LRO and one that has GRO_HW? It seems like this is going to cause
>>>> the bond interface to lie and say LRO isn't there when it actually is,
>>>> or worse yet it will force features off when they shouldn't be.
>>>
>>> This is just dropping incompatible features similar to how other
>>> incompatible feature flags are dropped in netdev_fix_features().
>>> Hardware cannot aggregate in both LRO and GRO_HW modes at the same
>>> time.  It's one or the other.
>>
>> I'm not saying this is on a single device. The problem is in the case
>> of bonding you have combined 2 devices. We don't want to have an upper
>> device saying LRO isn't there when it is. A bonding setup is supposed
>> to advertise LRO as being enabled if a lower device has it present.
>> You are now making this an either/or thing at the driver level
>> generically when that isn't the case due to things like bridging and
>> bonding. This is why I was arguing that it would be easier to just
>> make this a super-set of LRO with one form that is reversible and one
>> that isn't. As is you are introducing all sorts of regressions that
>> are going to be problematic in any sort of mixed environment.
>
> Are you saying that if LRO is dropped on a upper device (by the new
> code added in this patch), all lower devices will need to drop it too?
>
> If that's what you are saying, it's already taken care of.  There is
> netdev_sync_upper_features() and netdev_sync_lower_features() that
> will automatically do that as part of __netdev_update_features().
>
>>
>>> LRO and GRO_HW are different.  We may never agree on this but they are
>>> different.  If a bond needs to disable LRO, it will be propagated to
>>> all slaves.  But each slave can have GRO enabled.  If GRO is enabled,
>>> GRO_HW, being a subset of GRO, can be enabled.
>>
>> I get that they are very different. At a driver level it makes sense
>> to say you get one or the other at a physical driver level. The point
>> I was trying to make is that the bond is only able to set one or the
>> other when you might have a mix of devices. On the bonding device you
>> should be able to have both LRO and GRO_HW enabled.
>
> On the bond, you can have LRO enabled and it is propagated to lower
> devices so that lower devices will enable LRO if it is supported.  If
> LRO is disabled on the bond (e.g. due to bridging), It will be
> propagated so all lower devices will disable LRO if it is enabled.
>
> GRO/GRO_HW is different and it is not propagated from upper devices to
> lower devices like LRO.  Lower devices can have GRO/GRO_HW
> enabled/disabled regardless of upper device.  This is no different
> from say RXCSUM.  Lower devices can have RXCSUM on/off regardless.

So on the bond device you should be able to have LRO, GRO, and GRO_HW
enabled all at the same time and that means devices below it can have
any combination of those enabled. If I recall enabling it on the lower
device shouldn't be allowed if an upper device has it disabled. If it
is enabled on the upper device it can be either enabled or disabled on
the lower device depending on the state the device prefers.

The GRO and GRO_HW should be propagated just like RXCSUM and
everything else. Basically if I turn it off on the bond all of the
lower devices should have it disabled. Only if it is enabled should
lower devices be allowed to have it either enabled or disabled. The
easiest way to think of it is on the bond the features are like a mask
of what can be enabled/disabled for the lower devices. If a feature is
not present in the bond it should not be present on any of the devices
below it. That is why I am saying you cannot say LRO and GRO_HW are
mutually exclusive for all devices. For physical devices I would say
yes it can be mutually exclusive, but for things like bond it cannot
be since you can have one device doing LRO and one device doing GRO_HW
getting added to a bond and both features should be advertised there.

My concern is if I have a bond with both GRO_HW and LRO enabled on
lower devices, and then disable something like TSO we are going to see
LRO disabled on the bond and propagated to all the lower devices. That
is a bad behavior and shouldn't happen. In addition I would want to be
able to disable GRO_HW from the bond should I encounter a buggy
implementation of the GRO hardware offload at some point in the
future. I don't like it being tied so closely with GRO since all it
takes is one hardware failure and then we are dealing with people
disabling GRO due to reports of it being buggy instead of it being
tied specifically to GRO_HW.

- Alex

Powered by blists - more mailing lists