[PATCH net-next 0/4] Introduce NETIF_F_GRO_HW

[PATCH net-next 0/4] Introduce NETIF_F_GRO_HW

[Michael Chan]

Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
hardware GRO to use the flag.

Michael Chan (4):
  net: Introduce NETIF_F_GRO_HW
  bnxt_en: Use NETIF_F_GRO_HW.
  bnx2x: Use NETIF_F_GRO_HW.
  qede: Use NETIF_F_GRO_HW.

 Documentation/networking/netdev-features.txt     |  7 +++++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 10 +++-------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  4 +++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.c        |  8 +++++---
 drivers/net/ethernet/qlogic/qede/qede_filter.c   |  9 ++-------
 drivers/net/ethernet/qlogic/qede/qede_main.c     |  4 ++--
 include/linux/netdev_features.h                  |  5 ++++-
 net/core/dev.c                                   | 13 +++++++++++++
 net/core/ethtool.c                               |  1 +
 9 files changed, 40 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Michael Chan]

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.  Hardware GRO guarantees that packets can be
re-segmented by TSO/GSO to reconstruct the original packet stream.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/netdev-features.txt |  7 +++++++
 include/linux/netdev_features.h              |  5 ++++-
 net/core/dev.c                               | 13 +++++++++++++
 net/core/ethtool.c                           |  1 +
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
index 7413eb0..d76d332 100644
--- a/Documentation/networking/netdev-features.txt
+++ b/Documentation/networking/netdev-features.txt
@@ -163,3 +163,10 @@ This requests that the NIC receive all possible frames, including errored
 frames (such as bad FCS, etc).  This can be helpful when sniffing a link with
 bad packets on it.  Some NICs may receive more packets if also put into normal
 PROMISC mode.
+
+*  rx-gro-hw
+
+This requests that the NIC enables Hardware GRO (generic receive offload).
+Hardware GRO is basically the exact reverse of TSO, and is generally
+stricter than Hardware LRO.  A packet stream merged by Hardware GRO must
+be re-segmentable by GSO or TSO back to the exact packet stream.
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index dc8b489..d18ef6f 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -77,6 +77,8 @@ enum {
 	NETIF_F_HW_ESP_TX_CSUM_BIT,	/* ESP with TX checksum offload */
 	NETIF_F_RX_UDP_TUNNEL_PORT_BIT, /* Offload of RX port for UDP tunnels */
 
+	NETIF_F_GRO_HW_BIT,		/* Hardware Generic receive offload */
+
 	/*
 	 * Add your fresh new feature above and remember to update
 	 * netdev_features_strings[] in net/core/ethtool.c and maybe
@@ -96,6 +98,7 @@ enum {
 #define NETIF_F_FRAGLIST	__NETIF_F(FRAGLIST)
 #define NETIF_F_FSO		__NETIF_F(FSO)
 #define NETIF_F_GRO		__NETIF_F(GRO)
+#define NETIF_F_GRO_HW		__NETIF_F(GRO_HW)
 #define NETIF_F_GSO		__NETIF_F(GSO)
 #define NETIF_F_GSO_ROBUST	__NETIF_F(GSO_ROBUST)
 #define NETIF_F_HIGHDMA		__NETIF_F(HIGHDMA)
@@ -193,7 +196,7 @@ enum {
  * If upper/master device has these features disabled, they must be disabled
  * on all lower/slave devices as well.
  */
-#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
+#define NETIF_F_UPPER_DISABLES	(NETIF_F_LRO | NETIF_F_GRO_HW)
 
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
diff --git a/net/core/dev.c b/net/core/dev.c
index 30b5fe3..09c2ad0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7392,6 +7392,19 @@ 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. */
+		if (!(features & NETIF_F_GRO)) {
+			netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO 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;
+		}
+	}
+
 	return features;
 }
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f8fcf45..50a7920 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -73,6 +73,7 @@ int ethtool_op_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 	[NETIF_F_LLTX_BIT] =             "tx-lockless",
 	[NETIF_F_NETNS_LOCAL_BIT] =      "netns-local",
 	[NETIF_F_GRO_BIT] =              "rx-gro",
+	[NETIF_F_GRO_HW_BIT] =           "rx-gro-hw",
 	[NETIF_F_LRO_BIT] =              "rx-lro",
 
 	[NETIF_F_TSO_BIT] =              "tx-tcp-segmentation",
-- 
1.8.3.1

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Or Gerlitz]

On Mon, Dec 4, 2017 at 1:12 PM, Michael Chan <michael.chan@broadcom.com> wrote:
> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware GRO.  W

cool

> --- a/Documentation/networking/netdev-features.txt
> +++ b/Documentation/networking/netdev-features.txt
> @@ -163,3 +163,10 @@ This requests that the NIC receive all possible frames, including errored
>  frames (such as bad FCS, etc).  This can be helpful when sniffing a link with
>  bad packets on it.  Some NICs may receive more packets if also put into normal
>  PROMISC mode.
> +
> +*  rx-gro-hw
> +
> +This requests that the NIC enables Hardware GRO (generic receive offload).
> +Hardware GRO is basically the exact reverse of TSO, and is generally
> +stricter than Hardware LRO.  A packet stream merged by Hardware GRO must
> +be re-segmentable by GSO or TSO back to the exact packet stream.

Can you please add here a 1-2 liner with the grocery store list of requirements
for that matter or pointer to another kernel doc spot where this is defined.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
> re-segmented by TSO/GSO to reconstruct the original packet stream.
>
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Do we really need yet another feature bit for this? We already have
LRO and GRO and now we have to add something that isn't quite either
one?

I think I would rather have something like a netdev private flag that
says LRO assembled frames are routable and just have this all run over
the LRO flag with a test for the private flag to avoid triggering the
LRO disable in the case of the flag being present. Really this is just
a clean LRO implementation anyway so maybe we should just go that
route where LRO is the hardware offload and GRO is the generic
software implementation of that offload. That way when GRO gets some
new feature that your hardware doesn't support we don't have to argue
about the differences since LRO is meant to be a limited
implementation anyway due to the nature of it being in hardware.

To me it just seems like this is an attempt to use the GRO name as a
marketing term and I really don't like the feel of it.

> ---
>  Documentation/networking/netdev-features.txt |  7 +++++++
>  include/linux/netdev_features.h              |  5 ++++-
>  net/core/dev.c                               | 13 +++++++++++++
>  net/core/ethtool.c                           |  1 +
>  4 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
> index 7413eb0..d76d332 100644
> --- a/Documentation/networking/netdev-features.txt
> +++ b/Documentation/networking/netdev-features.txt
> @@ -163,3 +163,10 @@ This requests that the NIC receive all possible frames, including errored
>  frames (such as bad FCS, etc).  This can be helpful when sniffing a link with
>  bad packets on it.  Some NICs may receive more packets if also put into normal
>  PROMISC mode.
> +
> +*  rx-gro-hw
> +
> +This requests that the NIC enables Hardware GRO (generic receive offload).
> +Hardware GRO is basically the exact reverse of TSO, and is generally
> +stricter than Hardware LRO.  A packet stream merged by Hardware GRO must
> +be re-segmentable by GSO or TSO back to the exact packet stream.
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index dc8b489..d18ef6f 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -77,6 +77,8 @@ enum {
>         NETIF_F_HW_ESP_TX_CSUM_BIT,     /* ESP with TX checksum offload */
>         NETIF_F_RX_UDP_TUNNEL_PORT_BIT, /* Offload of RX port for UDP tunnels */
>
> +       NETIF_F_GRO_HW_BIT,             /* Hardware Generic receive offload */
> +
>         /*
>          * Add your fresh new feature above and remember to update
>          * netdev_features_strings[] in net/core/ethtool.c and maybe
> @@ -96,6 +98,7 @@ enum {
>  #define NETIF_F_FRAGLIST       __NETIF_F(FRAGLIST)
>  #define NETIF_F_FSO            __NETIF_F(FSO)
>  #define NETIF_F_GRO            __NETIF_F(GRO)
> +#define NETIF_F_GRO_HW         __NETIF_F(GRO_HW)
>  #define NETIF_F_GSO            __NETIF_F(GSO)
>  #define NETIF_F_GSO_ROBUST     __NETIF_F(GSO_ROBUST)
>  #define NETIF_F_HIGHDMA                __NETIF_F(HIGHDMA)
> @@ -193,7 +196,7 @@ enum {
>   * If upper/master device has these features disabled, they must be disabled
>   * on all lower/slave devices as well.
>   */
> -#define NETIF_F_UPPER_DISABLES NETIF_F_LRO
> +#define NETIF_F_UPPER_DISABLES (NETIF_F_LRO | NETIF_F_GRO_HW)
>
>  /* changeable features with no special hardware requirements */
>  #define NETIF_F_SOFT_FEATURES  (NETIF_F_GSO | NETIF_F_GRO)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 30b5fe3..09c2ad0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7392,6 +7392,19 @@ 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. */
> +               if (!(features & NETIF_F_GRO)) {
> +                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO 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;
> +               }
> +       }
> +
>         return features;
>  }
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f8fcf45..50a7920 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -73,6 +73,7 @@ int ethtool_op_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
>         [NETIF_F_LLTX_BIT] =             "tx-lockless",
>         [NETIF_F_NETNS_LOCAL_BIT] =      "netns-local",
>         [NETIF_F_GRO_BIT] =              "rx-gro",
> +       [NETIF_F_GRO_HW_BIT] =           "rx-gro-hw",
>         [NETIF_F_LRO_BIT] =              "rx-lro",
>
>         [NETIF_F_TSO_BIT] =              "tx-tcp-segmentation",
> --
> 1.8.3.1
>

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Michael Chan]

