| Message ID | 1512633815-25037-6-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: Thursday, December 07, 2017 1:34 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 v2 5/5] qede: Use NETIF_F_GRO_HW. > > Advertise NETIF_F_GRO_HW and set edev->gro_disable according to the feature > flag. Add qede_fix_features() to drop NETIF_F_GRO_HW if XDP is running or > MTU does not support GRO_HW. qede_change_mtu() also checks and disables > GRO_HW if MTU is not supported. > > 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.h | 2 ++ > drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 3 +++ > drivers/net/ethernet/qlogic/qede/qede_filter.c | 20 +++++++++++++------- > drivers/net/ethernet/qlogic/qede/qede_main.c | 17 ++++++----------- > 4 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/qlogic/qede/qede.h > b/drivers/net/ethernet/qlogic/qede/qede.h > index a3a70ad..8a33651 100644 > --- a/drivers/net/ethernet/qlogic/qede/qede.h > +++ b/drivers/net/ethernet/qlogic/qede/qede.h > @@ -494,6 +494,8 @@ int qede_free_tx_pkt(struct qede_dev *edev, void > qede_vlan_mark_nonconfigured(struct qede_dev *edev); int > qede_configure_vlan_filters(struct qede_dev *edev); > > +netdev_features_t qede_fix_features(struct net_device *dev, > + netdev_features_t features); > int qede_set_features(struct net_device *dev, netdev_features_t features); > void qede_set_rx_mode(struct net_device *ndev); void > qede_config_rx_mode(struct net_device *ndev); diff --git > a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c > b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c > index dae7412..4ca3847 100644 > --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c > +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c > @@ -940,6 +940,9 @@ int qede_change_mtu(struct net_device *ndev, int > new_mtu) > DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN), > "Configuring MTU size of %d\n", new_mtu); > > + if (new_mtu > PAGE_SIZE) > + ndev->features &= ~NETIF_F_GRO_HW; > + > /* Set the mtu field and re-start the interface if needed */ > args.u.mtu = new_mtu; > args.func = &qede_update_mtu; > diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c > b/drivers/net/ethernet/qlogic/qede/qede_filter.c > index c1a0708..2de947e 100644 > --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c > +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c > @@ -895,19 +895,25 @@ static void qede_set_features_reload(struct > qede_dev *edev, > edev->ndev->features = args->u.features; } > > +netdev_features_t qede_fix_features(struct net_device *dev, > + netdev_features_t features) > +{ > + struct qede_dev *edev = netdev_priv(dev); > + > + if (edev->xdp_prog || edev->ndev->mtu > PAGE_SIZE) > + features &= ~NETIF_F_GRO_HW; > + > + return features; > +} > + > int qede_set_features(struct net_device *dev, netdev_features_t features) { > struct qede_dev *edev = netdev_priv(dev); > 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 57332b3..90d79ae 100644 > --- a/drivers/net/ethernet/qlogic/qede/qede_main.c > +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c > @@ -545,6 +545,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq > *ifr, int cmd) #endif > .ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid, > + .ndo_fix_features = qede_fix_features, > .ndo_set_features = qede_set_features, > .ndo_get_stats64 = qede_get_stats64, > #ifdef CONFIG_QED_SRIOV > @@ -572,6 +573,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq > *ifr, int cmd) > .ndo_change_mtu = qede_change_mtu, > .ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid, > + .ndo_fix_features = qede_fix_features, > .ndo_set_features = qede_set_features, > .ndo_get_stats64 = qede_get_stats64, > .ndo_udp_tunnel_add = qede_udp_tunnel_add, @@ -589,6 +591,7 @@ > static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) > .ndo_change_mtu = qede_change_mtu, > .ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid, > + .ndo_fix_features = qede_fix_features, > .ndo_set_features = qede_set_features, > .ndo_get_stats64 = qede_get_stats64, > .ndo_udp_tunnel_add = qede_udp_tunnel_add, @@ -676,7 +679,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; > > @@ -1228,18 +1231,9 @@ static int qede_alloc_sge_mem(struct qede_dev > *edev, struct qede_rx_queue *rxq) > dma_addr_t mapping; > int i; > > - /* Don't perform FW aggregations in case of XDP */ > - if (edev->xdp_prog) > - edev->gro_disable = 1; > - > if (edev->gro_disable) > return 0; > > - if (edev->ndev->mtu > PAGE_SIZE) { > - edev->gro_disable = 1; > - return 0; > - } > - > for (i = 0; i < ETH_TPA_MAX_AGGS_NUM; i++) { > struct qede_agg_info *tpa_info = &rxq->tpa_info[i]; > struct sw_rx_data *replace_buf = &tpa_info->buffer; @@ - > 1269,6 +1263,7 @@ static int qede_alloc_sge_mem(struct qede_dev *edev, > struct qede_rx_queue *rxq) > err: > qede_free_sge_mem(edev, rxq); > edev->gro_disable = 1; > + edev->ndev->features &= ~NETIF_F_GRO_HW; > return -ENOMEM; > } > > @@ -1511,7 +1506,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 Hi Michael, we have currently tested changes for qede driver and that overall looks good. One small observation though in following steps - 1) HW GRO on initially as per "ethtool -k". 2) change MTU to 9000 which will not be supported with HW gro [HW gro gets off as per "ethtool -k"]. 3) Change MTU back to 1500 which is supported mtu for HW gro but HW gro remains off as per "ethtool -k". I don't have any strong recommendation that in step 3 if user fallback to supported mtu for HW gro then HW gro should be again turned on automatically. I will leave up to user to control it back through ethtool. Please note that we are yet to test bnx2x but I do see you are going to send V3. Hopefully, we will be able to provide feedback on bnx2x by that time. Thanks for making this change. Acked-by: Manish Chopra <manish.chopra@cavium.com>
Hi, On Thu, Dec 07, 2017 at 03:03:35AM -0500, Michael Chan wrote: > --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c > +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c > @@ -940,6 +940,9 @@ int qede_change_mtu(struct net_device *ndev, int new_mtu) > DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN), > "Configuring MTU size of %d\n", new_mtu); > > + if (new_mtu > PAGE_SIZE) I don't know the specs for this card but if it needs to fit the whole packet in a page, maybe it should consider the ethernet header size in such checks? Marcelo
On Fri, Dec 8, 2017 at 2:09 PM, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > Hi, > > On Thu, Dec 07, 2017 at 03:03:35AM -0500, Michael Chan wrote: >> --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c >> +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c >> @@ -940,6 +940,9 @@ int qede_change_mtu(struct net_device *ndev, int new_mtu) >> DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN), >> "Configuring MTU size of %d\n", new_mtu); >> >> + if (new_mtu > PAGE_SIZE) > > I don't know the specs for this card but if it needs to fit the whole > packet in a page, maybe it should consider the ethernet header size in > such checks? > I am not changing the logic in this patch, just moving the check from qede_alloc_sge_mem() to qede_fix_features(). Typically, the chip will also do header-data split when doing GRO_HW, so that's probably why it is checking the MTU and not the total packet size against page size. Ariel can confirm.
diff --git a/drivers/net/ethernet/qlogic/qede/qede.h b/drivers/net/ethernet/qlogic/qede/qede.h index a3a70ad..8a33651 100644 --- a/drivers/net/ethernet/qlogic/qede/qede.h +++ b/drivers/net/ethernet/qlogic/qede/qede.h @@ -494,6 +494,8 @@ int qede_free_tx_pkt(struct qede_dev *edev, void qede_vlan_mark_nonconfigured(struct qede_dev *edev); int qede_configure_vlan_filters(struct qede_dev *edev); +netdev_features_t qede_fix_features(struct net_device *dev, + netdev_features_t features); int qede_set_features(struct net_device *dev, netdev_features_t features); void qede_set_rx_mode(struct net_device *ndev); void qede_config_rx_mode(struct net_device *ndev); diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c index dae7412..4ca3847 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c @@ -940,6 +940,9 @@ int qede_change_mtu(struct net_device *ndev, int new_mtu) DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN), "Configuring MTU size of %d\n", new_mtu); + if (new_mtu > PAGE_SIZE) + ndev->features &= ~NETIF_F_GRO_HW; + /* Set the mtu field and re-start the interface if needed */ args.u.mtu = new_mtu; args.func = &qede_update_mtu; diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c index c1a0708..2de947e 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c @@ -895,19 +895,25 @@ static void qede_set_features_reload(struct qede_dev *edev, edev->ndev->features = args->u.features; } +netdev_features_t qede_fix_features(struct net_device *dev, + netdev_features_t features) +{ + struct qede_dev *edev = netdev_priv(dev); + + if (edev->xdp_prog || edev->ndev->mtu > PAGE_SIZE) + features &= ~NETIF_F_GRO_HW; + + return features; +} + int qede_set_features(struct net_device *dev, netdev_features_t features) { struct qede_dev *edev = netdev_priv(dev); 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 57332b3..90d79ae 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_main.c +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c @@ -545,6 +545,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) #endif .ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid, + .ndo_fix_features = qede_fix_features, .ndo_set_features = qede_set_features, .ndo_get_stats64 = qede_get_stats64, #ifdef CONFIG_QED_SRIOV @@ -572,6 +573,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) .ndo_change_mtu = qede_change_mtu, .ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid, + .ndo_fix_features = qede_fix_features, .ndo_set_features = qede_set_features, .ndo_get_stats64 = qede_get_stats64, .ndo_udp_tunnel_add = qede_udp_tunnel_add, @@ -589,6 +591,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) .ndo_change_mtu = qede_change_mtu, .ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid, + .ndo_fix_features = qede_fix_features, .ndo_set_features = qede_set_features, .ndo_get_stats64 = qede_get_stats64, .ndo_udp_tunnel_add = qede_udp_tunnel_add, @@ -676,7 +679,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; @@ -1228,18 +1231,9 @@ static int qede_alloc_sge_mem(struct qede_dev *edev, struct qede_rx_queue *rxq) dma_addr_t mapping; int i; - /* Don't perform FW aggregations in case of XDP */ - if (edev->xdp_prog) - edev->gro_disable = 1; - if (edev->gro_disable) return 0; - if (edev->ndev->mtu > PAGE_SIZE) { - edev->gro_disable = 1; - return 0; - } - for (i = 0; i < ETH_TPA_MAX_AGGS_NUM; i++) { struct qede_agg_info *tpa_info = &rxq->tpa_info[i]; struct sw_rx_data *replace_buf = &tpa_info->buffer; @@ -1269,6 +1263,7 @@ static int qede_alloc_sge_mem(struct qede_dev *edev, struct qede_rx_queue *rxq) err: qede_free_sge_mem(edev, rxq); edev->gro_disable = 1; + edev->ndev->features &= ~NETIF_F_GRO_HW; return -ENOMEM; } @@ -1511,7 +1506,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 set edev->gro_disable according to the feature flag. Add qede_fix_features() to drop NETIF_F_GRO_HW if XDP is running or MTU does not support GRO_HW. qede_change_mtu() also checks and disables GRO_HW if MTU is not supported. 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.h | 2 ++ drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 3 +++ drivers/net/ethernet/qlogic/qede/qede_filter.c | 20 +++++++++++++------- drivers/net/ethernet/qlogic/qede/qede_main.c | 17 ++++++----------- 4 files changed, 24 insertions(+), 18 deletions(-)