Re: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Michael Chan]

On Thu, Dec 14, 2017 at 11:07 PM, Chopra, Manish
 wrote:

> Michael, I checked it on again, I tried to set LRO in dev->features and 
> dev->hw_features.
> Somehow, it gets disabled after register_netdevice().  Any idea why ? 
> Although, I am not running any Bridge/bonding devices.
> Looks like, without this series also devices seems to have LRO disabled by 
> default.
> Not sure why register_netdevice() disables LRO even driver populates this 
> feature prior to register_netdevice().

May be you have ip forwarding enabled.

RE: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Chopra, Manish]

> -Original Message-
> From: Michael Chan [mailto:michael.c...@broadcom.com]
> Sent: Thursday, December 14, 2017 2:16 AM
> To: Chopra, Manish <manish.cho...@cavium.com>
> Cc: da...@davemloft.net; netdev@vger.kernel.org;
> andrew.gospoda...@broadcom.com; Elior, Ariel <ariel.el...@cavium.com>;
> Dept-Eng Everest Linux L2 <dept-engeverestlinu...@cavium.com>
> Subject: Re: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.
> 
> On Wed, Dec 13, 2017 at 1:08 AM, Chopra, Manish
> <manish.cho...@cavium.com> wrote:
> >
> > Hi Michael,  There seems a behavioral change here. This driver support
> > two HW aggregation modes [LRO and GRO] With the changes, Interfaces
> come with HW GRO enabled and LRO disabled by default as opposed to earlier
> where interfaces used to come with LRO enabled.
> 
> Right.  Before, you had both NETIF_F_GRO and NETIF_F_LRO set and the code
> looked at NETIF_F_LRO first and turned on LRO.
> 
> Now, we set NETIF_F_GRO and NETIF_F_GRO_HW by default.  NETIF_F_LRO is
> turned off since NETIF_F_GRO_HW is on.
> 
> If you want, I can change it back to the old default.
> 
Michael, I checked it on again, I tried to set LRO in dev->features and 
dev->hw_features.
Somehow, it gets disabled after register_netdevice().  Any idea why ? Although, 
I am not running any Bridge/bonding devices.
Looks like, without this series also devices seems to have LRO disabled by 
default.
Not sure why register_netdevice() disables LRO even driver populates this 
feature prior to register_netdevice().

Re: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Michael Chan]

On Wed, Dec 13, 2017 at 11:46 PM, Chopra, Manish
 wrote:

>
> 2). In bnx2x_fix_features() we used to do before these changes -
>
> /* TPA requires Rx CSUM offloading */
> if (!(features & NETIF_F_RXCSUM)) {
> features &= ~NETIF_F_LRO;
> features &= ~NETIF_F_GRO;
> }
>
> I think we should not turn off SW gro here, we should turn off HW gro here 
> now ?

OK.  I will remove GRO.

GRO_HW will be turned off by netdev_fix_features() when RXCSUM is not on.

RE: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Chopra, Manish]

> -Original Message-
> From: Michael Chan [mailto:michael.c...@broadcom.com]
> Sent: Thursday, December 14, 2017 2:16 AM
> To: Chopra, Manish <manish.cho...@cavium.com>
> Cc: da...@davemloft.net; netdev@vger.kernel.org;
> andrew.gospoda...@broadcom.com; Elior, Ariel <ariel.el...@cavium.com>;
> Dept-Eng Everest Linux L2 <dept-engeverestlinu...@cavium.com>
> Subject: Re: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.
> 
> On Wed, Dec 13, 2017 at 1:08 AM, Chopra, Manish
> <manish.cho...@cavium.com> wrote:
> >
> > Hi Michael,  There seems a behavioral change here. This driver support
> > two HW aggregation modes [LRO and GRO] With the changes, Interfaces
> come with HW GRO enabled and LRO disabled by default as opposed to earlier
> where interfaces used to come with LRO enabled.
> 
> Right.  Before, you had both NETIF_F_GRO and NETIF_F_LRO set and the code
> looked at NETIF_F_LRO first and turned on LRO.
> 
> Now, we set NETIF_F_GRO and NETIF_F_GRO_HW by default.  NETIF_F_LRO is
> turned off since NETIF_F_GRO_HW is on.
> 
> If you want, I can change it back to the old default.

Yes please, I think we should not make any default behavior change.
Few more comments -

1). This driver uses module param "disable_tpa" also to completely disable TPA 
[whether it be LRO or HW GRO] along the initialization flow.

For ex. 

/*Set TPA flags*/
if (bp->disable_tpa) {
bp->dev->hw_features &= ~NETIF_F_LRO;
bp->dev->features &= ~NETIF_F_LRO;
}

I think we should also disable HW gro here as well, so that device will come up 
with no TPA at all.

2). In bnx2x_fix_features() we used to do before these changes -

/* TPA requires Rx CSUM offloading */
if (!(features & NETIF_F_RXCSUM)) {
features &= ~NETIF_F_LRO;
features &= ~NETIF_F_GRO;
}

I think we should not turn off SW gro here, we should turn off HW gro here now ?

> 
> >
> > Also, there seems some problem when turning on LRO even after GRO disable.
> > When I tried to disable GRO and then tried to enable LRO it didn't go well. 
> > Not
> sure why ?
> 
> I just put an old BCM57810 card into my machine and it works for me.
> As long as I turn off GRO or GRO_HW, I can turn on LRO.
> 
> [root@localhost bnx2x]# ethtool -K p1p1 lro on Cannot get device udp-
> fragmentation-offload settings: Operation not supported Cannot get device
> udp-fragmentation-offload settings: Operation not supported Could not change
> any device features [root@localhost bnx2x]# ethtool -K p1p1 gro off Cannot get
> device udp-fragmentation-offload settings: Operation not supported Cannot get
> device udp-fragmentation-offload settings: Operation not supported Actual
> changes:
> generic-receive-offload: off
> large-receive-offload: on
> rx-gro-hw: off [requested on]
> [root@localhost bnx2x]# ethtool -K p1p1 lro on Cannot get device udp-
> fragmentation-offload settings: Operation not supported Cannot get device
> udp-fragmentation-offload settings: Operation not supported

