[RFC] net/mlx5: support count action in shared action API

Message ID 1615385863-30749-1-git-send-email-michaelba@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Raslan Darawsheh
Headers
Series [RFC] net/mlx5: support count action in shared action API |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Michael Baum March 10, 2021, 2:17 p.m. UTC
  Existing API supports counter action to count traffic of a single flow.
The user could use the shared flag to share the action between different
flows. The user gives an id for counter action in creating the flow, and
when he wants to share it with another flow, he uses the same id.

Recent patch [1] introduced the shared action API.
Using this API, an action can be created as shared, unattached to any
flow rule.
Multiple flows can then be created using the shared action.
The new API also supports query operation of a shared action.

The new API is more efficient because PMD selects the id for the user,
thus saving the conversion of the user's id to its id.

Following this RFC, the MLX5 PMD will implement support of shared count
action.
This new feature will support validate, create, query and destroy
operations.

Application will use the shared action query operation to query this
count action.
The response will be returned In the same existing query count format.

In the meantime the old sharing mechanism (with the sharing flag)
continues to be supported, and the user can choose the way he wants to
share the counter.
The new shared action API is only supported in DevX, so sharing counter
action in Verbs can only be done through the old mechanism.

[1] https://mails.dpdk.org/archives/dev/2020-July/174110.html

Signed-off-by: Michael Baum <michaelba@nvidia.com>
---
 drivers/net/mlx5/mlx5.h         |   5 +-
 drivers/net/mlx5/mlx5_defs.h    |   2 +-
 drivers/net/mlx5/mlx5_flow.c    |   6 ++
 drivers/net/mlx5/mlx5_flow.h    |   2 +
 drivers/net/mlx5/mlx5_flow_dv.c | 218 ++++++++++++++++++++++++++++++----------
 5 files changed, 176 insertions(+), 57 deletions(-)
  