On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>
>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>> Cc: everest-linux-l2@cavium.com
>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>
> Do we really need yet another feature bit for this? We already have
> LRO and GRO and now we have to add something that isn't quite either
> one?

I think so, to be consistent with TSO/GSO on the transmit side.  On
the receive side, we have LRO/GRO_HW/GRO.  There is difference between
LRO/GRO_HW that we need to distinguish between the 2.

>
> I think I would rather have something like a netdev private flag that
> says LRO assembled frames are routable and just have this all run over
> the LRO flag with a test for the private flag to avoid triggering the
> LRO disable in the case of the flag being present. Really this is just
> a clean LRO implementation anyway so maybe we should just go that
> route where LRO is the hardware offload and GRO is the generic
> software implementation of that offload. That way when GRO gets some
> new feature that your hardware doesn't support we don't have to argue
> about the differences since LRO is meant to be a limited
> implementation anyway due to the nature of it being in hardware.

Private flag will work.  But a standard feature flag is better since
there are multiple drivers supporting this.  A standard way to turn
this on/off is a better user experience.  It's also consistent with
TSO/GSO on the transmit side.

>
> To me it just seems like this is an attempt to use the GRO name as a
> marketing term and I really don't like the feel of it.
>

I disagree with this.  It's more than a marketing term.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>
>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>> Cc: everest-linux-l2@cavium.com
>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>
>> Do we really need yet another feature bit for this? We already have
>> LRO and GRO and now we have to add something that isn't quite either
>> one?
>
> I think so, to be consistent with TSO/GSO on the transmit side.  On
> the receive side, we have LRO/GRO_HW/GRO.  There is difference between
> LRO/GRO_HW that we need to distinguish between the 2.

I don't really see the difference. Your GRO_HW likely doens't do all
of the stuff GRO can do. Neither does LRO. Both occur in the hardware
normally. It would make sense to reuse the LRO flag for this instead
of coming up with a new feature flag that makes things confusing by
saying you are doing a software offload in hardware.

I view LRO as a subset of what GRO can handle, that is performed in
hardware. From the stack perspective the only thing that really
matters is that the frames can be segmented back into what they were
before they were assembled. That is why I think it would be better to
add a flag indicating that the LRO is reversible instead of adding yet
another feature bit that the user has to toggle. That way if at some
point in the future an issue is found where your "GRO in hardware"
feature has a bug that isn't reversible it is just a matter of
clearing the privage flag bit and the mechanisms already in place for
dealing with assembly and routing can take over.

>>
>> I think I would rather have something like a netdev private flag that
>> says LRO assembled frames are routable and just have this all run over
>> the LRO flag with a test for the private flag to avoid triggering the
>> LRO disable in the case of the flag being present. Really this is just
>> a clean LRO implementation anyway so maybe we should just go that
>> route where LRO is the hardware offload and GRO is the generic
>> software implementation of that offload. That way when GRO gets some
>> new feature that your hardware doesn't support we don't have to argue
>> about the differences since LRO is meant to be a limited
>> implementation anyway due to the nature of it being in hardware.
>
> Private flag will work.  But a standard feature flag is better since
> there are multiple drivers supporting this.  A standard way to turn
> this on/off is a better user experience.  It's also consistent with
> TSO/GSO on the transmit side.

I agree, and that is why I would prefer to see this use the LRO flag.
It is the flag that is normally used to indicating Rx coalescing in
hardware. Coming up with a custom feature flag for a form of LRO that
your hardware does doesn't make much sense to me. Otherwise I might as
well go modify ixgbe and rename the LRO it does to GRO_HW since I can
make it do most of what you are doing here.

>>
>> To me it just seems like this is an attempt to use the GRO name as a
>> marketing term and I really don't like the feel of it.
>>
>
> I disagree with this.  It's more than a marketing term.

Not really. It is a subset of GRO offload in hardware. In my mind that
is just LRO. I say subset since odds are you don't support all of the
same protocols and tunnels that GRO in software does.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[David Miller]

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Mon, 4 Dec 2017 10:43:58 -0800

> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>
>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>> Cc: everest-linux-l2@cavium.com
>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>
>>> Do we really need yet another feature bit for this? We already have
>>> LRO and GRO and now we have to add something that isn't quite either
>>> one?
>>
>> I think so, to be consistent with TSO/GSO on the transmit side.  On
>> the receive side, we have LRO/GRO_HW/GRO.  There is difference between
>> LRO/GRO_HW that we need to distinguish between the 2.
> 
> I don't really see the difference. Your GRO_HW likely doens't do all
> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
> normally. It would make sense to reuse the LRO flag for this instead
> of coming up with a new feature flag that makes things confusing by
> saying you are doing a software offload in hardware.
> 
> I view LRO as a subset of what GRO can handle, that is performed in
> hardware. From the stack perspective the only thing that really
> matters is that the frames can be segmented back into what they were
> before they were assembled. That is why I think it would be better to
> add a flag indicating that the LRO is reversible instead of adding yet
> another feature bit that the user has to toggle. That way if at some
> point in the future an issue is found where your "GRO in hardware"
> feature has a bug that isn't reversible it is just a matter of
> clearing the privage flag bit and the mechanisms already in place for
> dealing with assembly and routing can take over.

I don't think they should use the LRO flag.

If their HW GRO stream is fully reversible, which it is, then it's not
LRO.

LRO gets disabled when bridging or routing is enabled, and HW GRO
should not take this penalty.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Mon, Dec 4, 2017 at 10:59 AM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Date: Mon, 4 Dec 2017 10:43:58 -0800
>
>> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
>>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>>
>>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>>> Cc: everest-linux-l2@cavium.com
>>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>>
>>>> Do we really need yet another feature bit for this? We already have
>>>> LRO and GRO and now we have to add something that isn't quite either
>>>> one?
>>>
>>> I think so, to be consistent with TSO/GSO on the transmit side.  On
>>> the receive side, we have LRO/GRO_HW/GRO.  There is difference between
>>> LRO/GRO_HW that we need to distinguish between the 2.
>>
>> I don't really see the difference. Your GRO_HW likely doens't do all
>> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
>> normally. It would make sense to reuse the LRO flag for this instead
>> of coming up with a new feature flag that makes things confusing by
>> saying you are doing a software offload in hardware.
>>
>> I view LRO as a subset of what GRO can handle, that is performed in
>> hardware. From the stack perspective the only thing that really
>> matters is that the frames can be segmented back into what they were
>> before they were assembled. That is why I think it would be better to
>> add a flag indicating that the LRO is reversible instead of adding yet
>> another feature bit that the user has to toggle. That way if at some
>> point in the future an issue is found where your "GRO in hardware"
>> feature has a bug that isn't reversible it is just a matter of
>> clearing the privage flag bit and the mechanisms already in place for
>> dealing with assembly and routing can take over.
>
> I don't think they should use the LRO flag.
>
> If their HW GRO stream is fully reversible, which it is, then it's not
> LRO.

