| Message ID | 1512992470-28861-5-git-send-email-michael.chan@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Introduce NETIF_F_GRO_HW | expand |
> -----Original Message----- > From: Michael Chan [mailto:michael.chan@broadcom.com] > Sent: Monday, December 11, 2017 5:11 PM > To: davem@davemloft.net > Cc: netdev@vger.kernel.org; andrew.gospodarek@broadcom.com; Elior, Ariel > <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2 <Dept- > EngEverestLinuxL2@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 <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 | 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 supported Actual changes: generic-receive-offload: off rx-gro-hw: off [requested on] ethtool -K p3p1 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 Thanks, Manish
On Wed, Dec 13, 2017 at 1:08 AM, Chopra, Manish <Manish.Chopra@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. > > 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
> -----Original Message----- > From: Michael Chan [mailto:michael.chan@broadcom.com] > Sent: Thursday, December 14, 2017 2:16 AM > To: Chopra, Manish <Manish.Chopra@cavium.com> > Cc: davem@davemloft.net; netdev@vger.kernel.org; > andrew.gospodarek@broadcom.com; Elior, Ariel <Ariel.Elior@cavium.com>; > Dept-Eng Everest Linux L2 <Dept-EngEverestLinuxL2@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.Chopra@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.
On Wed, Dec 13, 2017 at 11:46 PM, Chopra, Manish <Manish.Chopra@cavium.com> 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.
> -----Original Message----- > From: Michael Chan [mailto:michael.chan@broadcom.com] > Sent: Thursday, December 14, 2017 2:16 AM > To: Chopra, Manish <Manish.Chopra@cavium.com> > Cc: davem@davemloft.net; netdev@vger.kernel.org; > andrew.gospodarek@broadcom.com; Elior, Ariel <Ariel.Elior@cavium.com>; > Dept-Eng Everest Linux L2 <Dept-EngEverestLinuxL2@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.Chopra@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().
On Thu, Dec 14, 2017 at 11:07 PM, Chopra, Manish <Manish.Chopra@cavium.com> 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.
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;
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 <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 | 19 ++++++++++++------- drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 4 +++- 2 files changed, 15 insertions(+), 8 deletions(-)