DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v1 1/1] drivers: remove implementation of Rx metadata negotiation
@ 2023-03-06 12:08 Hanumanth Pothula
  2023-03-07  7:47 ` Jerin Jacob
  2023-03-07 10:46 ` [PATCH v2 " Hanumanth Pothula
  0 siblings, 2 replies; 4+ messages in thread
From: Hanumanth Pothula @ 2023-03-06 12:08 UTC (permalink / raw)
  To: Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao
  Cc: dev, jerinj, hpothula

Presently, Rx metadata is sent to PMD by default, leading
to a performance drop as processing for the same in Rx path
takes extra cycles.

Hence, removing driver implementation of Rx metadata negotiation
and falling back to old implementation where mark actions are
tracked as part of the flow rule.

Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
---
 drivers/common/cnxk/roc_npc.c      | 19 +++++++++++++++++++
 drivers/common/cnxk/roc_npc.h      |  3 +++
 drivers/common/cnxk/roc_npc_priv.h |  1 +
 drivers/common/cnxk/version.map    |  2 ++
 drivers/net/cnxk/cn10k_ethdev.c    | 26 --------------------------
 drivers/net/cnxk/cn10k_flow.c      | 19 +++++++++++++++++++
 drivers/net/cnxk/cn9k_ethdev.c     | 25 -------------------------
 drivers/net/cnxk/cn9k_flow.c       | 20 ++++++++++++++++++++
 drivers/net/cnxk/cnxk_ethdev.h     |  1 -
 9 files changed, 64 insertions(+), 52 deletions(-)

diff --git a/drivers/common/cnxk/roc_npc.c b/drivers/common/cnxk/roc_npc.c
index a795114326..47536c8ce8 100644
--- a/drivers/common/cnxk/roc_npc.c
+++ b/drivers/common/cnxk/roc_npc.c
@@ -5,6 +5,23 @@
 #include "roc_api.h"
 #include "roc_priv.h"
 
+int
+roc_npc_mark_actions_get(struct roc_npc *roc_npc)
+{
+	struct npc *npc = roc_npc_to_npc_priv(roc_npc);
+
+	return npc->mark_actions;
+}
+
+int
+roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc, uint32_t count)
+{
+	struct npc *npc = roc_npc_to_npc_priv(roc_npc);
+
+	npc->mark_actions -= count;
+	return npc->mark_actions;
+}
+
 int
 roc_npc_vtag_actions_get(struct roc_npc *roc_npc)
 {
@@ -488,12 +505,14 @@ npc_parse_actions(struct roc_npc *roc_npc, const struct roc_npc_attr *attr,
 			}
 			mark = act_mark->id + 1;
 			req_act |= ROC_NPC_ACTION_TYPE_MARK;
+			npc->mark_actions += 1;
 			flow->match_id = mark;
 			break;
 
 		case ROC_NPC_ACTION_TYPE_FLAG:
 			mark = NPC_FLOW_FLAG_VAL;
 			req_act |= ROC_NPC_ACTION_TYPE_FLAG;
+			npc->mark_actions += 1;
 			break;
 
 		case ROC_NPC_ACTION_TYPE_COUNT:
diff --git a/drivers/common/cnxk/roc_npc.h b/drivers/common/cnxk/roc_npc.h
index 5e07e26a91..61d0628f5f 100644
--- a/drivers/common/cnxk/roc_npc.h
+++ b/drivers/common/cnxk/roc_npc.h
@@ -397,6 +397,9 @@ int __roc_api roc_npc_mcam_free_all_resources(struct roc_npc *roc_npc);
 void __roc_api roc_npc_flow_dump(FILE *file, struct roc_npc *roc_npc);
 void __roc_api roc_npc_flow_mcam_dump(FILE *file, struct roc_npc *roc_npc,
 				      struct roc_npc_flow *mcam);
+int __roc_api roc_npc_mark_actions_get(struct roc_npc *roc_npc);
+int __roc_api roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc,
+					      uint32_t count);
 int __roc_api roc_npc_vtag_actions_get(struct roc_npc *roc_npc);
 int __roc_api roc_npc_vtag_actions_sub_return(struct roc_npc *roc_npc,
 					      uint32_t count);
diff --git a/drivers/common/cnxk/roc_npc_priv.h b/drivers/common/cnxk/roc_npc_priv.h
index 08d763eeb4..714dcb09c9 100644
--- a/drivers/common/cnxk/roc_npc_priv.h
+++ b/drivers/common/cnxk/roc_npc_priv.h
@@ -393,6 +393,7 @@ struct npc {
 	uint16_t flow_prealloc_size;		/* Pre allocated mcam size */
 	uint16_t flow_max_priority;		/* Max priority for flow */
 	uint16_t switch_header_type; /* Supported switch header type */
+	uint32_t mark_actions;
 	uint32_t vtag_strip_actions; /* vtag insert/strip actions */
 	uint16_t pf_func;	     /* pf_func of device */
 	npc_dxcfg_t prx_dxcfg;	     /* intf, lid, lt, extract */
diff --git a/drivers/common/cnxk/version.map b/drivers/common/cnxk/version.map
index 5d2b75fb5a..3eff3870d1 100644
--- a/drivers/common/cnxk/version.map
+++ b/drivers/common/cnxk/version.map
@@ -344,6 +344,8 @@ INTERNAL {
 	roc_npc_flow_parse;
 	roc_npc_get_low_priority_mcam;
 	roc_npc_init;
+	roc_npc_mark_actions_get;
+	roc_npc_mark_actions_sub_return;
 	roc_npc_vtag_actions_get;
 	roc_npc_vtag_actions_sub_return;
 	roc_npc_mcam_alloc_entries;
diff --git a/drivers/net/cnxk/cn10k_ethdev.c b/drivers/net/cnxk/cn10k_ethdev.c
index b84fed6d90..512b9c2597 100644
--- a/drivers/net/cnxk/cn10k_ethdev.c
+++ b/drivers/net/cnxk/cn10k_ethdev.c
@@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
 	if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
 		flags |= NIX_RX_OFFLOAD_SECURITY_F;
 
-	if (dev->rx_mark_update)
-		flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-
 	return flags;
 }
 
@@ -562,27 +559,6 @@ cn10k_nix_dev_start(struct rte_eth_dev *eth_dev)
 	return 0;
 }
 