I get all that. I was just hoping that we could phase out the old LRO
over time and push it more towards a GRO friendly implementation. It
is going to be annoying when a marketing person gets a hold of the
term because what you are going to end up with is people pushing for
LRO implementations to be tweaked to work like GRO just so they can
use the feature flag.

> LRO gets disabled when bridging or routing is enabled, and HW GRO
> should not take this penalty.

That is why I suggested a private flag indicating that LRO on this
device is reversible. Basically if it is reversible then we don't need
to disable it when routing/bridging.

If we insist on using the name GRO for this I would prefer GRO_PARTIAL
instead of GRO_HW. It would help to keep things symmetric as we have
been saying since GSO_PARTIAL is a partial implementation of GSO in
the hardware and this would be a partial implementation of GRO in the
hardware as it would be missing some protocols and functionality.

In addition if anything can be done so that the hardware GRO
implementation doesn't prevent software GRO from aggregating things
even larger if possible that would be preferred.

Anyway that is just my $.02 on this.

- Alex

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Eric Dumazet]

On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote:
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Date: Mon, 4 Dec 2017 10:43:58 -0800
> 
> > On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadco
> m.com> wrote:
> >> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
> >> <alexander.duyck@gmail.com> wrote:
> >>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadc
> om.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.  Hardware GRO guarantees that packets can be
> >>>> re-segmented by TSO/GSO to reconstruct the original packet
> stream.
> >>>>
> >>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> >>>> Cc: everest-linux-l2@cavium.com
> >>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >>>
> >>> Do we really need yet another feature bit for this? We already
> have
> >>> LRO and GRO and now we have to add something that isn't quite
> either
> >>> one?
> >>
> >> I think so, to be consistent with TSO/GSO on the transmit side. 
> On
> >> the receive side, we have LRO/GRO_HW/GRO.  There is difference
> between
> >> LRO/GRO_HW that we need to distinguish between the 2.
> > 
> > I don't really see the difference. Your GRO_HW likely doens't do
> all
> > of the stuff GRO can do. Neither does LRO. Both occur in the
> hardware
> > normally. It would make sense to reuse the LRO flag for this
> instead
> > of coming up with a new feature flag that makes things confusing by
> > saying you are doing a software offload in hardware.
> > 
> > I view LRO as a subset of what GRO can handle, that is performed in
> > hardware. From the stack perspective the only thing that really
> > matters is that the frames can be segmented back into what they
> were
> > before they were assembled. That is why I think it would be better
> to
> > add a flag indicating that the LRO is reversible instead of adding
> yet
> > another feature bit that the user has to toggle. That way if at
> some
> > point in the future an issue is found where your "GRO in hardware"
> > feature has a bug that isn't reversible it is just a matter of
> > clearing the privage flag bit and the mechanisms already in place
> for
> > dealing with assembly and routing can take over.
> 
> I don't think they should use the LRO flag.
> 
> If their HW GRO stream is fully reversible, which it is, then it's
> not
> LRO.
> 
> LRO gets disabled when bridging or routing is enabled, and HW GRO
> should not take this penalty.

Also having separate flags means that one can decide to disable HW GRO
and enable (linux) GRO if he wants to. Or the opposite.

I definitely like this idea.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Mon, Dec 4, 2017 at 11:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote:
>> From: Alexander Duyck <alexander.duyck@gmail.com>
>> Date: Mon, 4 Dec 2017 10:43:58 -0800
>>
>> > On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadco
>> m.com> wrote:
>> >> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>> >> <alexander.duyck@gmail.com> wrote:
>> >>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadc
>> om.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.  Hardware GRO guarantees that packets can be
>> >>>> re-segmented by TSO/GSO to reconstruct the original packet
>> stream.
>> >>>>
>> >>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>> >>>> Cc: everest-linux-l2@cavium.com
>> >>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >>>
>> >>> Do we really need yet another feature bit for this? We already
>> have
>> >>> LRO and GRO and now we have to add something that isn't quite
>> either
>> >>> one?
>> >>
>> >> I think so, to be consistent with TSO/GSO on the transmit side.
>> On
>> >> the receive side, we have LRO/GRO_HW/GRO.  There is difference
>> between
>> >> LRO/GRO_HW that we need to distinguish between the 2.
>> >
>> > I don't really see the difference. Your GRO_HW likely doens't do
>> all
>> > of the stuff GRO can do. Neither does LRO. Both occur in the
>> hardware
>> > normally. It would make sense to reuse the LRO flag for this
>> instead
>> > of coming up with a new feature flag that makes things confusing by
>> > saying you are doing a software offload in hardware.
>> >
>> > I view LRO as a subset of what GRO can handle, that is performed in
>> > hardware. From the stack perspective the only thing that really
>> > matters is that the frames can be segmented back into what they
>> were
>> > before they were assembled. That is why I think it would be better
>> to
>> > add a flag indicating that the LRO is reversible instead of adding
>> yet
>> > another feature bit that the user has to toggle. That way if at
>> some
>> > point in the future an issue is found where your "GRO in hardware"
>> > feature has a bug that isn't reversible it is just a matter of
>> > clearing the privage flag bit and the mechanisms already in place
>> for
>> > dealing with assembly and routing can take over.
>>
>> I don't think they should use the LRO flag.
>>
>> If their HW GRO stream is fully reversible, which it is, then it's
>> not
>> LRO.
>>
>> LRO gets disabled when bridging or routing is enabled, and HW GRO
>> should not take this penalty.
>
> Also having separate flags means that one can decide to disable HW GRO
> and enable (linux) GRO if he wants to. Or the opposite.
>
> I definitely like this idea.

Yeah, I get that. More specifically it is the bit where you have to
disable the hardware GRO due to a bug and then that also prevents you
from using the software GRO. The reuse of the GRO flag to represent a
hardware offload in general was a poor decision.

I just don't get why these drivers have to support both LRO or GRO_HW
at the same time. It seems like it is just overkill. I would rather
see them just use the LRO feature to represent hardware aggregation of
frames and then let the stack know if LRO is reversible or not for a
given device.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Michael Chan]

On Mon, Dec 4, 2017 at 10:43 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>
>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>> Cc: everest-linux-l2@cavium.com
>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>
>>> Do we really need yet another feature bit for this? We already have
>>> LRO and GRO and now we have to add something that isn't quite either
>>> one?
>>
>> I think so, to be consistent with TSO/GSO on the transmit side.  On
>> the receive side, we have LRO/GRO_HW/GRO.  There is difference between
>> LRO/GRO_HW that we need to distinguish between the 2.
>
> I don't really see the difference. Your GRO_HW likely doens't do all
> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
> normally. It would make sense to reuse the LRO flag for this instead
> of coming up with a new feature flag that makes things confusing by
> saying you are doing a software offload in hardware.

I agree it is a little confusing, but LRO + private flag is just as
confusing in my opinion.

>
> I view LRO as a subset of what GRO can handle, that is performed in
> hardware.

I don't view LRO as a subset of GRO.  LRO loses information that
cannot be reconstructed back once an LRO frame is aggregated.  LRO is
just different from GRO.  GRO_HW must provide enough information to
reconstruct the original frames.

> From the stack perspective the only thing that really
> matters is that the frames can be segmented back into what they were
> before they were assembled. That is why I think it would be better to
> add a flag indicating that the LRO is reversible instead of adding yet
> another feature bit that the user has to toggle. That way if at some
> point in the future an issue is found where your "GRO in hardware"
> feature has a bug that isn't reversible it is just a matter of
> clearing the privage flag bit and the mechanisms already in place for
> dealing with assembly and routing can take over.

