Message-ID: <CACKFLik5dEaVsbgybPk0dKSVN8j+w6BLVT_thRzxsXkQMmeuKA@mail.gmail.com>
Date:   Thu, 7 Dec 2017 15:17:59 -0800
From:   Michael Chan <michael.chan@...adcom.com>
To:     Alexander Duyck <alexander.duyck@...il.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: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.

>
> 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.

> 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.

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

Powered by blists - more mailing lists