diff mbox series

[V3,net,1/2] net: hns: fix wrong head when modify the tx feature when sending packets

Message ID 20231204143232.3221542-2-shaojijie@huawei.com (mailing list archive)
State Accepted
Commit 84757d0839451b20b11e993128f0a77393ca50c1
Delegated to: Netdev Maintainers
Headers show
Series There are some bugfix for the HNS ethernet driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1115 this patch: 1115
netdev/cc_maintainers warning 1 maintainers not CCed: u.kleine-koenig@pengutronix.de
netdev/build_clang success Errors and warnings before: 1142 this patch: 1142
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1142 this patch: 1142
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 111 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao Dec. 4, 2023, 2:32 p.m. UTC 
From: Yonglong Liu <liuyonglong@huawei.com>

Upon changing the tx feature, the hns driver will modify the
maybe_stop_tx() and fill_desc() functions, if the modify happens
during packet sending, will cause the hardware and software
pointers do not match, and the port can not work anymore.

This patch deletes the maybe_stop_tx() and fill_desc() functions
modification when setting tx feature, and use the skb_is_gro()
to determine which functions to use in the tx path.

Fixes: 38f616da1c28 ("net:hns: Add support of ethtool TSO set option for Hip06 in HNS")
Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_enet.c | 53 +++++++++++--------
 drivers/net/ethernet/hisilicon/hns/hns_enet.h |  3 +-
 2 files changed, 33 insertions(+), 23 deletions(-)

Comments

Paolo Abeni Dec. 6, 2023, 11:18 a.m. UTC | #1 
On Mon, 2023-12-04 at 22:32 +0800, Jijie Shao wrote:
> @@ -2159,16 +2175,9 @@ static void hns_nic_set_priv_ops(struct net_device *netdev)
>  		priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
>  	} else {
>  		priv->ops.get_rxd_bnum = get_v2rx_desc_bnum;
> -		if ((netdev->features & NETIF_F_TSO) ||
> -		    (netdev->features & NETIF_F_TSO6)) {
> -			priv->ops.fill_desc = fill_tso_desc;
> -			priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tso;
> -			/* This chip only support 7*4096 */
> -			netif_set_tso_max_size(netdev, 7 * 4096);
> -		} else {
> -			priv->ops.fill_desc = fill_v2_desc;
> -			priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
> -		}
> +		priv->ops.fill_desc = fill_desc_v2;
> +		priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
> +		netif_set_tso_max_size(netdev, 7 * 4096);
>  		/* enable tso when init
>  		 * control tso on/off through TSE bit in bd
>  		 */

Side note: since both 'fill_desc' and 'maybe_stop_tx' have constant
values, for net-next you should really consider replacing the function
pointers with direct-calls.

You currently have at least 2 indirect calls per wire packet, which
hurt performances a lot in case security issues mitigations are in
place.

Cheers,

Paolo
Jijie Shao Dec. 6, 2023, 12:31 p.m. UTC | #2 
on 2023/12/6 19:18, Paolo Abeni wrote:
> +		priv->ops.fill_desc = fill_desc_v2;
> +		priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
> Side note: since both 'fill_desc' and 'maybe_stop_tx' have constant
> values, for net-next you should really consider replacing the function
> pointers with direct-calls.
>
> You currently have at least 2 indirect calls per wire packet, which
> hurt performances a lot in case security issues mitigations are in
> place.
>
> Cheers,
>
> Paolo

Thank you for your advice. Currently, because the hardware behavior is 
different, the two versions of ops are retained to unify the subsequent 
process. We will try to unify the two version ops, and if that does not 
work, we will consider maintaining the status quo. Thanks again! Jijie
Paolo Abeni Dec. 6, 2023, 2:27 p.m. UTC | #3 
On Wed, 2023-12-06 at 20:31 +0800, Jijie Shao wrote:
> on 2023/12/6 19:18, Paolo Abeni wrote:
> > +		priv->ops.fill_desc = fill_desc_v2;
> > +		priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
> > Side note: since both 'fill_desc' and 'maybe_stop_tx' have constant
> > values, for net-next you should really consider replacing the function
> > pointers with direct-calls.
> > 
> > You currently have at least 2 indirect calls per wire packet, which
> > hurt performances a lot in case security issues mitigations are in
> > place.
> > 
> > Cheers,
> > 
> > Paolo
> 
> Thank you for your advice. Currently, because the hardware behavior is 
> different, the two versions of ops are retained to unify the subsequent 
> process. We will try to unify the two version ops, and if that does not 
> work, we will consider maintaining the status quo. Thanks again! Jijie

Note that even if you will have to resort to call different functions
for different H/W revisions, from performance PoV you will be far
better off doing a test at runtime to call the corresponding helper.

Or you can use the indirect call wrappers - have a look into:

include/linux/indirect_call_wrapper.h

Cheers,

Paolo
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index 0900abf5c508..8a713eed4465 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -142,7 +142,8 @@  MODULE_DEVICE_TABLE(acpi, hns_enet_acpi_match);
 
 static void fill_desc(struct hnae_ring *ring, void *priv,
 		      int size, dma_addr_t dma, int frag_end,
-		      int buf_num, enum hns_desc_type type, int mtu)
+		      int buf_num, enum hns_desc_type type, int mtu,
+		      bool is_gso)
 {
 	struct hnae_desc *desc = &ring->desc[ring->next_to_use];
 	struct hnae_desc_cb *desc_cb = &ring->desc_cb[ring->next_to_use];
@@ -275,6 +276,15 @@  static int hns_nic_maybe_stop_tso(
 	return 0;
 }
 
+static int hns_nic_maybe_stop_tx_v2(struct sk_buff **out_skb, int *bnum,
+				    struct hnae_ring *ring)
+{
+	if (skb_is_gso(*out_skb))
+		return hns_nic_maybe_stop_tso(out_skb, bnum, ring);
+	else
+		return hns_nic_maybe_stop_tx(out_skb, bnum, ring);
+}
+
 static void fill_tso_desc(struct hnae_ring *ring, void *priv,
 			  int size, dma_addr_t dma, int frag_end,
 			  int buf_num, enum hns_desc_type type, int mtu)
@@ -300,6 +310,19 @@  static void fill_tso_desc(struct hnae_ring *ring, void *priv,
 				mtu);
 }
 