NETIF_F_GRO_HW is a flag that depends on NETIF_F_GRO.  In some ways,
it is similar to a private flag that depends on NETIF_F_LRO.  But I
think a standard flag is better.

>
>>>
>>> I think I would rather have something like a netdev private flag that
>>> says LRO assembled frames are routable and just have this all run over
>>> the LRO flag with a test for the private flag to avoid triggering the
>>> LRO disable in the case of the flag being present. Really this is just
>>> a clean LRO implementation anyway so maybe we should just go that
>>> route where LRO is the hardware offload and GRO is the generic
>>> software implementation of that offload. That way when GRO gets some
>>> new feature that your hardware doesn't support we don't have to argue
>>> about the differences since LRO is meant to be a limited
>>> implementation anyway due to the nature of it being in hardware.
>>
>> Private flag will work.  But a standard feature flag is better since
>> there are multiple drivers supporting this.  A standard way to turn
>> this on/off is a better user experience.  It's also consistent with
>> TSO/GSO on the transmit side.
>
> I agree, and that is why I would prefer to see this use the LRO flag.
> It is the flag that is normally used to indicating Rx coalescing in
> hardware. Coming up with a custom feature flag for a form of LRO that
> your hardware does doesn't make much sense to me. Otherwise I might as
> well go modify ixgbe and rename the LRO it does to GRO_HW since I can
> make it do most of what you are doing here.

Again, there is enough difference between LRO and hardware GRO that it
makes sense to add a new flag.  Functionally, a private flag will work
too, but a standard flag makes more intuitive sense to me and to
users.

Yeah, the idea is that any vendor can use GRO_HW.  Today, there are 3
drivers supporting it.  I'm sure there will be other drivers
supporting this in the future.  For something that is supported by
multiple vendors, that's another reason to use a standard flag.

>
>>>
>>> To me it just seems like this is an attempt to use the GRO name as a
>>> marketing term and I really don't like the feel of it.
>>>
>>
>> I disagree with this.  It's more than a marketing term.
>
> Not really. It is a subset of GRO offload in hardware. In my mind that
> is just LRO. I say subset since odds are you don't support all of the
> same protocols and tunnels that GRO in software does.

Of course, hardware has some limitations, such as the number of TCP
connections it can aggregate, etc.  But again, it is different from
LRO.

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Mon, Dec 4, 2017 at 11:52 AM, Michael Chan <michael.chan@broadcom.com> wrote:
> On Mon, Dec 4, 2017 at 10:43 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.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.  Hardware GRO guarantees that packets can be
>>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>>
>>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>>> Cc: everest-linux-l2@cavium.com
>>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>>
>>>> Do we really need yet another feature bit for this? We already have
>>>> LRO and GRO and now we have to add something that isn't quite either
>>>> one?
>>>
>>> I think so, to be consistent with TSO/GSO on the transmit side.  On
>>> the receive side, we have LRO/GRO_HW/GRO.  There is difference between
>>> LRO/GRO_HW that we need to distinguish between the 2.
>>
>> I don't really see the difference. Your GRO_HW likely doens't do all
>> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
>> normally. It would make sense to reuse the LRO flag for this instead
>> of coming up with a new feature flag that makes things confusing by
>> saying you are doing a software offload in hardware.
>
> I agree it is a little confusing, but LRO + private flag is just as
> confusing in my opinion.
>
>>
>> I view LRO as a subset of what GRO can handle, that is performed in
>> hardware.
>
> I don't view LRO as a subset of GRO.  LRO loses information that
> cannot be reconstructed back once an LRO frame is aggregated.  LRO is
> just different from GRO.  GRO_HW must provide enough information to
> reconstruct the original frames.

I think we can just agree to disagree on this. Defining GRO_HW as
reversible LRO is the way I view it. In my mind LRO is assembly in
hardware, the non-reversible part is an unfortunate side effect of
existing implementations. From the sound of things you and Dave view
the non-reversible part as a core bit of LRO and the hardware/driver
implementation of it is just a secondary thing.

>> From the stack perspective the only thing that really
>> matters is that the frames can be segmented back into what they were
>> before they were assembled. That is why I think it would be better to
>> add a flag indicating that the LRO is reversible instead of adding yet
>> another feature bit that the user has to toggle. That way if at some
>> point in the future an issue is found where your "GRO in hardware"
>> feature has a bug that isn't reversible it is just a matter of
>> clearing the privage flag bit and the mechanisms already in place for
>> dealing with assembly and routing can take over.
>
> NETIF_F_GRO_HW is a flag that depends on NETIF_F_GRO.  In some ways,
> it is similar to a private flag that depends on NETIF_F_LRO.  But I
> think a standard flag is better.

Why would you make it dependent on NETIF_F_GRO? That doesn't make any
sense to me. It gets in the way of GRO anyway as it can't assemble an
already aggregated frame.

It seems more like the two features should be able to co-exist with
either one being able to be disabled/enabled independently. It makes
it much easier to debug things this way. Otherwise there is no way to
tell if a given issue is software or hardware GRO since disabling
software disables them both.

>>
>>>>
>>>> I think I would rather have something like a netdev private flag that
>>>> says LRO assembled frames are routable and just have this all run over
>>>> the LRO flag with a test for the private flag to avoid triggering the
>>>> LRO disable in the case of the flag being present. Really this is just
>>>> a clean LRO implementation anyway so maybe we should just go that
>>>> route where LRO is the hardware offload and GRO is the generic
>>>> software implementation of that offload. That way when GRO gets some
>>>> new feature that your hardware doesn't support we don't have to argue
>>>> about the differences since LRO is meant to be a limited
>>>> implementation anyway due to the nature of it being in hardware.
>>>
>>> Private flag will work.  But a standard feature flag is better since
>>> there are multiple drivers supporting this.  A standard way to turn
>>> this on/off is a better user experience.  It's also consistent with
>>> TSO/GSO on the transmit side.
>>
>> I agree, and that is why I would prefer to see this use the LRO flag.
>> It is the flag that is normally used to indicating Rx coalescing in
>> hardware. Coming up with a custom feature flag for a form of LRO that
>> your hardware does doesn't make much sense to me. Otherwise I might as
>> well go modify ixgbe and rename the LRO it does to GRO_HW since I can
>> make it do most of what you are doing here.
>
> Again, there is enough difference between LRO and hardware GRO that it
> makes sense to add a new flag.  Functionally, a private flag will work
> too, but a standard flag makes more intuitive sense to me and to
> users.
>
> Yeah, the idea is that any vendor can use GRO_HW.  Today, there are 3
> drivers supporting it.  I'm sure there will be other drivers
> supporting this in the future.  For something that is supported by
> multiple vendors, that's another reason to use a standard flag.
>
>>
>>>>
>>>> To me it just seems like this is an attempt to use the GRO name as a
>>>> marketing term and I really don't like the feel of it.
>>>>
>>>
>>> I disagree with this.  It's more than a marketing term.
>>
>> Not really. It is a subset of GRO offload in hardware. In my mind that
>> is just LRO. I say subset since odds are you don't support all of the
>> same protocols and tunnels that GRO in software does.
>
> Of course, hardware has some limitations, such as the number of TCP
> connections it can aggregate, etc.  But again, it is different from
> LRO.

The bit I don't like about this is that if you bond a device that is
running LRO with one that is running GSO_HW you now have to disable
two flags on the bond in order to disable hardware aggregation.

Admittedly I haven't take a look at the entire patch set, but did you
also take care of the flag sychronization issues this is going to
cause with upper devices such as vlan, macvlan, bond, etc?

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Michael Chan]

On Mon, Dec 4, 2017 at 12:58 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 11:52 AM, Michael Chan <michael.chan@broadcom.com> wrote:

>> NETIF_F_GRO_HW is a flag that depends on NETIF_F_GRO.  In some ways,
>> it is similar to a private flag that depends on NETIF_F_LRO.  But I
>> think a standard flag is better.
>
> Why would you make it dependent on NETIF_F_GRO? That doesn't make any
> sense to me. It gets in the way of GRO anyway as it can't assemble an
> already aggregated frame.

