Message-ID: <CACKFLikDKvkq449A6=PFSsJrUpgfeogutawsW=nDApVfib92CQ@mail.gmail.com>
Date:   Sat, 9 Dec 2017 13:31:05 -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 v3 1/5] net: Introduce NETIF_F_GRO_HW.

On Sat, Dec 9, 2017 at 10:50 AM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> On Fri, Dec 8, 2017 at 10:27 PM, Michael Chan <michael.chan@...adcom.com> wrote:
>> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
>> GRO.  With this flag, we can now independently turn on or off hardware
>> GRO when GRO is on.  Previously, drivers were using NETIF_F_GRO to
>> control hardware GRO and so it cannot be independently turned on or
>> off without affecting GRO.
>>
>> Hardware GRO (just like GRO) guarantees that packets can be re-segmented
>> by TSO/GSO to reconstruct the original packet stream.  It is a subset of
>> NETIF_F_GRO and depends on it, as well as NETIF_F_RXCSUM.
>
> So I would disagree with it being a subset of NETIF_F_GRO. If anything
> it is an alternative to NETIF_F_GRO. It is performing GRO much earlier
> at the device level in the case of hardware drivers. My concern is
> this is probably going to end up applying to things other than just
> hardware drivers though. For example what is to prevent this from
> being applied to something like a virtio/tap interface? It seems like
> this should be something that would be easy to implement in software.

If you do it in software, it's called NETIF_F_GRO.  We already have
it.  The whole point of the new flag is that if the device has
software GRO enabled, and if the device supports GRO_HW, then we can
do a subset of GRO in hardware (hopefully faster).

> In addition as I said in my earlier comments I think we should
> probably look at using this new feature bit to indicate that we allow
> GRO to occur at or below this device as opposed to just above it as
> currently occurs with conventional GRO.
>
>> Since NETIF_F_GRO is not propagated between upper and lower devices,
>> NETIF_F_GRO_HW should follow suit since it is a subset of GRO.  In other
>> words, a lower device can independent have GRO/GRO_HW enabled or disabled
>> and no feature propagation is required.  This will preserve the current
>> GRO behavior.
>
> I'm going to back off on my requirement for you to handle propagation
> since after spending a couple hours working on it I did find it was
> more complex then I originally thought it would be. With that said
> however I would want to see this feature implemented in such a way
> that we can deal with propagating the bits in the future if we need to
> and that is what I am basing my comments on.

Nothing stops anyone from propagating the flag.  Just add
NETIF_F_GRO_HW to NETIF_F_UPPER_DISABLES and it will be propagated
just like LRO.


>> @@ -7424,6 +7424,18 @@ 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 still disagree with this bit. I think GRO is a pure software
> offload, whereas GRO_HW can represent either a software offload of
> some sort occurring in or before the driver, or in the hardware.
> Basically the difference between the two as I view it is where the GRO
> is occurring. I would like to keep that distinction and make use of
> it. As I mentioned before in the case of bonding we currently have no
> way to disable GRO on the lower devices partially because GRO is a
> pure software feature and always happens at each device along the way.
> The nice thing about this new bit is the assumption is that it is
> pushing GRO to the lowest possible level and not triggering any side
> effects like GRO currently does. I hope to use that logic with stacked
> devices so that we could clear the bit and have it disable GRO,
> GRO_HW, and LRO on all devices below the device that cleared it.
>
> I think this linking of GRO and GRO_HW is something that would be
> better served by moving it into the driver if you are wanting to
> maintain the behavior of how this was previously linked to GRO.

If you insist, I can move this to the driver's ndo_fix_features().
But I feel it is much better to enforce this dependency system wide.
Once again, GRO_HW is hardware accelerated GRO and should depend on
it.

> It
> also makes it so that it is much easier to compare the performance for
> GRO_HW against just a pure software GRO since you could then enable
> them independently. Software GRO can come at a cost, and leaving it
> enabled when you want to do it all in hardware is just adding a
> penalty of sorts since I know for many of my routing tests I normally
> disable GRO as it has a significant per-packet cost for small packet
> workloads.
>
>> +               if (!(features & NETIF_F_RXCSUM)) {
>> +                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
>> +                       features &= ~NETIF_F_GRO_HW;
>> +               }
>
> So I was thinking about this. For LRO it makes sense to disable it in
> the case of RXCSUM being disabled since most implementations leave the
> Rx checksum mangled. However for GRO I am not sure it makes complete
> sense. For software GRO we perform checksum validation in either
> tcp4_gro_receive or tcp6_gro_receive. Why should the hardware
> implementation behave differently? When a GRO frame is assembled the
> checksum is converted to CHECKSUM_PARTIAL anyway even if Rx checksum
> validation is disabled for the driver.

This is a logical feature dependency that Yuval Mintz suggested.  For
GRO_HW to work, hardware must verify the checksum of a packet before
the packet can be merged.

So if the user does not want to do RXCSUM on this device for whatever
reason, it logically means that he also doesn't want to do GRO_HW with
implied RXCSUM performed on each packet that is merged.

So I agree with Yuval that this dependency makes sense.

Powered by blists - more mailing lists