+static void fill_desc_v2(struct hnae_ring *ring, void *priv,
+			 int size, dma_addr_t dma, int frag_end,
+			 int buf_num, enum hns_desc_type type, int mtu,
+			 bool is_gso)
+{
+	if (is_gso)
+		fill_tso_desc(ring, priv, size, dma, frag_end, buf_num, type,
+			      mtu);
+	else
+		fill_v2_desc(ring, priv, size, dma, frag_end, buf_num, type,
+			     mtu);
+}
+
 netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
 				struct sk_buff *skb,
 				struct hns_nic_ring_data *ring_data)
@@ -313,6 +336,7 @@  netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
 	int seg_num;
 	dma_addr_t dma;
 	int size, next_to_use;
+	bool is_gso;
 	int i;
 
 	switch (priv->ops.maybe_stop_tx(&skb, &buf_num, ring)) {
@@ -339,8 +363,9 @@  netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
 		ring->stats.sw_err_cnt++;
 		goto out_err_tx_ok;
 	}
+	is_gso = skb_is_gso(skb);
 	priv->ops.fill_desc(ring, skb, size, dma, seg_num == 1 ? 1 : 0,
-			    buf_num, DESC_TYPE_SKB, ndev->mtu);
+			    buf_num, DESC_TYPE_SKB, ndev->mtu, is_gso);
 
 	/* fill the fragments */
 	for (i = 1; i < seg_num; i++) {
@@ -354,7 +379,7 @@  netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
 		}
 		priv->ops.fill_desc(ring, skb_frag_page(frag), size, dma,
 				    seg_num - 1 == i ? 1 : 0, buf_num,
-				    DESC_TYPE_PAGE, ndev->mtu);
+				    DESC_TYPE_PAGE, ndev->mtu, is_gso);
 	}
 
 	/*complete translate all packets*/
@@ -1776,15 +1801,6 @@  static int hns_nic_set_features(struct net_device *netdev,
 			netdev_info(netdev, "enet v1 do not support tso!\n");
 		break;
 	default:
-		if (features & (NETIF_F_TSO | NETIF_F_TSO6)) {
-			priv->ops.fill_desc = fill_tso_desc;
-			priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tso;
-			/* The chip only support 7*4096 */
-			netif_set_tso_max_size(netdev, 7 * 4096);
-		} else {
-			priv->ops.fill_desc = fill_v2_desc;
-			priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
-		}
 		break;
 	}
 	netdev->features = features;
@@ -2159,16 +2175,9 @@  static void hns_nic_set_priv_ops(struct net_device *netdev)
 		priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
 	} else {
 		priv->ops.get_rxd_bnum = get_v2rx_desc_bnum;
-		if ((netdev->features & NETIF_F_TSO) ||
-		    (netdev->features & NETIF_F_TSO6)) {
-			priv->ops.fill_desc = fill_tso_desc;
-			priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tso;
-			/* This chip only support 7*4096 */
-			netif_set_tso_max_size(netdev, 7 * 4096);
-		} else {
-			priv->ops.fill_desc = fill_v2_desc;
-			priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
-		}
+		priv->ops.fill_desc = fill_desc_v2;
+		priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
+		netif_set_tso_max_size(netdev, 7 * 4096);
 		/* enable tso when init
 		 * control tso on/off through TSE bit in bd
 		 */
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.h b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
index ffa9d6573f54..3f3ee032f631 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
@@ -44,7 +44,8 @@  struct hns_nic_ring_data {
 struct hns_nic_ops {
 	void (*fill_desc)(struct hnae_ring *ring, void *priv,
 			  int size, dma_addr_t dma, int frag_end,
-			  int buf_num, enum hns_desc_type type, int mtu);
+			  int buf_num, enum hns_desc_type type, int mtu,
+			  bool is_gso);
 	int (*maybe_stop_tx)(struct sk_buff **out_skb,
 			     int *bnum, struct hnae_ring *ring);
 	void (*get_rxd_bnum)(u32 bnum_flag, int *out_bnum);