GRO_HW is basically hardware accelerated GRO.  If the user doesn't
want GRO at all (let's say he is running generic XDP, or he wants
tcpdump to show the packets on the wire), disabling GRO should disable
everything.  It's more intuitive that way.

>
> It seems more like the two features should be able to co-exist with
> either one being able to be disabled/enabled independently. It makes
> it much easier to debug things this way. Otherwise there is no way to
> tell if a given issue is software or hardware GRO since disabling
> software disables them both.
>
>>>
>>>>>
>>>>> I think I would rather have something like a netdev private flag that
>>>>> says LRO assembled frames are routable and just have this all run over
>>>>> the LRO flag with a test for the private flag to avoid triggering the
>>>>> LRO disable in the case of the flag being present. Really this is just
>>>>> a clean LRO implementation anyway so maybe we should just go that
>>>>> route where LRO is the hardware offload and GRO is the generic
>>>>> software implementation of that offload. That way when GRO gets some
>>>>> new feature that your hardware doesn't support we don't have to argue
>>>>> about the differences since LRO is meant to be a limited
>>>>> implementation anyway due to the nature of it being in hardware.
>>>>
>>>> Private flag will work.  But a standard feature flag is better since
>>>> there are multiple drivers supporting this.  A standard way to turn
>>>> this on/off is a better user experience.  It's also consistent with
>>>> TSO/GSO on the transmit side.
>>>
>>> I agree, and that is why I would prefer to see this use the LRO flag.
>>> It is the flag that is normally used to indicating Rx coalescing in
>>> hardware. Coming up with a custom feature flag for a form of LRO that
>>> your hardware does doesn't make much sense to me. Otherwise I might as
>>> well go modify ixgbe and rename the LRO it does to GRO_HW since I can
>>> make it do most of what you are doing here.
>>
>> Again, there is enough difference between LRO and hardware GRO that it
>> makes sense to add a new flag.  Functionally, a private flag will work
>> too, but a standard flag makes more intuitive sense to me and to
>> users.
>>
>> Yeah, the idea is that any vendor can use GRO_HW.  Today, there are 3
>> drivers supporting it.  I'm sure there will be other drivers
>> supporting this in the future.  For something that is supported by
>> multiple vendors, that's another reason to use a standard flag.
>>
>>>
>>>>>
>>>>> To me it just seems like this is an attempt to use the GRO name as a
>>>>> marketing term and I really don't like the feel of it.
>>>>>
>>>>
>>>> I disagree with this.  It's more than a marketing term.
>>>
>>> Not really. It is a subset of GRO offload in hardware. In my mind that
>>> is just LRO. I say subset since odds are you don't support all of the
>>> same protocols and tunnels that GRO in software does.
>>
>> Of course, hardware has some limitations, such as the number of TCP
>> connections it can aggregate, etc.  But again, it is different from
>> LRO.
>
> The bit I don't like about this is that if you bond a device that is
> running LRO with one that is running GSO_HW you now have to disable
> two flags on the bond in order to disable hardware aggregation.
>
> Admittedly I haven't take a look at the entire patch set, but did you
> also take care of the flag sychronization issues this is going to
> cause with upper devices such as vlan, macvlan, bond, etc?

I will take another look.  Thanks.

RE: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Yuval Mintz]

> 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.  Hardware GRO guarantees that packets can be
> re-segmented by TSO/GSO to reconstruct the original packet stream.
> 
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  Documentation/networking/netdev-features.txt |  7 +++++++
>  include/linux/netdev_features.h              |  5 ++++-
>  net/core/dev.c                               | 13 +++++++++++++
>  net/core/ethtool.c                           |  1 +
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/netdev-features.txt
> b/Documentation/networking/netdev-features.txt
> index 7413eb0..d76d332 100644
> --- a/Documentation/networking/netdev-features.txt
> +++ b/Documentation/networking/netdev-features.txt
> @@ -163,3 +163,10 @@ This requests that the NIC receive all possible
> frames, including errored
>  frames (such as bad FCS, etc).  This can be helpful when sniffing a link with
>  bad packets on it.  Some NICs may receive more packets if also put into
> normal
>  PROMISC mode.
> +
> +*  rx-gro-hw
> +
> +This requests that the NIC enables Hardware GRO (generic receive offload).
> +Hardware GRO is basically the exact reverse of TSO, and is generally
> +stricter than Hardware LRO.  A packet stream merged by Hardware GRO
> must
> +be re-segmentable by GSO or TSO back to the exact packet stream.
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index dc8b489..d18ef6f 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -77,6 +77,8 @@ enum {
>  	NETIF_F_HW_ESP_TX_CSUM_BIT,	/* ESP with TX checksum
> offload */
>  	NETIF_F_RX_UDP_TUNNEL_PORT_BIT, /* Offload of RX port for UDP
> tunnels */
> 
> +	NETIF_F_GRO_HW_BIT,		/* Hardware Generic receive
> offload */
> +
>  	/*
>  	 * Add your fresh new feature above and remember to update
>  	 * netdev_features_strings[] in net/core/ethtool.c and maybe
> @@ -96,6 +98,7 @@ enum {
>  #define NETIF_F_FRAGLIST	__NETIF_F(FRAGLIST)
>  #define NETIF_F_FSO		__NETIF_F(FSO)
>  #define NETIF_F_GRO		__NETIF_F(GRO)
> +#define NETIF_F_GRO_HW		__NETIF_F(GRO_HW)
>  #define NETIF_F_GSO		__NETIF_F(GSO)
>  #define NETIF_F_GSO_ROBUST	__NETIF_F(GSO_ROBUST)
>  #define NETIF_F_HIGHDMA		__NETIF_F(HIGHDMA)
> @@ -193,7 +196,7 @@ enum {
>   * If upper/master device has these features disabled, they must be disabled
>   * on all lower/slave devices as well.
>   */
> -#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
> +#define NETIF_F_UPPER_DISABLES	(NETIF_F_LRO | NETIF_F_GRO_HW)
> 
>  /* changeable features with no special hardware requirements */
>  #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 30b5fe3..09c2ad0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7392,6 +7392,19 @@ 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. */
> +		if (!(features & NETIF_F_GRO)) {

While at it, perhaps also make it dependent on NETIF_F_RXCSUM?

> +			netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
> no GRO 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;

Isn't this considered to be breaking an existing API?
After this, while NETIF_F_GRO_HW is published an application trying to
set NETIF_F_LRO and then query its state would discover it failed
[while previously it could have succeeded, such as for bnx2]
 
While I understand the need to make sure core doesn't enable
two competing aggregation offloads, why make GRO_HW > LRO?
I understand it's probably the better one, but until LRO gets deprecated
isn't it safer to do this limitation the opposite way?
I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?

> +		}
> +	}
> +
>  	return features;
>  }
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f8fcf45..50a7920 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -73,6 +73,7 @@ int ethtool_op_get_ts_info(struct net_device *dev,
> struct ethtool_ts_info *info)
>  	[NETIF_F_LLTX_BIT] =             "tx-lockless",
>  	[NETIF_F_NETNS_LOCAL_BIT] =      "netns-local",
>  	[NETIF_F_GRO_BIT] =              "rx-gro",
> +	[NETIF_F_GRO_HW_BIT] =           "rx-gro-hw",
>  	[NETIF_F_LRO_BIT] =              "rx-lro",
> 
>  	[NETIF_F_TSO_BIT] =              "tx-tcp-segmentation",
> --
> 1.8.3.1

Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Michael Chan]

