| Message ID | 1512385967-32339-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 |
> 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
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.
> -----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; }
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.
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)
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(-)