-static int
-cn10k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
-{
-	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
-
-	*features &=
-		(RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
-
-	if (*features) {
-		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = true;
-	} else {
-		dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = false;
-	}
-
-	cn10k_eth_set_rx_function(eth_dev);
-
-	return 0;
-}
-
 static int
 cn10k_nix_reassembly_capability_get(struct rte_eth_dev *eth_dev,
 		struct rte_eth_ip_reassembly_params *reassembly_capa)
@@ -770,8 +746,6 @@ nix_eth_dev_ops_override(void)
 	cnxk_eth_dev_ops.dev_ptypes_set = cn10k_nix_ptypes_set;
 	cnxk_eth_dev_ops.timesync_enable = cn10k_nix_timesync_enable;
 	cnxk_eth_dev_ops.timesync_disable = cn10k_nix_timesync_disable;
-	cnxk_eth_dev_ops.rx_metadata_negotiate =
-		cn10k_nix_rx_metadata_negotiate;
 	cnxk_eth_dev_ops.timesync_read_tx_timestamp =
 		cn10k_nix_timesync_read_tx_timestamp;
 	cnxk_eth_dev_ops.ip_reassembly_capability_get =
diff --git a/drivers/net/cnxk/cn10k_flow.c b/drivers/net/cnxk/cn10k_flow.c
index 2ce9e1de74..3a7b09d794 100644
--- a/drivers/net/cnxk/cn10k_flow.c
+++ b/drivers/net/cnxk/cn10k_flow.c
@@ -135,6 +135,7 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
 	struct roc_npc_flow *flow;
 	int vtag_actions = 0;
 	uint32_t req_act = 0;
+	int mark_actions = 0;
 	int i, rc;
 
 	for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) {
@@ -197,6 +198,12 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
 			cn10k_mtr_connect(eth_dev, mtr->mtr_id);
 	}
 