On Mon, Dec 4, 2017 at 2:15 PM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> @@ -96,6 +98,7 @@ enum {
>>  #define NETIF_F_FRAGLIST     __NETIF_F(FRAGLIST)
>>  #define NETIF_F_FSO          __NETIF_F(FSO)
>>  #define NETIF_F_GRO          __NETIF_F(GRO)
>> +#define NETIF_F_GRO_HW               __NETIF_F(GRO_HW)
>>  #define NETIF_F_GSO          __NETIF_F(GSO)
>>  #define NETIF_F_GSO_ROBUST   __NETIF_F(GSO_ROBUST)
>>  #define NETIF_F_HIGHDMA              __NETIF_F(HIGHDMA)
>> @@ -193,7 +196,7 @@ enum {
>>   * If upper/master device has these features disabled, they must be disabled
>>   * on all lower/slave devices as well.
>>   */
>> -#define NETIF_F_UPPER_DISABLES       NETIF_F_LRO
>> +#define NETIF_F_UPPER_DISABLES       (NETIF_F_LRO | NETIF_F_GRO_HW)
>>
>>  /* changeable features with no special hardware requirements */
>>  #define NETIF_F_SOFT_FEATURES        (NETIF_F_GSO | NETIF_F_GRO)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 30b5fe3..09c2ad0 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -7392,6 +7392,19 @@ 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. */
>> +             if (!(features & NETIF_F_GRO)) {
>
> While at it, perhaps also make it dependent on NETIF_F_RXCSUM?

OK.  Makes sense.

>
>> +                     netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
>> no GRO 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;
>
> Isn't this considered to be breaking an existing API?
> After this, while NETIF_F_GRO_HW is published an application trying to
> set NETIF_F_LRO and then query its state would discover it failed
> [while previously it could have succeeded, such as for bnx2]
>
> While I understand the need to make sure core doesn't enable
> two competing aggregation offloads, why make GRO_HW > LRO?
> I understand it's probably the better one, but until LRO gets deprecated
> isn't it safer to do this limitation the opposite way?
> I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?

I am just following precedents in the netdev_fix_features() logic to
drop incompatible features.  I can make LRO and GRO_HW have equal
standing by dropping the other when one is set.  So if I do that, user
will be able to always enable LRO.  The code will just drop GRO_HW if
it is set.

[PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Michael Chan]

Advertise NETIF_F_GRO_HW if hardware GRO is supported.  Turn on or off
hardware GRO based on NETIF_F_GRO_HW.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 33c49ad..5d15cdb 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -2751,7 +2751,7 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 		return;
 	if (bp->dev->features & NETIF_F_LRO)
 		bp->flags |= BNXT_FLAG_LRO;
-	if (bp->dev->features & NETIF_F_GRO)
+	if (bp->dev->features & NETIF_F_GRO_HW)
 		bp->flags |= BNXT_FLAG_GRO;
 }
 
@@ -6817,7 +6817,7 @@ static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 	bool update_tpa = false;
 
 	flags &= ~BNXT_FLAG_ALL_CONFIG_FEATS;
-	if ((features & NETIF_F_GRO) && !BNXT_CHIP_TYPE_NITRO_A0(bp))
+	if (features & NETIF_F_GRO_HW)
 		flags |= BNXT_FLAG_GRO;
 	if (features & NETIF_F_LRO)
 		flags |= BNXT_FLAG_LRO;
@@ -8093,7 +8093,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 			   NETIF_F_RXCSUM | NETIF_F_GRO;
 
 	if (!BNXT_CHIP_TYPE_NITRO_A0(bp))
-		dev->hw_features |= NETIF_F_LRO;
+		dev->hw_features |= NETIF_F_LRO | NETIF_F_GRO_HW;
 
 	dev->hw_enc_features =
 			NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_SG |
@@ -8107,6 +8107,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX |
 			    NETIF_F_HW_VLAN_STAG_RX | NETIF_F_HW_VLAN_STAG_TX;
 	dev->features |= dev->hw_features | NETIF_F_HIGHDMA;
+	if (dev->features & NETIF_F_GRO_HW)
+		dev->features &= ~NETIF_F_LRO;
 	dev->priv_flags |= IFF_UNICAST_FLT;
 
 #ifdef CONFIG_BNXT_SRIOV
-- 
1.8.3.1

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Or Gerlitz]

On Mon, Dec 4, 2017 at 1:12 PM, Michael Chan <michael.chan@broadcom.com> wrote:
> Advertise NETIF_F_GRO_HW if hardware GRO is supported.  Turn on or off
> hardware GRO based on NETIF_F_GRO_HW.

So the patch only deals with advertisement biz.. and I even failed to
find a place where you arm
something in the FW... so this just works OOB, all your buffers && skb
logic && FW setup are fully
ready for that? also, nothing you need to put differently on the SKB
for HW GRO completions?
can you explain this little further or it's all in your netdev slides...

Or.

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Michael Chan]

On Mon, Dec 4, 2017 at 8:35 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 1:12 PM, Michael Chan <michael.chan@broadcom.com> wrote:
>> Advertise NETIF_F_GRO_HW if hardware GRO is supported.  Turn on or off
>> hardware GRO based on NETIF_F_GRO_HW.
>
> So the patch only deals with advertisement biz.. and I even failed to
> find a place where you arm
> something in the FW... so this just works OOB, all your buffers && skb
> logic && FW setup are fully
> ready for that? also, nothing you need to put differently on the SKB
> for HW GRO completions?
> can you explain this little further or it's all in your netdev slides...
>

All the logic is already in these 3 drivers in the tree.  You can see
the additional logic in any of these drivers.  It's just that these
drivers have been using NETIF_F_GRO to turn on this mode in
hardware/firmware.  So these patches are just to switch over to use
the new flag to turn on this mode so that it can be independently
turned on or off.

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Or Gerlitz]

On Mon, Dec 4, 2017 at 8:11 PM, Michael Chan <michael.chan@broadcom.com> wrote:

> All the logic is already in these 3 drivers in the tree.  You can see
> the additional logic in any of these drivers.  It's just that these
> drivers have been using NETIF_F_GRO to turn on this mode in
> hardware/firmware.

What happens for tcp encaped by some udp tunnel, does
the HW know how to GRO that?

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Eric Dumazet]

On Mon, 2017-12-04 at 23:06 +0200, Or Gerlitz wrote:
> On Mon, Dec 4, 2017 at 8:11 PM, Michael Chan <michael.chan@broadcom.c
> om> wrote:
> 
> > All the logic is already in these 3 drivers in the tree.  You can
> > see
> > the additional logic in any of these drivers.  It's just that these
> > drivers have been using NETIF_F_GRO to turn on this mode in
> > hardware/firmware.
> 
> What happens for tcp encaped by some udp tunnel, does
> the HW know how to GRO that?

Yes it does, at least for common encaps (GRE, SIT, IPIP)

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Michael Chan]

On Mon, Dec 4, 2017 at 2:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2017-12-04 at 23:06 +0200, Or Gerlitz wrote:
>> On Mon, Dec 4, 2017 at 8:11 PM, Michael Chan <michael.chan@broadcom.c
>> om> wrote:
>>
>> > All the logic is already in these 3 drivers in the tree.  You can
>> > see
>> > the additional logic in any of these drivers.  It's just that these
>> > drivers have been using NETIF_F_GRO to turn on this mode in
>> > hardware/firmware.
>>
>> What happens for tcp encaped by some udp tunnel, does
>> the HW know how to GRO that?
>
> Yes it does, at least for common encaps (GRE, SIT, IPIP)
>

Correct, bnxt_en supports all common encaps.  Basically, if we can do
TSO on the packet, we can do the reverse GRO_HW on the same packet
stream.

As already pointed out, GRO_HW is a subset of GRO.  Packets that
cannot be aggregated in hardware (due to hardware resource limitations
or protocol types that it doesn't handle) can just be passed to the
stack for GRO aggregation.

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Marcelo Ricardo Leitner]