Patch

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index a281fd2..447767b 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -341,7 +341,10 @@  struct flow_counter_stats {
 
 /* Shared counters information for counters. */
 struct mlx5_flow_counter_shared {
-	uint32_t id; /**< User counter ID. */
+	union {
+		uint32_t refcnt; /* Count action reference counter. */
+		uint32_t id; /* User counter ID. */
+	};
 };
 
 /* Shared counter configuration. */
diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index af29d93..8e45f55 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -197,7 +197,7 @@ 
 #define MLX5_HAIRPIN_JUMBO_LOG_SIZE (14 + 2)
 
 /* Maximum number of shared actions supported by rte_flow */
-#define MLX5_MAX_SHARED_ACTIONS 2
+#define MLX5_MAX_SHARED_ACTIONS 3
 
 /*
  * Linux definition of static_assert is found in /usr/include/assert.h.
diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index ab5be3d..6b5889a 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -3498,6 +3498,12 @@  struct mlx5_translated_shared_action {
 			translated[shared->index].conf =
 				&shared_rss->origin;
 			break;
+		case MLX5_SHARED_ACTION_TYPE_COUNT:
+			translated[shared->index].type =
+						(enum rte_flow_action_type)
+						MLX5_RTE_FLOW_ACTION_TYPE_COUNT;
+			translated[shared->index].conf = (void *)(uintptr_t)idx;
+			break;
 		case MLX5_SHARED_ACTION_TYPE_AGE:
 			if (priv->sh->flow_hit_aso_en) {
 				translated[shared->index].type =
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 8324e18..16659be 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -37,6 +37,7 @@  enum mlx5_rte_flow_action_type {
 	MLX5_RTE_FLOW_ACTION_TYPE_DEFAULT_MISS,
 	MLX5_RTE_FLOW_ACTION_TYPE_TUNNEL_SET,
 	MLX5_RTE_FLOW_ACTION_TYPE_AGE,
+	MLX5_RTE_FLOW_ACTION_TYPE_COUNT,
 };
 
 #define MLX5_SHARED_ACTION_TYPE_OFFSET 30
@@ -44,6 +45,7 @@  enum mlx5_rte_flow_action_type {
 enum {
 	MLX5_SHARED_ACTION_TYPE_RSS,
 	MLX5_SHARED_ACTION_TYPE_AGE,
+	MLX5_SHARED_ACTION_TYPE_COUNT,
 };
 
 /* Matches on selected register. */
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 1a74d5a..a23704e 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -3121,12 +3121,29 @@  struct field_modify_info modify_tcp[] = {
 }
 
 /**
+ * Check if action counter is shared by the old mechanism.
+ *
+ * @param[in] action
+ *   Pointer to the action structure.
+ *
+ * @return
+ *   True when shared bit is on, false otherwise.
+ */
+static inline bool
+is_shared_action_count(const struct rte_flow_action *action)
+{
+	const struct rte_flow_action_count *count =
+			(const struct rte_flow_action_count *)action->conf;
+	return !!(count && count->shared);
+}
+
+/**
  * Validate count action.
  *
  * @param[in] dev
  *   Pointer to rte_eth_dev structure.
- * @param[in] action
- *   Pointer to the action structure.
+ * @param[in] shared
+ *   Indicator if action is shared.
  * @param[in] action_flags
  *   Holds the actions detected until now.
  * @param[out] error
@@ -3136,13 +3153,11 @@  struct field_modify_info modify_tcp[] = {
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
 static int
-flow_dv_validate_action_count(struct rte_eth_dev *dev,
-			      const struct rte_flow_action *action,
+flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared,
 			      uint64_t action_flags,
 			      struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
-	const struct rte_flow_action_count *count;
 
 	if (!priv->config.devx)
 		goto notsup_err;
@@ -3150,8 +3165,7 @@  struct field_modify_info modify_tcp[] = {
 		return rte_flow_error_set(error, EINVAL,
 					  RTE_FLOW_ERROR_TYPE_ACTION, NULL,
 					  "duplicate count actions set");
-	count = (const struct rte_flow_action_count *)action->conf;
-	if (count && count->shared && (action_flags & MLX5_FLOW_ACTION_AGE) &&
+	if (shared && (action_flags & MLX5_FLOW_ACTION_AGE) &&
 	    !priv->sh->flow_hit_aso_en)
 		return rte_flow_error_set(error, EINVAL,
 					  RTE_FLOW_ERROR_TYPE_ACTION, NULL,
@@ -5213,7 +5227,7 @@  struct mlx5_hlist_entry *
 			break;
 		case RTE_FLOW_ACTION_TYPE_COUNT:
 			ret = flow_dv_validate_action_count
-				(dev, act,
+				(dev, is_shared_action_count(act),
 				 *action_flags | sub_action_flags,
 				 error);
 			if (ret < 0)
@@ -5372,7 +5386,7 @@  struct mlx5_hlist_entry *
  * @param[in] idx
  *   mlx5 flow counter index in the container.
  * @param[out] ppool
- *   mlx5 flow counter pool in the container,
+ *   mlx5 flow counter pool in the container.
  *
  * @return
  *   Pointer to the counter, NULL otherwise.
@@ -5743,6 +5757,14 @@  struct mlx5_hlist_entry *
 	if (!fallback && !priv->sh->cmng.query_thread_on)
 		/* Start the asynchronous batch query by the host thread. */
 		mlx5_set_query_alarm(priv->sh);
+	/*
+	 * When the count action isn't shared (by old mechanism), shared_info
+	 * field is used for shared action API's refcnt. when the action is not
+	 * shared neither old nor new mechanism, shared info have to be zero.
+	 */
+	if (!IS_SHARED_CNT(cnt_idx))
+		__atomic_store_n(&cnt_free->shared_info.refcnt, 0,
+				 __ATOMIC_RELAXED);
 	return cnt_idx;
 err:
 	if (cnt_free) {
@@ -5889,9 +5911,24 @@  struct mlx5_hlist_entry *
 		return;
 	cnt = flow_dv_counter_get_by_idx(dev, counter, &pool);
 	MLX5_ASSERT(pool);
+	/*
+	 * If the counter action is shared by old mechanism, the
+	 * l3t_clear_entry function reduces its references counter.
+	 * If after the reduction the action is still referenced, the
+	 * function returns here and does not release it.
+	 */
 	if (IS_SHARED_CNT(counter) &&
 	    mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, cnt->shared_info.id))
 		return;
+	/*
+	 * If the counter action is shared by new mechanism, the atomic function
+	 * reduces its references counter. If after the reduction the action is
+	 * still referenced, the function returns here and does not release it.
+	 */
+	if (!IS_SHARED_CNT(counter) &&
+	    __atomic_load_n(&cnt->shared_info.refcnt, __ATOMIC_RELAXED) &&
+	    __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, __ATOMIC_RELAXED))
+		return;
 	if (pool->is_aged)
 		flow_dv_counter_remove_from_age(dev, counter, cnt);
 	cnt->pool = pool;