Doesn't sound like HW specific.
Probably, I was testing on your older series, I will check again on latest 
series.

Re: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Michael Chan]

On Wed, Dec 13, 2017 at 1:08 AM, Chopra, Manish
 wrote:
>
> Hi Michael,  There seems a behavioral change here. This driver support two HW 
> aggregation modes [LRO and GRO]
> With the changes, Interfaces come with HW GRO enabled and LRO disabled by 
> default as opposed to earlier where interfaces used to come with LRO enabled.

Right.  Before, you had both NETIF_F_GRO and NETIF_F_LRO set and the
code looked at NETIF_F_LRO first and turned on LRO.

Now, we set NETIF_F_GRO and NETIF_F_GRO_HW by default.  NETIF_F_LRO is
turned off since NETIF_F_GRO_HW is on.

If you want, I can change it back to the old default.

>
> Also, there seems some problem when turning on LRO even after GRO disable.
> When I tried to disable GRO and then tried to enable LRO it didn't go well. 
> Not sure why ?

I just put an old BCM57810 card into my machine and it works for me.
As long as I turn off GRO or GRO_HW, I can turn on LRO.

[root@localhost bnx2x]# ethtool -K p1p1 lro on
Cannot get device udp-fragmentation-offload settings: Operation not supported
Cannot get device udp-fragmentation-offload settings: Operation not supported
Could not change any device features
[root@localhost bnx2x]# ethtool -K p1p1 gro off
Cannot get device udp-fragmentation-offload settings: Operation not supported
Cannot get device udp-fragmentation-offload settings: Operation not supported
Actual changes:
generic-receive-offload: off
large-receive-offload: on
rx-gro-hw: off [requested on]
[root@localhost bnx2x]# ethtool -K p1p1 lro on
Cannot get device udp-fragmentation-offload settings: Operation not supported
Cannot get device udp-fragmentation-offload settings: Operation not supported

RE: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Chopra, Manish]

> -Original Message-
> From: Michael Chan [mailto:michael.c...@broadcom.com]
> Sent: Monday, December 11, 2017 5:11 PM
> To: da...@davemloft.net
> Cc: netdev@vger.kernel.org; andrew.gospoda...@broadcom.com; Elior, Ariel
> ; Dept-Eng Everest Linux L2  engeverestlinu...@cavium.com>
> Subject: [PATCH net-next v4 4/5] bnx2x: Use NETIF_F_GRO_HW.
> 
> Advertise NETIF_F_GRO_HW and turn on TPA_MODE_GRO when
> NETIF_F_GRO_HW is set.  Disable NETIF_F_GRO_HW in bnx2x_fix_features() if
> the MTU does not support TPA_MODE_GRO or GRO is not set.
> bnx2x_change_mtu() also needs to disable NETIF_F_GRO_HW if the MTU does
> not support it.
> 
> Cc: Ariel Elior 
> Cc: everest-linux...@cavium.com
> Signed-off-by: Michael Chan 
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 19 ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  4 +++-
>  2 files changed, 15 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..f9b18a7 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -2482,8 +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 &&
> -  bnx2x_mtu_allows_gro(bp->dev->mtu))
> + else if (bp->dev->features & NETIF_F_GRO_HW)
>   fp->mode = TPA_MODE_GRO;
>   else
>   fp->mode = TPA_MODE_DISABLED;
> @@ -4874,6 +4873,9 @@ int bnx2x_change_mtu(struct net_device *dev, int
> new_mtu)
>*/
>   dev->mtu = new_mtu;
> 
> + if (!bnx2x_mtu_allows_gro(new_mtu))
> + dev->features &= ~NETIF_F_GRO_HW;
> +
>   if (IS_PF(bp) && SHMEM2_HAS(bp, curr_cfg))
>   SHMEM2_WR(bp, curr_cfg, CURR_CFG_MET_OS);
> 
> @@ -4907,6 +4909,10 @@ netdev_features_t bnx2x_fix_features(struct
> net_device *dev,
>   features &= ~NETIF_F_LRO;
>   features &= ~NETIF_F_GRO;
>   }
> + if (!(features & NETIF_F_GRO) || !bnx2x_mtu_allows_gro(dev->mtu))
> + features &= ~NETIF_F_GRO_HW;
> + if (features & NETIF_F_GRO_HW)
> + features &= ~NETIF_F_LRO;
> 
>   return features;
>  }
> @@ -4933,13 +4939,12 @@ 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;
> + /* Don't care about GRO changes */
> + 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 91e2a75..1c7f821 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -13273,7 +13273,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 | @@ -13309,6 +13309,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

Hi Michael,  There seems a behavioral change here. This driver support two HW 
aggregation modes [LRO and GRO]
With the changes, Interfaces come with HW GRO enabled and LRO disabled by 
default as opposed to earlier where interfaces used to come with LRO enabled.

Also, there seems some problem when turning on LRO even after GRO disable.
When I tried to disable GRO and then tried to enable LRO it didn't go well. Not 
sure why ?

ethtool -K p3p1 gro off
Cannot get device udp-fragmentation-offload settings: Operation not supported
Cannot get device udp-fragmentation-offload settings: Operation not