On Mon, Dec 04, 2017 at 04:07:15PM -0800, Michael Chan wrote:
> As already pointed out, GRO_HW is a subset of GRO.  Packets that
> cannot be aggregated in hardware (due to hardware resource limitations
> or protocol types that it doesn't handle) can just be passed to the
> stack for GRO aggregation.

How would the parameters/limits work in this case? I mean, currently
we have the default weight of 64 packets per napi poll cycle, the
budget of 300 per cycle and also the time constrain,
net.core.netdev_budget_usecs.

With GRO_HW, this 64 limit may be exceeded. I'm looking at qede code
and it works by couting each completion as 1 rcv_pkts
(qede_fp.c:1318). So if it now gets 64 packets, it's up to 64*MTU
aprox, GRO'ed or not. But with GRO_HW, seems it may be much more than
that and which may not be fair with other interfaces in the system.
Drivers supporting GRO_HW probably should account for this.

And how can one control how much time a packet may spend on NIC queue
waiting to be GRO'ed? Does it use the coalescing parameters for that?

  Marcelo

Re: [PATCH net-next 2/4] bnxt_en: Use NETIF_F_GRO_HW.

[Michael Chan]

On Tue, Dec 5, 2017 at 10:10 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Mon, Dec 04, 2017 at 04:07:15PM -0800, Michael Chan wrote:
>> As already pointed out, GRO_HW is a subset of GRO.  Packets that
>> cannot be aggregated in hardware (due to hardware resource limitations
>> or protocol types that it doesn't handle) can just be passed to the
>> stack for GRO aggregation.
>
> How would the parameters/limits work in this case? I mean, currently
> we have the default weight of 64 packets per napi poll cycle, the
> budget of 300 per cycle and also the time constrain,
> net.core.netdev_budget_usecs.

Good point.  Currently, it is no different than LRO.  Each aggregated
packet is counted as 1.  With LRO, you don't necessarily know many
packets were merged.  With GRO_HW, we know and it's possible to count
the original packets towards the NAPI budget.

>
> With GRO_HW, this 64 limit may be exceeded. I'm looking at qede code
> and it works by couting each completion as 1 rcv_pkts
> (qede_fp.c:1318). So if it now gets 64 packets, it's up to 64*MTU
> aprox, GRO'ed or not. But with GRO_HW, seems it may be much more than
> that and which may not be fair with other interfaces in the system.
> Drivers supporting GRO_HW probably should account for this.

Right.  We can make this adjustment for GRO_HW in a future patchset.

>
> And how can one control how much time a packet may spend on NIC queue
> waiting to be GRO'ed? Does it use the coalescing parameters for that?
>

The GRO_HW timer is currently not exposed.  It's different from
interrupt coalescing.  It's possible to make this a tunable parameter
in the future.

[PATCH net-next 3/4] bnx2x: Use NETIF_F_GRO_HW.

[Michael Chan]

Advertise NETIF_F_GRO_HW and turn on TPA_MODE_GRO when NETIF_F_GRO_HW
is set.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 10 +++-------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  4 +++-
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 4c739d5..79371fb 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -2482,7 +2482,7 @@ static void bnx2x_bz_fp(struct bnx2x *bp, int index)
 	 */
 	if (bp->dev->features & NETIF_F_LRO)
 		fp->mode = TPA_MODE_LRO;
-	else if (bp->dev->features & NETIF_F_GRO &&
+	else if (bp->dev->features & NETIF_F_GRO_HW &&
 		 bnx2x_mtu_allows_gro(bp->dev->mtu))
 		fp->mode = TPA_MODE_GRO;
 	else
@@ -4933,13 +4933,9 @@ int bnx2x_set_features(struct net_device *dev, netdev_features_t features)
 		}
 	}
 
-	/* if GRO is changed while LRO is enabled, don't force a reload */
-	if ((changes & NETIF_F_GRO) && (features & NETIF_F_LRO))
-		changes &= ~NETIF_F_GRO;
-
 	/* if GRO is changed while HW TPA is off, don't force a reload */
-	if ((changes & NETIF_F_GRO) && bp->disable_tpa)
-		changes &= ~NETIF_F_GRO;
+	if ((changes & NETIF_F_GRO_HW) && bp->disable_tpa)
+		changes &= ~NETIF_F_GRO_HW;
 
 	if (changes)
 		bnx2x_reload = true;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 54d15713..92d54b4 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -13266,7 +13266,7 @@ static int bnx2x_init_dev(struct bnx2x *bp, struct pci_dev *pdev,
 
 	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
-		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_GRO |
+		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_GRO | NETIF_F_GRO_HW |
 		NETIF_F_RXHASH | NETIF_F_HW_VLAN_CTAG_TX;
 	if (!chip_is_e1x) {
 		dev->hw_features |= NETIF_F_GSO_GRE | NETIF_F_GSO_GRE_CSUM |
@@ -13302,6 +13302,8 @@ static int bnx2x_init_dev(struct bnx2x *bp, struct pci_dev *pdev,
 
 	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_CTAG_RX;
 	dev->features |= NETIF_F_HIGHDMA;
+	if (dev->features & NETIF_F_GRO_HW)
+		dev->features &= ~NETIF_F_LRO;
 
 	/* Add Loopback capability to the device */
 	dev->hw_features |= NETIF_F_LOOPBACK;
-- 
1.8.3.1

[PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.

[Michael Chan]

Advertise NETIF_F_GRO_HW and turn on or off hardware GRO based on
NETIF_F_GRO_FW flag.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/qlogic/qede/qede_filter.c | 9 ++-------
 drivers/net/ethernet/qlogic/qede/qede_main.c   | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index c1a0708..7ee49b4 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -901,13 +901,8 @@ int qede_set_features(struct net_device *dev, netdev_features_t features)
 	netdev_features_t changes = features ^ dev->features;
 	bool need_reload = false;
 
-	/* No action needed if hardware GRO is disabled during driver load */
-	if (changes & NETIF_F_GRO) {
-		if (dev->features & NETIF_F_GRO)
-			need_reload = !edev->gro_disable;
-		else
-			need_reload = edev->gro_disable;
-	}
+	if (changes & NETIF_F_GRO_HW)
+		need_reload = true;
 
 	if (need_reload) {
 		struct qede_reload_args args;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 8f9b3eb..b81620e 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -676,7 +676,7 @@ static void qede_init_ndev(struct qede_dev *edev)
 	ndev->priv_flags |= IFF_UNICAST_FLT;
 
 	/* user-changeble features */
-	hw_features = NETIF_F_GRO | NETIF_F_SG |
+	hw_features = NETIF_F_GRO | NETIF_F_GRO_HW | NETIF_F_SG |
 		      NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		      NETIF_F_TSO | NETIF_F_TSO6;
 
@@ -1515,7 +1515,7 @@ static void qede_init_fp(struct qede_dev *edev)
 			 edev->ndev->name, queue_id);
 	}
 
-	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO);
+	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO_HW);
 }
 
 static int qede_set_real_num_queues(struct qede_dev *edev)
-- 
1.8.3.1

RE: [PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.

[Yuval Mintz]

> Advertise NETIF_F_GRO_HW and turn on or off hardware GRO based on
> NETIF_F_GRO_FW flag.
> 
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/qlogic/qede/qede_filter.c | 9 ++-------
>  drivers/net/ethernet/qlogic/qede/qede_main.c   | 4 ++--
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> index c1a0708..7ee49b4 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> @@ -901,13 +901,8 @@ int qede_set_features(struct net_device *dev,
> netdev_features_t features)
>  	netdev_features_t changes = features ^ dev->features;
>  	bool need_reload = false;
> 
> -	/* No action needed if hardware GRO is disabled during driver load */
> -	if (changes & NETIF_F_GRO) {
> -		if (dev->features & NETIF_F_GRO)
> -			need_reload = !edev->gro_disable;
> -		else
> -			need_reload = edev->gro_disable;
> -	}
> +	if (changes & NETIF_F_GRO_HW)
> +		need_reload = true;

This doesn't look right; edev->gro_disable can change due to other
conditions as well - otherwise, it would have been synonymous with 
(dev->features & NETIF_F_GRO).

> 
>  	if (need_reload) {
>  		struct qede_reload_args args;
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index 8f9b3eb..b81620e 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -676,7 +676,7 @@ static void qede_init_ndev(struct qede_dev *edev)
>  	ndev->priv_flags |= IFF_UNICAST_FLT;
> 
>  	/* user-changeble features */
> -	hw_features = NETIF_F_GRO | NETIF_F_SG |
> +	hw_features = NETIF_F_GRO | NETIF_F_GRO_HW | NETIF_F_SG |
>  		      NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  		      NETIF_F_TSO | NETIF_F_TSO6;
> 
> @@ -1515,7 +1515,7 @@ static void qede_init_fp(struct qede_dev *edev)
>  			 edev->ndev->name, queue_id);
>  	}
> 
> -	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO);
> +	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO_HW);
>  }
> 
>  static int qede_set_real_num_queues(struct qede_dev *edev)
> --
> 1.8.3.1