@@ -6563,12 +6600,22 @@  struct mlx5_hlist_entry *
 			action_flags |= MLX5_FLOW_ACTION_DEFAULT_MISS;
 			++actions_n;
 			break;
-		case RTE_FLOW_ACTION_TYPE_COUNT:
-			ret = flow_dv_validate_action_count(dev, actions,
+		case MLX5_RTE_FLOW_ACTION_TYPE_COUNT:
+			ret = flow_dv_validate_action_count(dev, true,
 							    action_flags,
 							    error);
 			if (ret < 0)
 				return ret;
+			action_flags |= MLX5_FLOW_ACTION_COUNT;
+			++actions_n;
+			break;
+		case RTE_FLOW_ACTION_TYPE_COUNT:
+			ret = flow_dv_validate_action_count
+					       (dev,
+						is_shared_action_count(actions),
+						action_flags, error);
+			if (ret < 0)
+				return ret;
 			count = actions->conf;
 			action_flags |= MLX5_FLOW_ACTION_COUNT;
 			++actions_n;
@@ -9659,7 +9706,7 @@  struct mlx5_hlist_entry *
 		counter = flow_dv_counter_alloc(dev, !!age);
 	if (!counter || age == NULL)
 		return counter;
-	age_param  = flow_dv_counter_idx_get_age(dev, counter);
+	age_param = flow_dv_counter_idx_get_age(dev, counter);
 	age_param->context = age->context ? age->context :
 		(void *)(uintptr_t)(dev_flow->flow_idx);
 	age_param->timeout = age->timeout;
@@ -10983,6 +11030,7 @@  struct mlx5_cache_entry *
 		const struct rte_flow_action_meter *mtr;
 		struct mlx5_flow_tbl_resource *tbl;
 		struct mlx5_aso_age_action *age_act;
+		struct mlx5_flow_counter *cnt_act;
 		uint32_t port_id = 0;
 		struct mlx5_flow_dv_port_id_action_resource port_id_resource;
 		int action_type = actions->type;
@@ -11129,6 +11177,15 @@  struct mlx5_cache_entry *
 			dev_flow->dv.actions[actions_n++] = age_act->dr_action;
 			action_flags |= MLX5_FLOW_ACTION_AGE;
 			break;
+		case MLX5_RTE_FLOW_ACTION_TYPE_COUNT:
+			flow->counter = (uint32_t)(uintptr_t)(action->conf);
+			cnt_act = flow_dv_counter_get_by_idx(dev, flow->counter,
+							     NULL);
+			__atomic_fetch_add(&cnt_act->shared_info.refcnt, 1,
+					   __ATOMIC_RELAXED);
+			/* Save information first, will apply later. */
+			action_flags |= MLX5_FLOW_ACTION_COUNT;
+			break;
 		case RTE_FLOW_ACTION_TYPE_AGE:
 			if (priv->sh->flow_hit_aso_en && attr->group) {
 				/*
@@ -12895,6 +12952,7 @@  struct mlx5_cache_entry *
 		      const struct rte_flow_action *action,
 		      struct rte_flow_error *err)
 {
+	struct mlx5_flow_counter *cnt;
 	uint32_t idx = 0;
 	uint32_t ret = 0;
 
@@ -12917,6 +12975,28 @@  struct mlx5_cache_entry *
 							 (void *)(uintptr_t)idx;
 		}
 		break;
+	case RTE_FLOW_ACTION_TYPE_COUNT:
+		/*
+		 * There are two mechanisms to share the action count.
+		 * The old mechanism uses the shared field to share, while the
+		 * new mechanism uses the shared action API.
+		 * This validation comes to make sure that the two mechanisms
+		 * are not combined.
+		 */
+		if (is_shared_action_count(action)) {
+			rte_flow_error_set(err, ENOTSUP,
+					   RTE_FLOW_ERROR_TYPE_ACTION, NULL,
+					   "mix shared count mechanisms is not "
+					   "supported");
+			break;
+		}
+		ret = flow_dv_translate_create_counter(dev, NULL,
+						       action->conf, NULL);
+		cnt = flow_dv_counter_get_by_idx(dev, ret, NULL);
+		__atomic_store_n(&cnt->shared_info.refcnt, 1, __ATOMIC_RELAXED);
+		idx = (MLX5_SHARED_ACTION_TYPE_COUNT <<
+		       MLX5_SHARED_ACTION_TYPE_OFFSET) | ret;
+		break;
 	default:
 		rte_flow_error_set(err, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
 				   NULL, "action type not supported");
@@ -12950,11 +13030,25 @@  struct mlx5_cache_entry *
 	uint32_t act_idx = (uint32_t)(uintptr_t)action;
 	uint32_t type = act_idx >> MLX5_SHARED_ACTION_TYPE_OFFSET;
 	uint32_t idx = act_idx & ((1u << MLX5_SHARED_ACTION_TYPE_OFFSET) - 1);
+	struct mlx5_flow_counter *cnt;
+	uint32_t old_refcnt = 1;
 	int ret;
 
 	switch (type) {
 	case MLX5_SHARED_ACTION_TYPE_RSS:
 		return __flow_dv_action_rss_release(dev, idx, error);
+	case MLX5_SHARED_ACTION_TYPE_COUNT:
+		cnt = flow_dv_counter_get_by_idx(dev, idx, NULL);
+		if (!__atomic_compare_exchange_n(&cnt->shared_info.refcnt,
+						 &old_refcnt, 0, false,
+						 __ATOMIC_ACQUIRE,
+						 __ATOMIC_RELAXED))
+			return rte_flow_error_set(error, EBUSY,
+						 RTE_FLOW_ERROR_TYPE_ACTION,
+						 NULL,
+						 "shared count has references");
+		flow_dv_counter_free(dev, idx);
+		return 0;
 	case MLX5_SHARED_ACTION_TYPE_AGE:
 		ret = flow_dv_aso_age_release(dev, idx);
 		if (ret)
@@ -13079,46 +13173,16 @@  struct mlx5_cache_entry *
 	}
 }
 
-static int
-flow_dv_action_query(struct rte_eth_dev *dev,
-		     const struct rte_flow_shared_action *action, void *data,
-		     struct rte_flow_error *error)
-{
-	struct mlx5_age_param *age_param;
-	struct rte_flow_query_age *resp;
-	uint32_t act_idx = (uint32_t)(uintptr_t)action;
-	uint32_t type = act_idx >> MLX5_SHARED_ACTION_TYPE_OFFSET;
-	uint32_t idx = act_idx & ((1u << MLX5_SHARED_ACTION_TYPE_OFFSET) - 1);
-
-	switch (type) {
-	case MLX5_SHARED_ACTION_TYPE_AGE:
-		age_param = &flow_aso_age_get_by_idx(dev, idx)->age_params;
-		resp = data;
-		resp->aged = __atomic_load_n(&age_param->state,
-					      __ATOMIC_RELAXED) == AGE_TMOUT ?
-									  1 : 0;
-		resp->sec_since_last_hit_valid = !resp->aged;
-		if (resp->sec_since_last_hit_valid)
-			resp->sec_since_last_hit = __atomic_load_n
-			     (&age_param->sec_since_last_hit, __ATOMIC_RELAXED);
-		return 0;
-	default:
-		return rte_flow_error_set(error, ENOTSUP,
-					  RTE_FLOW_ERROR_TYPE_ACTION,
-					  NULL,
-					  "action type query not supported");
-	}
-}
 
 /**
- * Query a dv flow  rule for its statistics via devx.
+ * Query a DV flow rule for its statistics via DevX.
  *
  * @param[in] dev
  *   Pointer to Ethernet device.
- * @param[in] flow
- *   Pointer to the sub flow.
+ * @param[in] cnt_idx
+ *   Index to the flow counter.
  * @param[out] data
- *   data retrieved by the query.
+ *   Data retrieved by the query.
  * @param[out] error
  *   Perform verbose error reporting if not NULL.
  *
@@ -13126,8 +13190,8 @@  struct mlx5_cache_entry *
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
 static int
-flow_dv_query_count(struct rte_eth_dev *dev, struct rte_flow *flow,
-		    void *data, struct rte_flow_error *error)
+flow_dv_query_count(struct rte_eth_dev *dev, uint32_t cnt_idx, void *data,
+		    struct rte_flow_error *error)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct rte_flow_query_count *qc = data;
@@ -13137,19 +13201,16 @@  struct mlx5_cache_entry *
 					  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 					  NULL,
 					  "counters are not supported");
-	if (flow->counter) {
+	if (cnt_idx) {
 		uint64_t pkts, bytes;
 		struct mlx5_flow_counter *cnt;
-
-		cnt = flow_dv_counter_get_by_idx(dev, flow->counter,
-						 NULL);
-		int err = _flow_dv_query_count(dev, flow->counter, &pkts,
-					       &bytes);
+		int err = _flow_dv_query_count(dev, cnt_idx, &pkts, &bytes);
 
 		if (err)
 			return rte_flow_error_set(error, -err,
 					RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 					NULL, "cannot read counters");
+		cnt = flow_dv_counter_get_by_idx(dev, cnt_idx, NULL);
 		qc->hits_set = 1;
 		qc->bytes_set = 1;
 		qc->hits = pkts - cnt->hits;
@@ -13165,6 +13226,37 @@  struct mlx5_cache_entry *
 				  NULL,
 				  "counters are not available");
 }
+static int
+flow_dv_action_query(struct rte_eth_dev *dev,
+		     const struct rte_flow_shared_action *action, void *data,
+		     struct rte_flow_error *error)
+{
+	struct mlx5_age_param *age_param;
+	struct rte_flow_query_age *resp;
+	uint32_t act_idx = (uint32_t)(uintptr_t)action;
+	uint32_t type = act_idx >> MLX5_SHARED_ACTION_TYPE_OFFSET;
+	uint32_t idx = act_idx & ((1u << MLX5_SHARED_ACTION_TYPE_OFFSET) - 1);
+
+	switch (type) {
+	case MLX5_SHARED_ACTION_TYPE_AGE:
+		age_param = &flow_aso_age_get_by_idx(dev, idx)->age_params;
+		resp = data;
+		resp->aged = __atomic_load_n(&age_param->state,
+					      __ATOMIC_RELAXED) == AGE_TMOUT ?
+									  1 : 0;
+		resp->sec_since_last_hit_valid = !resp->aged;
+		if (resp->sec_since_last_hit_valid)
+			resp->sec_since_last_hit = __atomic_load_n
+			     (&age_param->sec_since_last_hit, __ATOMIC_RELAXED);
+		return 0;
+	case MLX5_SHARED_ACTION_TYPE_COUNT:
+		return flow_dv_query_count(dev, idx, data, error);
+	default:
+		return rte_flow_error_set(error, ENOTSUP,
+					  RTE_FLOW_ERROR_TYPE_ACTION, NULL,
+					  "action type query not supported");
+	}
+}
 
 /**
  * Query a flow rule AGE action for aging information.
@@ -13235,7 +13327,8 @@  struct mlx5_cache_entry *
 		case RTE_FLOW_ACTION_TYPE_VOID:
 			break;
 		case RTE_FLOW_ACTION_TYPE_COUNT:
-			ret = flow_dv_query_count(dev, flow, data, error);
+			ret = flow_dv_query_count(dev, flow->counter, data,
+						  error);
 			break;
 		case RTE_FLOW_ACTION_TYPE_AGE:
 			ret = flow_dv_query_age(dev, flow, data, error);
@@ -13915,6 +14008,21 @@  struct mlx5_cache_entry *
 						NULL,
 					     "shared age action not supported");
 		return flow_dv_validate_action_age(0, action, dev, err);
+	case RTE_FLOW_ACTION_TYPE_COUNT:
+		/*
+		 * There are two mechanisms to share the action count.
+		 * The old mechanism uses the shared field to share, while the
+		 * new mechanism uses the shared action API.
+		 * This validation comes to make sure that the two mechanisms
+		 * are not combined.
+		 */
+		if (is_shared_action_count(action))
+			return rte_flow_error_set(err, ENOTSUP,
+						  RTE_FLOW_ERROR_TYPE_ACTION,
+						  NULL,
+						  "mix shared count mechanisms "
+						  "is not supported");
+		return flow_dv_validate_action_count(dev, true, 0, err);
 	default:
 		return rte_flow_error_set(err, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION,