[net-next,v2,5/5] qede: Use NETIF_F_GRO_HW.

Message ID 1512633815-25037-6-git-send-email-michael.chan@broadcom.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • Introduce NETIF_F_GRO_HW
Related show

Commit Message

Michael Chan Dec. 7, 2017, 8:03 a.m.
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(-)

Comments

Chopra, Manish Dec. 8, 2017, 5:02 p.m. | #1
> -----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>
Marcelo Ricardo Leitner Dec. 8, 2017, 10:09 p.m. | #2
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
Michael Chan Dec. 8, 2017, 10:40 p.m. | #3
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.

Patch

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)