Message-ID: <CAKgT0UeKTW8ued4NFcbqiSOy5xyhPKAcZUtb-nUmMC13uHO-Yw@mail.gmail.com>
Date:   Thu, 7 Dec 2017 15:35:22 -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 3:17 PM, Michael Chan <michael.chan@...adcom.com> wrote:
> On Thu, Dec 7, 2017 at 2:43 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Thu, Dec 7, 2017 at 2:08 PM, Michael Chan <michael.chan@...adcom.com> wrote:
>>> 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.
>
> Only LRO behaves the way you describe.  I believe LRO is treated like
> this differently because bridging or routing needs to be able to
> disable it completely.
>
> GRO is not like that.  RXCSUM is not like that.

I think that what your saying is your patch has exposed a bug. If I
disable GRO or GRO_HW on the upper device I would expect to have it
disabled on all lower devices. If anything it should probably be added
to NETIF_F_UPPER_DISABLES. Otherwise we are going to be breaking
things like XPS which you pointed out earlier.

>> 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.
>
> I think you keep thinking that whatever we do with LRO, we must do the
> same thing with GRO_HW.
>
> But that's not true.  GRO/GRO_HW don't have to be disabled when
> bridging/routing are enabled.  That's why upper devices don't have to
> treat GRO/GRO_HW like LRO.  It can be treated just like RXCSUM which
> is don't care.
>
>>
>> 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.
>
> I don't get this.  I don't see how TSO is related.

It isn't. That is the point. If I change ANY feature it will trigger
fix_features to get called. It will then see we have GRO_HW and LRO
since we have one interface that supports one and one that supports
another. It will trigger this code and cause LRO to be disabled which
then will be propagated down.

>> 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.
>>
>
> Today, GRO, RXCSUM, NTUPLE, etc on a bond do not get propagated.  You
> can propose and make them propagate if you want.

Or you can fix your patches so you take it into account now instead of
putting things in a broken state and asking others to fix it.

> So GRO_HW just naturally follows GRO since it is a true subset of it.

Right, and I am saying you have exposed a bug. If I don't want GRO on
the bond it should be propagated down. It doesn't make sense to allow
lower devices to assemble frames when the bond doesn't want them and
GSO won't kick in before it gets to the bond.

Powered by blists - more mailing lists