+	mark_actions = roc_npc_mark_actions_get(npc);
+	if (mark_actions) {
+		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
+		cn10k_eth_set_rx_function(eth_dev);
+	}
+
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 
 	if (vtag_actions) {
@@ -231,9 +238,21 @@ cn10k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
 	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
 	struct roc_npc *npc = &dev->npc;
 	int vtag_actions = 0;
+	int mark_actions = 0;
+	uint16_t match_id = 0;
 	uint32_t mtr_id;
 	int rc;
 
+	match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
+		   NPC_RX_ACT_MATCH_MASK;
+	if (match_id) {
+		mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
+		if (mark_actions == 0) {
+			dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
+			cn10k_eth_set_rx_function(eth_dev);
+		}
+	}
+
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 	if (vtag_actions) {
 		if (flow->nix_intf == ROC_NPC_INTF_RX) {
diff --git a/drivers/net/cnxk/cn9k_ethdev.c b/drivers/net/cnxk/cn9k_ethdev.c
index e5c3074d91..e55a2aa133 100644
--- a/drivers/net/cnxk/cn9k_ethdev.c
+++ b/drivers/net/cnxk/cn9k_ethdev.c
@@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
 	if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
 		flags |= NIX_RX_OFFLOAD_SECURITY_F;
 
-	if (dev->rx_mark_update)
-		flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-
 	return flags;
 }
 
@@ -541,27 +538,6 @@ cn9k_nix_timesync_read_tx_timestamp(struct rte_eth_dev *eth_dev,
 	return 0;
 }
 
-static int
-cn9k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
-{
-	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
-
-	*features &=
-		(RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
-
-	if (*features) {
-		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = true;
-	} else {
-		dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = false;
-	}
-
-	cn9k_eth_set_rx_function(eth_dev);
-
-	return 0;
-}
-
 static int
 cn9k_nix_tm_mark_vlan_dei(struct rte_eth_dev *eth_dev, int mark_green,
 			  int mark_yellow, int mark_red,
@@ -695,7 +671,6 @@ nix_eth_dev_ops_override(void)
 	cnxk_eth_dev_ops.timesync_enable = cn9k_nix_timesync_enable;
 	cnxk_eth_dev_ops.timesync_disable = cn9k_nix_timesync_disable;
 	cnxk_eth_dev_ops.mtr_ops_get = NULL;
-	cnxk_eth_dev_ops.rx_metadata_negotiate = cn9k_nix_rx_metadata_negotiate;
 	cnxk_eth_dev_ops.timesync_read_tx_timestamp =
 		cn9k_nix_timesync_read_tx_timestamp;
 }
diff --git a/drivers/net/cnxk/cn9k_flow.c b/drivers/net/cnxk/cn9k_flow.c
index a418af185d..b9ce1112a7 100644
--- a/drivers/net/cnxk/cn9k_flow.c
+++ b/drivers/net/cnxk/cn9k_flow.c
@@ -16,11 +16,19 @@ cn9k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
 	struct roc_npc *npc = &dev->npc;
 	struct roc_npc_flow *flow;
 	int vtag_actions = 0;
+	int mark_actions = 0;
 
 	flow = cnxk_flow_create(eth_dev, attr, pattern, actions, error);
 	if (!flow)
 		return NULL;
 
+	mark_actions = roc_npc_mark_actions_get(npc);
+
+	if (mark_actions) {
+		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
+		cn9k_eth_set_rx_function(eth_dev);
+	}
+
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 
 	if (vtag_actions) {
@@ -39,6 +47,18 @@ cn9k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
 	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
 	struct roc_npc *npc = &dev->npc;
 	int vtag_actions = 0;
+	uint16_t match_id = 0;
+	int mark_actions = 0;
+
+	match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
+		   NPC_RX_ACT_MATCH_MASK;
+	if (match_id) {
+		mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
+		if (mark_actions == 0) {
+			dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
+			cn9k_eth_set_rx_function(eth_dev);
+		}
+	}
 
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 	if (vtag_actions) {
diff --git a/drivers/net/cnxk/cnxk_ethdev.h b/drivers/net/cnxk/cnxk_ethdev.h
index f0eab4244c..59a06292d8 100644
--- a/drivers/net/cnxk/cnxk_ethdev.h
+++ b/drivers/net/cnxk/cnxk_ethdev.h
@@ -315,7 +315,6 @@ struct cnxk_eth_dev {
 	bool tx_compl_ena;
 	bool tx_mark;
 	bool ptp_en;
-	bool rx_mark_update; /* Enable/Disable mark update to mbuf */
 
 	/* Pointer back to rte */
 	struct rte_eth_dev *eth_dev;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v1 1/1] drivers: remove implementation of Rx metadata negotiation
  2023-03-06 12:08 [PATCH v1 1/1] drivers: remove implementation of Rx metadata negotiation Hanumanth Pothula
@ 2023-03-07  7:47 ` Jerin Jacob
  2023-03-07 10:46 ` [PATCH v2 " Hanumanth Pothula
  1 sibling, 0 replies; 4+ messages in thread
From: Jerin Jacob @ 2023-03-07  7:47 UTC (permalink / raw)
  To: Hanumanth Pothula
  Cc: Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
	dev, jerinj

On Mon, Mar 6, 2023 at 5:39 PM Hanumanth Pothula <hpothula@marvell.com> wrote:
>
> Presently, Rx metadata is sent to PMD by default, leading
> to a performance drop as processing for the same in Rx path
> takes extra cycles.
>
> Hence, removing driver implementation of Rx metadata negotiation
> and falling back to old implementation where mark actions are
> tracked as part of the flow rule.
>
> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
> ---
>  drivers/common/cnxk/roc_npc.c      | 19 +++++++++++++++++++
>  drivers/common/cnxk/roc_npc.h      |  3 +++
>  drivers/common/cnxk/roc_npc_priv.h |  1 +
>  drivers/common/cnxk/version.map    |  2 ++
>  drivers/net/cnxk/cn10k_ethdev.c    | 26 --------------------------
>  drivers/net/cnxk/cn10k_flow.c      | 19 +++++++++++++++++++
>  drivers/net/cnxk/cn9k_ethdev.c     | 25 -------------------------
>  drivers/net/cnxk/cn9k_flow.c       | 20 ++++++++++++++++++++
>  drivers/net/cnxk/cnxk_ethdev.h     |  1 -
>  9 files changed, 64 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/common/cnxk/roc_npc.c b/drivers/common/cnxk/roc_npc.c
> index a795114326..47536c8ce8 100644
> --- a/drivers/common/cnxk/roc_npc.c
> +++ b/drivers/common/cnxk/roc_npc.c
> @@ -5,6 +5,23 @@
>  #include "roc_api.h"
>  #include "roc_priv.h"
>
> +int
> +roc_npc_mark_actions_get(struct roc_npc *roc_npc)
> +{
> +       struct npc *npc = roc_npc_to_npc_priv(roc_npc);
> +
> +       return npc->mark_actions;
> +}
> +
> +int
> +roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc, uint32_t count)
> +{
> +       struct npc *npc = roc_npc_to_npc_priv(roc_npc);
> +
> +       npc->mark_actions -= count;
> +       return npc->mark_actions;
> +}
> +
>  int
>  roc_npc_vtag_actions_get(struct roc_npc *roc_npc)
>  {
> @@ -488,12 +505,14 @@ npc_parse_actions(struct roc_npc *roc_npc, const struct roc_npc_attr *attr,
>                         }
>                         mark = act_mark->id + 1;
>                         req_act |= ROC_NPC_ACTION_TYPE_MARK;
> +                       npc->mark_actions += 1;
>                         flow->match_id = mark;
>                         break;
>
>                 case ROC_NPC_ACTION_TYPE_FLAG:
>                         mark = NPC_FLOW_FLAG_VAL;
>                         req_act |= ROC_NPC_ACTION_TYPE_FLAG;
> +                       npc->mark_actions += 1;
>                         break;
>
>                 case ROC_NPC_ACTION_TYPE_COUNT:
> diff --git a/drivers/common/cnxk/roc_npc.h b/drivers/common/cnxk/roc_npc.h
> index 5e07e26a91..61d0628f5f 100644
> --- a/drivers/common/cnxk/roc_npc.h
> +++ b/drivers/common/cnxk/roc_npc.h
> @@ -397,6 +397,9 @@ int __roc_api roc_npc_mcam_free_all_resources(struct roc_npc *roc_npc);
>  void __roc_api roc_npc_flow_dump(FILE *file, struct roc_npc *roc_npc);
>  void __roc_api roc_npc_flow_mcam_dump(FILE *file, struct roc_npc *roc_npc,
>                                       struct roc_npc_flow *mcam);
> +int __roc_api roc_npc_mark_actions_get(struct roc_npc *roc_npc);
> +int __roc_api roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc,
> +                                             uint32_t count);
>  int __roc_api roc_npc_vtag_actions_get(struct roc_npc *roc_npc);
>  int __roc_api roc_npc_vtag_actions_sub_return(struct roc_npc *roc_npc,
>                                               uint32_t count);
> diff --git a/drivers/common/cnxk/roc_npc_priv.h b/drivers/common/cnxk/roc_npc_priv.h
> index 08d763eeb4..714dcb09c9 100644
> --- a/drivers/common/cnxk/roc_npc_priv.h
> +++ b/drivers/common/cnxk/roc_npc_priv.h
> @@ -393,6 +393,7 @@ struct npc {
>         uint16_t flow_prealloc_size;            /* Pre allocated mcam size */
>         uint16_t flow_max_priority;             /* Max priority for flow */
>         uint16_t switch_header_type; /* Supported switch header type */
> +       uint32_t mark_actions;
>         uint32_t vtag_strip_actions; /* vtag insert/strip actions */
>         uint16_t pf_func;            /* pf_func of device */
>         npc_dxcfg_t prx_dxcfg;       /* intf, lid, lt, extract */
> diff --git a/drivers/common/cnxk/version.map b/drivers/common/cnxk/version.map
> index 5d2b75fb5a..3eff3870d1 100644
> --- a/drivers/common/cnxk/version.map
> +++ b/drivers/common/cnxk/version.map
> @@ -344,6 +344,8 @@ INTERNAL {
>         roc_npc_flow_parse;
>         roc_npc_get_low_priority_mcam;
>         roc_npc_init;
> +       roc_npc_mark_actions_get;
> +       roc_npc_mark_actions_sub_return;
>         roc_npc_vtag_actions_get;
>         roc_npc_vtag_actions_sub_return;
>         roc_npc_mcam_alloc_entries;
> diff --git a/drivers/net/cnxk/cn10k_ethdev.c b/drivers/net/cnxk/cn10k_ethdev.c
> index b84fed6d90..512b9c2597 100644
> --- a/drivers/net/cnxk/cn10k_ethdev.c
> +++ b/drivers/net/cnxk/cn10k_ethdev.c
> @@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
>         if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
>                 flags |= NIX_RX_OFFLOAD_SECURITY_F;
>
> -       if (dev->rx_mark_update)
> -               flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -
>         return flags;
>  }
>
> @@ -562,27 +559,6 @@ cn10k_nix_dev_start(struct rte_eth_dev *eth_dev)
>         return 0;
>  }
>
> -static int
> -cn10k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
> -{
> -       struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> -
> -       *features &=
> -               (RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
> -
> -       if (*features) {
> -               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = true;
> -       } else {
> -               dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = false;
> -       }
> -
> -       cn10k_eth_set_rx_function(eth_dev);
> -
> -       return 0;
> -}
> -
>  static int
>  cn10k_nix_reassembly_capability_get(struct rte_eth_dev *eth_dev,
>                 struct rte_eth_ip_reassembly_params *reassembly_capa)
> @@ -770,8 +746,6 @@ nix_eth_dev_ops_override(void)
>         cnxk_eth_dev_ops.dev_ptypes_set = cn10k_nix_ptypes_set;
>         cnxk_eth_dev_ops.timesync_enable = cn10k_nix_timesync_enable;
>         cnxk_eth_dev_ops.timesync_disable = cn10k_nix_timesync_disable;
> -       cnxk_eth_dev_ops.rx_metadata_negotiate =
> -               cn10k_nix_rx_metadata_negotiate;
>         cnxk_eth_dev_ops.timesync_read_tx_timestamp =
>                 cn10k_nix_timesync_read_tx_timestamp;
>         cnxk_eth_dev_ops.ip_reassembly_capability_get =
> diff --git a/drivers/net/cnxk/cn10k_flow.c b/drivers/net/cnxk/cn10k_flow.c
> index 2ce9e1de74..3a7b09d794 100644
> --- a/drivers/net/cnxk/cn10k_flow.c
> +++ b/drivers/net/cnxk/cn10k_flow.c
> @@ -135,6 +135,7 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
>         struct roc_npc_flow *flow;
>         int vtag_actions = 0;
>         uint32_t req_act = 0;
> +       int mark_actions = 0;

Is explict 0 assigmenet needed? Please check in other place too?

>         int i, rc;
>
>         for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) {
> @@ -197,6 +198,12 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
>                         cn10k_mtr_connect(eth_dev, mtr->mtr_id);
>         }
>
> +       mark_actions = roc_npc_mark_actions_get(npc);
> +       if (mark_actions) {
> +               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +               cn10k_eth_set_rx_function(eth_dev);
> +       }
> +
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>
>         if (vtag_actions) {
> @@ -231,9 +238,21 @@ cn10k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
>         struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
>         struct roc_npc *npc = &dev->npc;
>         int vtag_actions = 0;
> +       int mark_actions = 0;
> +       uint16_t match_id = 0;

Same as above.

>         uint32_t mtr_id;
>         int rc;
>
> +       match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
> +                  NPC_RX_ACT_MATCH_MASK;
> +       if (match_id) {
> +               mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
> +               if (mark_actions == 0) {
> +                       dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +                       cn10k_eth_set_rx_function(eth_dev);
> +               }
> +       }
> +
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>         if (vtag_actions) {
>                 if (flow->nix_intf == ROC_NPC_INTF_RX) {
> diff --git a/drivers/net/cnxk/cn9k_ethdev.c b/drivers/net/cnxk/cn9k_ethdev.c
> index e5c3074d91..e55a2aa133 100644
> --- a/drivers/net/cnxk/cn9k_ethdev.c
> +++ b/drivers/net/cnxk/cn9k_ethdev.c
> @@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
>         if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
>                 flags |= NIX_RX_OFFLOAD_SECURITY_F;
>
> -       if (dev->rx_mark_update)
> -               flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -
>         return flags;
>  }
>
> @@ -541,27 +538,6 @@ cn9k_nix_timesync_read_tx_timestamp(struct rte_eth_dev *eth_dev,
>         return 0;
>  }
>
> -static int
> -cn9k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
> -{
> -       struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> -
> -       *features &=
> -               (RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
> -
> -       if (*features) {
> -               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = true;
> -       } else {
> -               dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = false;
> -       }
> -
> -       cn9k_eth_set_rx_function(eth_dev);
> -
> -       return 0;
> -}
> -
>  static int
>  cn9k_nix_tm_mark_vlan_dei(struct rte_eth_dev *eth_dev, int mark_green,
>                           int mark_yellow, int mark_red,
> @@ -695,7 +671,6 @@ nix_eth_dev_ops_override(void)
>         cnxk_eth_dev_ops.timesync_enable = cn9k_nix_timesync_enable;
>         cnxk_eth_dev_ops.timesync_disable = cn9k_nix_timesync_disable;
>         cnxk_eth_dev_ops.mtr_ops_get = NULL;
> -       cnxk_eth_dev_ops.rx_metadata_negotiate = cn9k_nix_rx_metadata_negotiate;
>         cnxk_eth_dev_ops.timesync_read_tx_timestamp =
>                 cn9k_nix_timesync_read_tx_timestamp;
>  }
> diff --git a/drivers/net/cnxk/cn9k_flow.c b/drivers/net/cnxk/cn9k_flow.c
> index a418af185d..b9ce1112a7 100644
> --- a/drivers/net/cnxk/cn9k_flow.c
> +++ b/drivers/net/cnxk/cn9k_flow.c
> @@ -16,11 +16,19 @@ cn9k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
>         struct roc_npc *npc = &dev->npc;
>         struct roc_npc_flow *flow;
>         int vtag_actions = 0;
> +       int mark_actions = 0;

Same as above.

>
>         flow = cnxk_flow_create(eth_dev, attr, pattern, actions, error);
>         if (!flow)
>                 return NULL;
>
> +       mark_actions = roc_npc_mark_actions_get(npc);
> +
> +       if (mark_actions) {
> +               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +               cn9k_eth_set_rx_function(eth_dev);
> +       }
> +
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>
>         if (vtag_actions) {
> @@ -39,6 +47,18 @@ cn9k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
>         struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
>         struct roc_npc *npc = &dev->npc;
>         int vtag_actions = 0;
> +       uint16_t match_id = 0;
> +       int mark_actions = 0;
> +
> +       match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
> +                  NPC_RX_ACT_MATCH_MASK;
> +       if (match_id) {
> +               mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
> +               if (mark_actions == 0) {
> +                       dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +                       cn9k_eth_set_rx_function(eth_dev);
> +               }
> +       }
>
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>         if (vtag_actions) {
> diff --git a/drivers/net/cnxk/cnxk_ethdev.h b/drivers/net/cnxk/cnxk_ethdev.h
> index f0eab4244c..59a06292d8 100644
> --- a/drivers/net/cnxk/cnxk_ethdev.h
> +++ b/drivers/net/cnxk/cnxk_ethdev.h
> @@ -315,7 +315,6 @@ struct cnxk_eth_dev {
>         bool tx_compl_ena;
>         bool tx_mark;
>         bool ptp_en;
> -       bool rx_mark_update; /* Enable/Disable mark update to mbuf */
>
>         /* Pointer back to rte */
>         struct rte_eth_dev *eth_dev;
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2 1/1] drivers: remove implementation of Rx metadata negotiation
  2023-03-06 12:08 [PATCH v1 1/1] drivers: remove implementation of Rx metadata negotiation Hanumanth Pothula
  2023-03-07  7:47 ` Jerin Jacob
@ 2023-03-07 10:46 ` Hanumanth Pothula
  2023-03-08  9:31   ` Jerin Jacob
  1 sibling, 1 reply; 4+ messages in thread
From: Hanumanth Pothula @ 2023-03-07 10:46 UTC (permalink / raw)
  To: Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao
  Cc: dev, jerinj, hpothula

Presently, Rx metadata is sent to PMD by default, leading
to a performance drop as processing for the same in Rx path
takes extra cycles.

Hence, removing driver implementation of Rx metadata negotiation
and falling back to old implementation where mark actions are
tracked as part of the flow rule.

Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
---
v2: Remove explicit initializations.
---
 drivers/common/cnxk/roc_npc.c      | 19 +++++++++++++++++++
 drivers/common/cnxk/roc_npc.h      |  3 +++
 drivers/common/cnxk/roc_npc_priv.h |  1 +
 drivers/common/cnxk/version.map    |  2 ++
 drivers/net/cnxk/cn10k_ethdev.c    | 26 --------------------------
 drivers/net/cnxk/cn10k_flow.c      | 19 +++++++++++++++++++
 drivers/net/cnxk/cn9k_ethdev.c     | 25 -------------------------
 drivers/net/cnxk/cn9k_flow.c       | 20 ++++++++++++++++++++
 drivers/net/cnxk/cnxk_ethdev.h     |  1 -
 9 files changed, 64 insertions(+), 52 deletions(-)

diff --git a/drivers/common/cnxk/roc_npc.c b/drivers/common/cnxk/roc_npc.c
index a795114326..47536c8ce8 100644
--- a/drivers/common/cnxk/roc_npc.c
+++ b/drivers/common/cnxk/roc_npc.c
@@ -5,6 +5,23 @@
 #include "roc_api.h"
 #include "roc_priv.h"
 
+int
+roc_npc_mark_actions_get(struct roc_npc *roc_npc)
+{
+	struct npc *npc = roc_npc_to_npc_priv(roc_npc);
+
+	return npc->mark_actions;
+}
+
+int
+roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc, uint32_t count)
+{
+	struct npc *npc = roc_npc_to_npc_priv(roc_npc);
+
+	npc->mark_actions -= count;
+	return npc->mark_actions;
+}
+
 int
 roc_npc_vtag_actions_get(struct roc_npc *roc_npc)
 {
@@ -488,12 +505,14 @@ npc_parse_actions(struct roc_npc *roc_npc, const struct roc_npc_attr *attr,
 			}
 			mark = act_mark->id + 1;
 			req_act |= ROC_NPC_ACTION_TYPE_MARK;
+			npc->mark_actions += 1;
 			flow->match_id = mark;
 			break;
 
 		case ROC_NPC_ACTION_TYPE_FLAG:
 			mark = NPC_FLOW_FLAG_VAL;
 			req_act |= ROC_NPC_ACTION_TYPE_FLAG;
+			npc->mark_actions += 1;
 			break;
 
 		case ROC_NPC_ACTION_TYPE_COUNT:
diff --git a/drivers/common/cnxk/roc_npc.h b/drivers/common/cnxk/roc_npc.h
index 5e07e26a91..61d0628f5f 100644
--- a/drivers/common/cnxk/roc_npc.h
+++ b/drivers/common/cnxk/roc_npc.h
@@ -397,6 +397,9 @@ int __roc_api roc_npc_mcam_free_all_resources(struct roc_npc *roc_npc);
 void __roc_api roc_npc_flow_dump(FILE *file, struct roc_npc *roc_npc);
 void __roc_api roc_npc_flow_mcam_dump(FILE *file, struct roc_npc *roc_npc,
 				      struct roc_npc_flow *mcam);
+int __roc_api roc_npc_mark_actions_get(struct roc_npc *roc_npc);
+int __roc_api roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc,
+					      uint32_t count);
 int __roc_api roc_npc_vtag_actions_get(struct roc_npc *roc_npc);
 int __roc_api roc_npc_vtag_actions_sub_return(struct roc_npc *roc_npc,
 					      uint32_t count);
diff --git a/drivers/common/cnxk/roc_npc_priv.h b/drivers/common/cnxk/roc_npc_priv.h
index 08d763eeb4..714dcb09c9 100644
--- a/drivers/common/cnxk/roc_npc_priv.h
+++ b/drivers/common/cnxk/roc_npc_priv.h
@@ -393,6 +393,7 @@ struct npc {
 	uint16_t flow_prealloc_size;		/* Pre allocated mcam size */
 	uint16_t flow_max_priority;		/* Max priority for flow */
 	uint16_t switch_header_type; /* Supported switch header type */
+	uint32_t mark_actions;
 	uint32_t vtag_strip_actions; /* vtag insert/strip actions */
 	uint16_t pf_func;	     /* pf_func of device */
 	npc_dxcfg_t prx_dxcfg;	     /* intf, lid, lt, extract */
diff --git a/drivers/common/cnxk/version.map b/drivers/common/cnxk/version.map
index 5d2b75fb5a..3eff3870d1 100644
--- a/drivers/common/cnxk/version.map
+++ b/drivers/common/cnxk/version.map
@@ -344,6 +344,8 @@ INTERNAL {
 	roc_npc_flow_parse;
 	roc_npc_get_low_priority_mcam;
 	roc_npc_init;
+	roc_npc_mark_actions_get;
+	roc_npc_mark_actions_sub_return;
 	roc_npc_vtag_actions_get;
 	roc_npc_vtag_actions_sub_return;
 	roc_npc_mcam_alloc_entries;
diff --git a/drivers/net/cnxk/cn10k_ethdev.c b/drivers/net/cnxk/cn10k_ethdev.c
index b84fed6d90..512b9c2597 100644
--- a/drivers/net/cnxk/cn10k_ethdev.c
+++ b/drivers/net/cnxk/cn10k_ethdev.c
@@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
 	if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
 		flags |= NIX_RX_OFFLOAD_SECURITY_F;
 
-	if (dev->rx_mark_update)
-		flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-
 	return flags;
 }
 
@@ -562,27 +559,6 @@ cn10k_nix_dev_start(struct rte_eth_dev *eth_dev)
 	return 0;
 }
 
-static int
-cn10k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
-{
-	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
-
-	*features &=
-		(RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
-
-	if (*features) {
-		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = true;
-	} else {
-		dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = false;
-	}
-
-	cn10k_eth_set_rx_function(eth_dev);
-
-	return 0;
-}
-
 static int
 cn10k_nix_reassembly_capability_get(struct rte_eth_dev *eth_dev,
 		struct rte_eth_ip_reassembly_params *reassembly_capa)
@@ -770,8 +746,6 @@ nix_eth_dev_ops_override(void)
 	cnxk_eth_dev_ops.dev_ptypes_set = cn10k_nix_ptypes_set;
 	cnxk_eth_dev_ops.timesync_enable = cn10k_nix_timesync_enable;
 	cnxk_eth_dev_ops.timesync_disable = cn10k_nix_timesync_disable;
-	cnxk_eth_dev_ops.rx_metadata_negotiate =
-		cn10k_nix_rx_metadata_negotiate;
 	cnxk_eth_dev_ops.timesync_read_tx_timestamp =
 		cn10k_nix_timesync_read_tx_timestamp;
 	cnxk_eth_dev_ops.ip_reassembly_capability_get =
diff --git a/drivers/net/cnxk/cn10k_flow.c b/drivers/net/cnxk/cn10k_flow.c
index 2ce9e1de74..d7a3442c5f 100644
--- a/drivers/net/cnxk/cn10k_flow.c
+++ b/drivers/net/cnxk/cn10k_flow.c
@@ -135,6 +135,7 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
 	struct roc_npc_flow *flow;
 	int vtag_actions = 0;
 	uint32_t req_act = 0;
+	int mark_actions;
 	int i, rc;
 
 	for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) {
@@ -197,6 +198,12 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
 			cn10k_mtr_connect(eth_dev, mtr->mtr_id);
 	}
 
+	mark_actions = roc_npc_mark_actions_get(npc);
+	if (mark_actions) {
+		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
+		cn10k_eth_set_rx_function(eth_dev);
+	}
+
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 
 	if (vtag_actions) {
@@ -231,9 +238,21 @@ cn10k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
 	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
 	struct roc_npc *npc = &dev->npc;
 	int vtag_actions = 0;
+	int mark_actions;
+	uint16_t match_id;
 	uint32_t mtr_id;
 	int rc;
 
+	match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
+		   NPC_RX_ACT_MATCH_MASK;
+	if (match_id) {
+		mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
+		if (mark_actions == 0) {
+			dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
+			cn10k_eth_set_rx_function(eth_dev);
+		}
+	}
+
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 	if (vtag_actions) {
 		if (flow->nix_intf == ROC_NPC_INTF_RX) {
diff --git a/drivers/net/cnxk/cn9k_ethdev.c b/drivers/net/cnxk/cn9k_ethdev.c
index e5c3074d91..e55a2aa133 100644
--- a/drivers/net/cnxk/cn9k_ethdev.c
+++ b/drivers/net/cnxk/cn9k_ethdev.c
@@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
 	if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
 		flags |= NIX_RX_OFFLOAD_SECURITY_F;
 
-	if (dev->rx_mark_update)
-		flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-
 	return flags;
 }
 
@@ -541,27 +538,6 @@ cn9k_nix_timesync_read_tx_timestamp(struct rte_eth_dev *eth_dev,
 	return 0;
 }
 
-static int
-cn9k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
-{
-	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
-
-	*features &=
-		(RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
-
-	if (*features) {
-		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = true;
-	} else {
-		dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
-		dev->rx_mark_update = false;
-	}
-
-	cn9k_eth_set_rx_function(eth_dev);
-
-	return 0;
-}
-
 static int
 cn9k_nix_tm_mark_vlan_dei(struct rte_eth_dev *eth_dev, int mark_green,
 			  int mark_yellow, int mark_red,
@@ -695,7 +671,6 @@ nix_eth_dev_ops_override(void)
 	cnxk_eth_dev_ops.timesync_enable = cn9k_nix_timesync_enable;
 	cnxk_eth_dev_ops.timesync_disable = cn9k_nix_timesync_disable;
 	cnxk_eth_dev_ops.mtr_ops_get = NULL;
-	cnxk_eth_dev_ops.rx_metadata_negotiate = cn9k_nix_rx_metadata_negotiate;
 	cnxk_eth_dev_ops.timesync_read_tx_timestamp =
 		cn9k_nix_timesync_read_tx_timestamp;
 }
diff --git a/drivers/net/cnxk/cn9k_flow.c b/drivers/net/cnxk/cn9k_flow.c
index a418af185d..dc5476a189 100644
--- a/drivers/net/cnxk/cn9k_flow.c
+++ b/drivers/net/cnxk/cn9k_flow.c
@@ -16,11 +16,19 @@ cn9k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
 	struct roc_npc *npc = &dev->npc;
 	struct roc_npc_flow *flow;
 	int vtag_actions = 0;
+	int mark_actions;
 
 	flow = cnxk_flow_create(eth_dev, attr, pattern, actions, error);
 	if (!flow)
 		return NULL;
 
+	mark_actions = roc_npc_mark_actions_get(npc);
+
+	if (mark_actions) {
+		dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
+		cn9k_eth_set_rx_function(eth_dev);
+	}
+
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 
 	if (vtag_actions) {
@@ -39,6 +47,18 @@ cn9k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
 	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
 	struct roc_npc *npc = &dev->npc;
 	int vtag_actions = 0;
+	uint16_t match_id;
+	int mark_actions;
+
+	match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
+		   NPC_RX_ACT_MATCH_MASK;
+	if (match_id) {
+		mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
+		if (mark_actions == 0) {
+			dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
+			cn9k_eth_set_rx_function(eth_dev);
+		}
+	}
 
 	vtag_actions = roc_npc_vtag_actions_get(npc);
 	if (vtag_actions) {
diff --git a/drivers/net/cnxk/cnxk_ethdev.h b/drivers/net/cnxk/cnxk_ethdev.h
index f0eab4244c..59a06292d8 100644
--- a/drivers/net/cnxk/cnxk_ethdev.h
+++ b/drivers/net/cnxk/cnxk_ethdev.h
@@ -315,7 +315,6 @@ struct cnxk_eth_dev {
 	bool tx_compl_ena;
 	bool tx_mark;
 	bool ptp_en;
-	bool rx_mark_update; /* Enable/Disable mark update to mbuf */
 
 	/* Pointer back to rte */
 	struct rte_eth_dev *eth_dev;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 1/1] drivers: remove implementation of Rx metadata negotiation
  2023-03-07 10:46 ` [PATCH v2 " Hanumanth Pothula
@ 2023-03-08  9:31   ` Jerin Jacob
  0 siblings, 0 replies; 4+ messages in thread
From: Jerin Jacob @ 2023-03-08  9:31 UTC (permalink / raw)
  To: Hanumanth Pothula
  Cc: Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
	dev, jerinj

On Tue, Mar 7, 2023 at 4:16 PM Hanumanth Pothula <hpothula@marvell.com> wrote:
>
> Presently, Rx metadata is sent to PMD by default, leading
> to a performance drop as processing for the same in Rx path
> takes extra cycles.
>
> Hence, removing driver implementation of Rx metadata negotiation
> and falling back to old implementation where mark actions are
> tracked as part of the flow rule.
>
> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>


Updated the git commit as follows and applied to
dpdk-next-net-mrvl/for-next-net. Thanks

net/cnxk: remove Rx metadata negotiation

> ---
> v2: Remove explicit initializations.
> ---
>  drivers/common/cnxk/roc_npc.c      | 19 +++++++++++++++++++
>  drivers/common/cnxk/roc_npc.h      |  3 +++
>  drivers/common/cnxk/roc_npc_priv.h |  1 +
>  drivers/common/cnxk/version.map    |  2 ++
>  drivers/net/cnxk/cn10k_ethdev.c    | 26 --------------------------
>  drivers/net/cnxk/cn10k_flow.c      | 19 +++++++++++++++++++
>  drivers/net/cnxk/cn9k_ethdev.c     | 25 -------------------------
>  drivers/net/cnxk/cn9k_flow.c       | 20 ++++++++++++++++++++
>  drivers/net/cnxk/cnxk_ethdev.h     |  1 -
>  9 files changed, 64 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/common/cnxk/roc_npc.c b/drivers/common/cnxk/roc_npc.c
> index a795114326..47536c8ce8 100644
> --- a/drivers/common/cnxk/roc_npc.c
> +++ b/drivers/common/cnxk/roc_npc.c
> @@ -5,6 +5,23 @@
>  #include "roc_api.h"
>  #include "roc_priv.h"
>
> +int
> +roc_npc_mark_actions_get(struct roc_npc *roc_npc)
> +{
> +       struct npc *npc = roc_npc_to_npc_priv(roc_npc);
> +
> +       return npc->mark_actions;
> +}
> +
> +int
> +roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc, uint32_t count)
> +{
> +       struct npc *npc = roc_npc_to_npc_priv(roc_npc);
> +
> +       npc->mark_actions -= count;
> +       return npc->mark_actions;
> +}
> +
>  int
>  roc_npc_vtag_actions_get(struct roc_npc *roc_npc)
>  {
> @@ -488,12 +505,14 @@ npc_parse_actions(struct roc_npc *roc_npc, const struct roc_npc_attr *attr,
>                         }
>                         mark = act_mark->id + 1;
>                         req_act |= ROC_NPC_ACTION_TYPE_MARK;
> +                       npc->mark_actions += 1;
>                         flow->match_id = mark;
>                         break;
>
>                 case ROC_NPC_ACTION_TYPE_FLAG:
>                         mark = NPC_FLOW_FLAG_VAL;
>                         req_act |= ROC_NPC_ACTION_TYPE_FLAG;
> +                       npc->mark_actions += 1;
>                         break;
>
>                 case ROC_NPC_ACTION_TYPE_COUNT:
> diff --git a/drivers/common/cnxk/roc_npc.h b/drivers/common/cnxk/roc_npc.h
> index 5e07e26a91..61d0628f5f 100644
> --- a/drivers/common/cnxk/roc_npc.h
> +++ b/drivers/common/cnxk/roc_npc.h
> @@ -397,6 +397,9 @@ int __roc_api roc_npc_mcam_free_all_resources(struct roc_npc *roc_npc);
>  void __roc_api roc_npc_flow_dump(FILE *file, struct roc_npc *roc_npc);
>  void __roc_api roc_npc_flow_mcam_dump(FILE *file, struct roc_npc *roc_npc,
>                                       struct roc_npc_flow *mcam);
> +int __roc_api roc_npc_mark_actions_get(struct roc_npc *roc_npc);
> +int __roc_api roc_npc_mark_actions_sub_return(struct roc_npc *roc_npc,
> +                                             uint32_t count);
>  int __roc_api roc_npc_vtag_actions_get(struct roc_npc *roc_npc);
>  int __roc_api roc_npc_vtag_actions_sub_return(struct roc_npc *roc_npc,
>                                               uint32_t count);
> diff --git a/drivers/common/cnxk/roc_npc_priv.h b/drivers/common/cnxk/roc_npc_priv.h
> index 08d763eeb4..714dcb09c9 100644
> --- a/drivers/common/cnxk/roc_npc_priv.h
> +++ b/drivers/common/cnxk/roc_npc_priv.h
> @@ -393,6 +393,7 @@ struct npc {
>         uint16_t flow_prealloc_size;            /* Pre allocated mcam size */
>         uint16_t flow_max_priority;             /* Max priority for flow */
>         uint16_t switch_header_type; /* Supported switch header type */
> +       uint32_t mark_actions;
>         uint32_t vtag_strip_actions; /* vtag insert/strip actions */
>         uint16_t pf_func;            /* pf_func of device */
>         npc_dxcfg_t prx_dxcfg;       /* intf, lid, lt, extract */
> diff --git a/drivers/common/cnxk/version.map b/drivers/common/cnxk/version.map
> index 5d2b75fb5a..3eff3870d1 100644
> --- a/drivers/common/cnxk/version.map
> +++ b/drivers/common/cnxk/version.map
> @@ -344,6 +344,8 @@ INTERNAL {
>         roc_npc_flow_parse;
>         roc_npc_get_low_priority_mcam;
>         roc_npc_init;
> +       roc_npc_mark_actions_get;
> +       roc_npc_mark_actions_sub_return;
>         roc_npc_vtag_actions_get;
>         roc_npc_vtag_actions_sub_return;
>         roc_npc_mcam_alloc_entries;
> diff --git a/drivers/net/cnxk/cn10k_ethdev.c b/drivers/net/cnxk/cn10k_ethdev.c
> index b84fed6d90..512b9c2597 100644
> --- a/drivers/net/cnxk/cn10k_ethdev.c
> +++ b/drivers/net/cnxk/cn10k_ethdev.c
> @@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
>         if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
>                 flags |= NIX_RX_OFFLOAD_SECURITY_F;
>
> -       if (dev->rx_mark_update)
> -               flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -
>         return flags;
>  }
>
> @@ -562,27 +559,6 @@ cn10k_nix_dev_start(struct rte_eth_dev *eth_dev)
>         return 0;
>  }
>
> -static int
> -cn10k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
> -{
> -       struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> -
> -       *features &=
> -               (RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
> -
> -       if (*features) {
> -               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = true;
> -       } else {
> -               dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = false;
> -       }
> -
> -       cn10k_eth_set_rx_function(eth_dev);
> -
> -       return 0;
> -}
> -
>  static int
>  cn10k_nix_reassembly_capability_get(struct rte_eth_dev *eth_dev,
>                 struct rte_eth_ip_reassembly_params *reassembly_capa)
> @@ -770,8 +746,6 @@ nix_eth_dev_ops_override(void)
>         cnxk_eth_dev_ops.dev_ptypes_set = cn10k_nix_ptypes_set;
>         cnxk_eth_dev_ops.timesync_enable = cn10k_nix_timesync_enable;
>         cnxk_eth_dev_ops.timesync_disable = cn10k_nix_timesync_disable;
> -       cnxk_eth_dev_ops.rx_metadata_negotiate =
> -               cn10k_nix_rx_metadata_negotiate;
>         cnxk_eth_dev_ops.timesync_read_tx_timestamp =
>                 cn10k_nix_timesync_read_tx_timestamp;
>         cnxk_eth_dev_ops.ip_reassembly_capability_get =
> diff --git a/drivers/net/cnxk/cn10k_flow.c b/drivers/net/cnxk/cn10k_flow.c
> index 2ce9e1de74..d7a3442c5f 100644
> --- a/drivers/net/cnxk/cn10k_flow.c
> +++ b/drivers/net/cnxk/cn10k_flow.c
> @@ -135,6 +135,7 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
>         struct roc_npc_flow *flow;
>         int vtag_actions = 0;
>         uint32_t req_act = 0;
> +       int mark_actions;
>         int i, rc;
>
>         for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) {
> @@ -197,6 +198,12 @@ cn10k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
>                         cn10k_mtr_connect(eth_dev, mtr->mtr_id);
>         }
>
> +       mark_actions = roc_npc_mark_actions_get(npc);
> +       if (mark_actions) {
> +               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +               cn10k_eth_set_rx_function(eth_dev);
> +       }
> +
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>
>         if (vtag_actions) {
> @@ -231,9 +238,21 @@ cn10k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
>         struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
>         struct roc_npc *npc = &dev->npc;
>         int vtag_actions = 0;
> +       int mark_actions;
> +       uint16_t match_id;
>         uint32_t mtr_id;
>         int rc;
>
> +       match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
> +                  NPC_RX_ACT_MATCH_MASK;
> +       if (match_id) {
> +               mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
> +               if (mark_actions == 0) {
> +                       dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +                       cn10k_eth_set_rx_function(eth_dev);
> +               }
> +       }
> +
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>         if (vtag_actions) {
>                 if (flow->nix_intf == ROC_NPC_INTF_RX) {
> diff --git a/drivers/net/cnxk/cn9k_ethdev.c b/drivers/net/cnxk/cn9k_ethdev.c
> index e5c3074d91..e55a2aa133 100644
> --- a/drivers/net/cnxk/cn9k_ethdev.c
> +++ b/drivers/net/cnxk/cn9k_ethdev.c
> @@ -39,9 +39,6 @@ nix_rx_offload_flags(struct rte_eth_dev *eth_dev)
>         if (dev->rx_offloads & RTE_ETH_RX_OFFLOAD_SECURITY)
>                 flags |= NIX_RX_OFFLOAD_SECURITY_F;
>
> -       if (dev->rx_mark_update)
> -               flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -
>         return flags;
>  }
>
> @@ -541,27 +538,6 @@ cn9k_nix_timesync_read_tx_timestamp(struct rte_eth_dev *eth_dev,
>         return 0;
>  }
>
> -static int
> -cn9k_nix_rx_metadata_negotiate(struct rte_eth_dev *eth_dev, uint64_t *features)
> -{
> -       struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> -
> -       *features &=
> -               (RTE_ETH_RX_METADATA_USER_FLAG | RTE_ETH_RX_METADATA_USER_MARK);
> -
> -       if (*features) {
> -               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = true;
> -       } else {
> -               dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> -               dev->rx_mark_update = false;
> -       }
> -
> -       cn9k_eth_set_rx_function(eth_dev);
> -
> -       return 0;
> -}
> -
>  static int
>  cn9k_nix_tm_mark_vlan_dei(struct rte_eth_dev *eth_dev, int mark_green,
>                           int mark_yellow, int mark_red,
> @@ -695,7 +671,6 @@ nix_eth_dev_ops_override(void)
>         cnxk_eth_dev_ops.timesync_enable = cn9k_nix_timesync_enable;
>         cnxk_eth_dev_ops.timesync_disable = cn9k_nix_timesync_disable;
>         cnxk_eth_dev_ops.mtr_ops_get = NULL;
> -       cnxk_eth_dev_ops.rx_metadata_negotiate = cn9k_nix_rx_metadata_negotiate;
>         cnxk_eth_dev_ops.timesync_read_tx_timestamp =
>                 cn9k_nix_timesync_read_tx_timestamp;
>  }
> diff --git a/drivers/net/cnxk/cn9k_flow.c b/drivers/net/cnxk/cn9k_flow.c
> index a418af185d..dc5476a189 100644
> --- a/drivers/net/cnxk/cn9k_flow.c
> +++ b/drivers/net/cnxk/cn9k_flow.c
> @@ -16,11 +16,19 @@ cn9k_flow_create(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr,
>         struct roc_npc *npc = &dev->npc;
>         struct roc_npc_flow *flow;
>         int vtag_actions = 0;
> +       int mark_actions;
>
>         flow = cnxk_flow_create(eth_dev, attr, pattern, actions, error);
>         if (!flow)
>                 return NULL;
>
> +       mark_actions = roc_npc_mark_actions_get(npc);
> +
> +       if (mark_actions) {
> +               dev->rx_offload_flags |= NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +               cn9k_eth_set_rx_function(eth_dev);
> +       }
> +
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>
>         if (vtag_actions) {
> @@ -39,6 +47,18 @@ cn9k_flow_destroy(struct rte_eth_dev *eth_dev, struct rte_flow *rte_flow,
>         struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
>         struct roc_npc *npc = &dev->npc;
>         int vtag_actions = 0;
> +       uint16_t match_id;
> +       int mark_actions;
> +
> +       match_id = (flow->npc_action >> NPC_RX_ACT_MATCH_OFFSET) &
> +                  NPC_RX_ACT_MATCH_MASK;
> +       if (match_id) {
> +               mark_actions = roc_npc_mark_actions_sub_return(npc, 1);
> +               if (mark_actions == 0) {
> +                       dev->rx_offload_flags &= ~NIX_RX_OFFLOAD_MARK_UPDATE_F;
> +                       cn9k_eth_set_rx_function(eth_dev);
> +               }
> +       }
>
>         vtag_actions = roc_npc_vtag_actions_get(npc);
>         if (vtag_actions) {
> diff --git a/drivers/net/cnxk/cnxk_ethdev.h b/drivers/net/cnxk/cnxk_ethdev.h
> index f0eab4244c..59a06292d8 100644
> --- a/drivers/net/cnxk/cnxk_ethdev.h
> +++ b/drivers/net/cnxk/cnxk_ethdev.h
> @@ -315,7 +315,6 @@ struct cnxk_eth_dev {
>         bool tx_compl_ena;
>         bool tx_mark;
>         bool ptp_en;
> -       bool rx_mark_update; /* Enable/Disable mark update to mbuf */
>
>         /* Pointer back to rte */
>         struct rte_eth_dev *eth_dev;
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-03-08  9:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-06 12:08 [PATCH v1 1/1] drivers: remove implementation of Rx metadata negotiation Hanumanth Pothula
2023-03-07  7:47 ` Jerin Jacob
2023-03-07 10:46 ` [PATCH v2 " Hanumanth Pothula
2023-03-08  9:31   ` Jerin Jacob

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).