Re: [PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.

[Michael Chan]

On Mon, Dec 4, 2017 at 1:48 PM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> Advertise NETIF_F_GRO_HW and turn on or off hardware GRO based on
>> NETIF_F_GRO_FW flag.
>>
>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>> Cc: everest-linux-l2@cavium.com
>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> ---
>>  drivers/net/ethernet/qlogic/qede/qede_filter.c | 9 ++-------
>>  drivers/net/ethernet/qlogic/qede/qede_main.c   | 4 ++--
>>  2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
>> b/drivers/net/ethernet/qlogic/qede/qede_filter.c
>> index c1a0708..7ee49b4 100644
>> --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
>> +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
>> @@ -901,13 +901,8 @@ int qede_set_features(struct net_device *dev,
>> netdev_features_t features)
>>       netdev_features_t changes = features ^ dev->features;
>>       bool need_reload = false;
>>
>> -     /* No action needed if hardware GRO is disabled during driver load */
>> -     if (changes & NETIF_F_GRO) {
>> -             if (dev->features & NETIF_F_GRO)
>> -                     need_reload = !edev->gro_disable;
>> -             else
>> -                     need_reload = edev->gro_disable;
>> -     }
>> +     if (changes & NETIF_F_GRO_HW)
>> +             need_reload = true;
>
> This doesn't look right; edev->gro_disable can change due to other
> conditions as well - otherwise, it would have been synonymous with
> (dev->features & NETIF_F_GRO).
>

Feel free to modify the patch.  The idea is for the driver to
determine in ndo_fix_features()/ndo_set_features() if GRO_HW can be
enabled.  For things like generic XDP, I think we can add code to
centrally disable GRO_HW and not have to worry about that in the
driver.

RE: [PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.

[Chopra, Manish]

> -----Original Message-----
> From: Michael Chan [mailto:michael.chan@broadcom.com]
> Sent: Tuesday, December 05, 2017 4:15 AM
> To: Yuval Mintz <yuvalm@mellanox.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Elior, Ariel
> <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2 <Dept-
> EngEverestLinuxL2@cavium.com>
> Subject: Re: [PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.
> 
> On Mon, Dec 4, 2017 at 1:48 PM, Yuval Mintz <yuvalm@mellanox.com> wrote:
> >> Advertise NETIF_F_GRO_HW and turn on or off hardware GRO based on
> >> NETIF_F_GRO_FW flag.
> >>
> >> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> >> Cc: everest-linux-l2@cavium.com
> >> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >> ---
> >>  drivers/net/ethernet/qlogic/qede/qede_filter.c | 9 ++-------
> >>  drivers/net/ethernet/qlogic/qede/qede_main.c   | 4 ++--
> >>  2 files changed, 4 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> >> b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> >> index c1a0708..7ee49b4 100644
> >> --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> >> +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> >> @@ -901,13 +901,8 @@ int qede_set_features(struct net_device *dev,
> >> netdev_features_t features)
> >>       netdev_features_t changes = features ^ dev->features;
> >>       bool need_reload = false;
> >>
> >> -     /* No action needed if hardware GRO is disabled during driver load */
> >> -     if (changes & NETIF_F_GRO) {
> >> -             if (dev->features & NETIF_F_GRO)
> >> -                     need_reload = !edev->gro_disable;
> >> -             else
> >> -                     need_reload = edev->gro_disable;
> >> -     }
> >> +     if (changes & NETIF_F_GRO_HW)
> >> +             need_reload = true;
> >
> > This doesn't look right; edev->gro_disable can change due to other
> > conditions as well - otherwise, it would have been synonymous with
> > (dev->features & NETIF_F_GRO).
> >
> 
> Feel free to modify the patch.  The idea is for the driver to determine in
> ndo_fix_features()/ndo_set_features() if GRO_HW can be enabled.  For things
> like generic XDP, I think we can add code to centrally disable GRO_HW and not
> have to worry about that in the driver.

Since we are now differentiating HW gro with distinct feature bit, I think we should consider this feature bit everywhere where driver disables
HW gro internally [not explicitly using ethtool]. This is not just specific to XDP but on some other conditions also driver disables HW gro in load flow.
I think with just this change we would end up with showing HW gro feature enabled but actually driver has disabled it in XDP or other scenarios internally.

I don't know if it's a good thing to do but just a suggestion - 
What if in driver load flow that is in the end of function qede_alloc_mem_rxq() we do something like below so that it will actually represent
the actual state of the feature ?

If (edev->gro_disable) {
	ndev->hw_features &= ~ NETIF_F_GRO_HW;
	ndev->features &= ~ NETIF_F_GRO_HW;
}

Re: [PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.

[Michael Chan]

On Tue, Dec 5, 2017 at 4:32 AM, Chopra, Manish <Manish.Chopra@cavium.com> wrote:

>
> Since we are now differentiating HW gro with distinct feature bit, I think we should consider this feature bit everywhere where driver disables
> HW gro internally [not explicitly using ethtool]. This is not just specific to XDP but on some other conditions also driver disables HW gro in load flow.
> I think with just this change we would end up with showing HW gro feature enabled but actually driver has disabled it in XDP or other scenarios internally.
>
> I don't know if it's a good thing to do but just a suggestion -
> What if in driver load flow that is in the end of function qede_alloc_mem_rxq() we do something like below so that it will actually represent
> the actual state of the feature ?
>
> If (edev->gro_disable) {
>         ndev->hw_features &= ~ NETIF_F_GRO_HW;
>         ndev->features &= ~ NETIF_F_GRO_HW;
> }

If edev->gro_disable means that the current configuration cannot
support GRO_HW, then this makes sense.  But I think it is best to
centralize this logic in ndo_fix_features().  The driver can then call
netdev_update_features() to update all features whenever there are
changes that can affect features.

RE: [PATCH net-next 0/4] Introduce NETIF_F_GRO_HW

[Elior, Ariel]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Michael Chan
> Sent: Monday, December 04, 2017 1:13 PM
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org
> Subject: [PATCH net-next 0/4] Introduce NETIF_F_GRO_HW
> 
> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
> hardware GRO to use the flag.
> 
> Michael Chan (4):
>   net: Introduce NETIF_F_GRO_HW
>   bnxt_en: Use NETIF_F_GRO_HW.
>   bnx2x: Use NETIF_F_GRO_HW.
>   qede: Use NETIF_F_GRO_HW.
> 
>  Documentation/networking/netdev-features.txt     |  7 +++++++
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 10 +++-------
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  4 +++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c        |  8 +++++---
>  drivers/net/ethernet/qlogic/qede/qede_filter.c   |  9 ++-------
>  drivers/net/ethernet/qlogic/qede/qede_main.c     |  4 ++--
>  include/linux/netdev_features.h                  |  5 ++++-
>  net/core/dev.c                                   | 13 +++++++++++++
>  net/core/ethtool.c                               |  1 +
>  9 files changed, 40 insertions(+), 21 deletions(-)
> 
> --
> 1.8.3.1

Michael, Thanks for adding this. 
Dave, we will be reviewing and testing out the changes on bnx2x and qede.
Will provide feedback shortly.
Thanks,
Ariel

Re: [PATCH net-next 0/4] Introduce NETIF_F_GRO_HW

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Mon,  4 Dec 2017 06:12:43 -0500

> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
> hardware GRO to use the flag.

So I think we should continue ahead with NETIF_F_GRO_HW.

LRO, even if it were in some way more appropriate, has so many stigmas
attached to it.  And if you look around the internet, articles say to
turn it off for this reason or that due to the information loss LRO
creates.

The only thing unresolved is how to manage the situation where a
bond or bridge has a mixture of LRO and GRO_HW devices attached,
and Michael said he would look into it.

Thanks.