| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 8 Dec 2017 17:02:06 +0000
From: "Chopra, Manish" <Manish.Chopra@...ium.com>
To: Michael Chan <michael.chan@...adcom.com>,
"davem@...emloft.net" <davem@...emloft.net>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"andrew.gospodarek@...adcom.com" <andrew.gospodarek@...adcom.com>,
"Elior, Ariel" <Ariel.Elior@...ium.com>,
Dept-Eng Everest Linux L2 <Dept-EngEverestLinuxL2@...ium.com>
Subject: RE: [PATCH net-next v2 5/5] qede: Use NETIF_F_GRO_HW.
> -----Original Message-----
> From: Michael Chan [mailto:michael.chan@...adcom.com]
> Sent: Thursday, December 07, 2017 1:34 PM
> To: davem@...emloft.net
> Cc: netdev@...r.kernel.org; andrew.gospodarek@...adcom.com; Elior, Ariel
> <Ariel.Elior@...ium.com>; Dept-Eng Everest Linux L2 <Dept-
> EngEverestLinuxL2@...ium.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@...ium.com>
> Cc: everest-linux-l2@...ium.com
> Signed-off-by: Michael Chan <michael.chan@...adcom.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@...ium.com>
Powered by blists - more mailing lists