Indirect actions should be used to do shared counters. Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> --- app/test-pmd/cmdline_flow.c | 10 -- doc/guides/prog_guide/rte_flow.rst | 19 +-- doc/guides/rel_notes/deprecation.rst | 4 - doc/guides/rel_notes/release_21_11.rst | 4 + drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - drivers/net/cnxk/cnxk_rte_flow.c | 8 -- drivers/net/hns3/hns3_flow.c | 3 +- drivers/net/ice/ice_fdir_filter.c | 4 +- drivers/net/mlx5/mlx5.h | 7 -- drivers/net/mlx5/mlx5_flow_dv.c | 135 ++------------------- drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- drivers/net/octeontx2/otx2_flow_parse.c | 10 -- drivers/net/sfc/sfc_mae.c | 9 +- drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- lib/ethdev/rte_flow.h | 17 +-- 15 files changed, 23 insertions(+), 241 deletions(-) diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index bb22294dd3..0b5856c7d5 100644 --- a/app/test-pmd/cmdline_flow.c +++ b/app/test-pmd/cmdline_flow.c @@ -322,7 +322,6 @@ enum index { ACTION_QUEUE_INDEX, ACTION_DROP, ACTION_COUNT, - ACTION_COUNT_SHARED, ACTION_COUNT_ID, ACTION_RSS, ACTION_RSS_FUNC, @@ -1451,7 +1450,6 @@ static const enum index action_queue[] = { static const enum index action_count[] = { ACTION_COUNT_ID, - ACTION_COUNT_SHARED, ACTION_NEXT, ZERO, }; @@ -3712,14 +3710,6 @@ static const struct token token_list[] = { .args = ARGS(ARGS_ENTRY(struct rte_flow_action_count, id)), .call = parse_vc_conf, }, - [ACTION_COUNT_SHARED] = { - .name = "shared", - .help = "shared counter", - .next = NEXT(action_count, NEXT_ENTRY(COMMON_BOOLEAN)), - .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_action_count, - shared, 1)), - .call = parse_vc_conf, - }, [ACTION_RSS] = { .name = "rss", .help = "spread packets among several queues", diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst index 2b42d5ec8c..3cb014c1fa 100644 --- a/doc/guides/prog_guide/rte_flow.rst +++ b/doc/guides/prog_guide/rte_flow.rst @@ -158,7 +158,7 @@ Several pattern items and actions are valid and can be used in both directions. At least one direction must be specified. Specifying both directions at once for a given rule is not recommended but -may be valid in a few cases (e.g. shared counters). +may be valid in a few cases. Attribute: Transfer ^^^^^^^^^^^^^^^^^^^ @@ -1491,9 +1491,7 @@ Actions are performed in list order: +=======+========+============+=======+ | 0 | MARK | ``mark`` | 0x2a | +-------+--------+------------+-------+ - | 1 | COUNT | ``shared`` | 0 | - | | +------------+-------+ - | | | ``id`` | 0 | + | 1 | COUNT | ``id`` | 0 | +-------+--------+------------+-------+ | 2 | QUEUE | ``queue`` | 10 | +-------+--------+------------+-------+ @@ -1734,20 +1732,9 @@ action must specify a unique id. Counters can be retrieved and reset through ``rte_flow_query()``, see ``struct rte_flow_query_count``. -The shared flag indicates whether the counter is unique to the flow rule the -action is specified with, or whether it is a shared counter. - -For a count action with the shared flag set, then a global device -namespace is assumed for the counter id, so that any matched flow rules using -a count action with the same counter id on the same port will contribute to -that counter. - For ports within the same switch domain then the counter id namespace extends to all ports within that switch domain. -The shared flag is DEPRECATED and ``INDIRECT`` ``COUNT`` action should be used -to make shared counters. - .. _table_rte_flow_action_count: .. table:: COUNT @@ -1755,8 +1742,6 @@ to make shared counters. +------------+---------------------------------+ | Field | Value | +============+=================================+ - | ``shared`` | DEPRECATED, shared counter flag | - +------------+---------------------------------+ | ``id`` | counter id | +------------+---------------------------------+ diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst index 59445a6f42..21ef39841f 100644 --- a/doc/guides/rel_notes/deprecation.rst +++ b/doc/guides/rel_notes/deprecation.rst @@ -124,10 +124,6 @@ Deprecation Notices to support modifying fields larger than 64 bits. In addition, documentation will be updated to clarify byte order. -* ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count`` - is deprecated and will be removed in DPDK 21.11. Shared counters should - be managed using shared actions API (``rte_flow_shared_action_create`` etc). - * ethdev: Definition of the flow API action ``RTE_FLOW_ACTION_TYPE_PORT_ID`` is ambiguous and needs clarification. Structure ``rte_flow_action_port_id`` will be extended to specify diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst index a84c912f20..0d91ad5d7b 100644 --- a/doc/guides/rel_notes/release_21_11.rst +++ b/doc/guides/rel_notes/release_21_11.rst @@ -126,6 +126,10 @@ Removed Items blacklist/whitelist are removed. Users must use the new block/allow list arguments. +* ethdev: Removed deprecated ``shared`` attribute of the + ``struct rte_flow_action_count``. Shared counters should be managed + using indirect actions API (``rte_flow_action_handle_create`` etc). + API Changes ----------- diff --git a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c index 3a9c9bba27..f1e270af8b 100644 --- a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +++ b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c @@ -2111,11 +2111,6 @@ ulp_rte_count_act_handler(const struct rte_flow_action *action_item, act_count = action_item->conf; if (act_count) { - if (act_count->shared) { - BNXT_TF_DBG(ERR, - "Parse Error:Shared count not supported\n"); - return BNXT_TF_RC_PARSE_ERR; - } memcpy(&act_prop->act_details[BNXT_ULP_ACT_PROP_IDX_COUNT], &act_count->id, BNXT_ULP_ACT_PROP_SZ_COUNT); diff --git a/drivers/net/cnxk/cnxk_rte_flow.c b/drivers/net/cnxk/cnxk_rte_flow.c index 32c1b5dee5..27defd2fa9 100644 --- a/drivers/net/cnxk/cnxk_rte_flow.c +++ b/drivers/net/cnxk/cnxk_rte_flow.c @@ -110,7 +110,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr, struct roc_npc_action in_actions[], uint32_t *flowkey_cfg) { struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev); - const struct rte_flow_action_count *act_count; const struct rte_flow_action_queue *act_q; int i = 0, rc = 0; int rq; @@ -131,13 +130,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr, break; case RTE_FLOW_ACTION_TYPE_COUNT: - act_count = (const struct rte_flow_action_count *) - actions->conf; - - if (act_count->shared == 1) { - plt_npc_dbg("Shared counter is not supported"); - goto err_exit; - } in_actions[i].type = ROC_NPC_ACTION_TYPE_COUNT; break; diff --git a/drivers/net/hns3/hns3_flow.c b/drivers/net/hns3/hns3_flow.c index 841e0b9da3..fe1a387526 100644 --- a/drivers/net/hns3/hns3_flow.c +++ b/drivers/net/hns3/hns3_flow.c @@ -1782,8 +1782,7 @@ hns3_flow_create(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, goto out; if (fdir_rule.flags & HNS3_RULE_FLAG_COUNTER) { - ret = hns3_counter_new(dev, fdir_rule.act_cnt.shared, - fdir_rule.act_cnt.id, error); + ret = hns3_counter_new(dev, 0, fdir_rule.act_cnt.id, error); if (ret) goto out; diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fdir_filter.c index e0cca7cb3c..14e4e069c4 100644 --- a/drivers/net/ice/ice_fdir_filter.c +++ b/drivers/net/ice/ice_fdir_filter.c @@ -1339,9 +1339,7 @@ ice_fdir_create_filter(struct ice_adapter *ad, if (filter->input.cnt_ena) { struct rte_flow_action_count *act_count = &filter->act_count; - filter->counter = ice_fdir_counter_alloc(pf, - act_count->shared, - act_count->id); + filter->counter = ice_fdir_counter_alloc(pf, 0, act_count->id); if (!filter->counter) { rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION, NULL, diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index 3581414b78..ba48a0fee2 100644 --- a/drivers/net/mlx5/mlx5.h +++ b/drivers/net/mlx5/mlx5.h @@ -324,7 +324,6 @@ struct mlx5_lb_ctx { #define MLX5_MAX_PENDING_QUERIES 4 #define MLX5_CNT_CONTAINER_RESIZE 64 #define MLX5_CNT_SHARED_OFFSET 0x80000000 -#define IS_LEGACY_SHARED_CNT(cnt) (!!((cnt) & MLX5_CNT_SHARED_OFFSET)) #define IS_BATCH_CNT(cnt) (((cnt) & (MLX5_CNT_SHARED_OFFSET - 1)) >= \ MLX5_CNT_BATCH_OFFSET) #define MLX5_CNT_SIZE (sizeof(struct mlx5_flow_counter)) @@ -392,12 +391,6 @@ struct mlx5_flow_counter_shared { }; }; -/* Shared counter configuration. */ -struct mlx5_shared_counter_conf { - struct rte_eth_dev *dev; /* The device shared counter belongs to. */ - uint32_t id; /* The shared counter ID. */ -}; - struct mlx5_flow_counter_pool; /* Generic counters information. */ struct mlx5_flow_counter { diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index b610ad3ef4..91314d5ea5 100644 --- a/drivers/net/mlx5/mlx5_flow_dv.c +++ b/drivers/net/mlx5/mlx5_flow_dv.c @@ -3308,33 +3308,11 @@ flow_dv_validate_action_set_tag(struct rte_eth_dev *dev, return 0; } -/** - * Check if action counter is shared by either old or new mechanism. - * - * @param[in] action - * Pointer to the action structure. - * - * @return - * True when counter is shared, 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; - - if ((int)action->type == MLX5_RTE_FLOW_ACTION_TYPE_COUNT) - return true; - return !!(count && count->shared); -} - /** * Validate count action. * * @param[in] dev * Pointer to rte_eth_dev structure. - * @param[in] shared - * Indicator if action is shared. * @param[in] action_flags * Holds the actions detected until now. * @param[out] error @@ -3344,7 +3322,7 @@ is_shared_action_count(const struct rte_flow_action *action) * 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, bool shared, +flow_dv_validate_action_count(struct rte_eth_dev *dev, uint64_t action_flags, struct rte_flow_error *error) { @@ -3356,11 +3334,6 @@ flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared, return rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION, NULL, "duplicate count actions set"); - 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, - "old age and shared count combination is not supported"); #ifdef HAVE_IBV_FLOW_DEVX_COUNTERS return 0; #endif @@ -5658,9 +5631,7 @@ flow_dv_validate_action_sample(uint64_t *action_flags, break; case RTE_FLOW_ACTION_TYPE_COUNT: ret = flow_dv_validate_action_count - (dev, is_shared_action_count(act), - *action_flags | sub_action_flags, - error); + (dev, *action_flags | sub_action_flags, error); if (ret < 0) return ret; *count = act->conf; @@ -6230,60 +6201,6 @@ flow_dv_counter_alloc(struct rte_eth_dev *dev, uint32_t age) return 0; } -/** - * Allocate a shared flow counter. - * - * @param[in] ctx - * Pointer to the shared counter configuration. - * @param[in] data - * Pointer to save the allocated counter index. - * - * @return - * Index to flow counter on success, 0 otherwise and rte_errno is set. - */ - -static int32_t -flow_dv_counter_alloc_shared_cb(void *ctx, union mlx5_l3t_data *data) -{ - struct mlx5_shared_counter_conf *conf = ctx; - struct rte_eth_dev *dev = conf->dev; - struct mlx5_flow_counter *cnt; - - data->dword = flow_dv_counter_alloc(dev, 0); - data->dword |= MLX5_CNT_SHARED_OFFSET; - cnt = flow_dv_counter_get_by_idx(dev, data->dword, NULL); - cnt->shared_info.id = conf->id; - return 0; -} - -/** - * Get a shared flow counter. - * - * @param[in] dev - * Pointer to the Ethernet device structure. - * @param[in] id - * Counter identifier. - * - * @return - * Index to flow counter on success, 0 otherwise and rte_errno is set. - */ -static uint32_t -flow_dv_counter_get_shared(struct rte_eth_dev *dev, uint32_t id) -{ - struct mlx5_priv *priv = dev->data->dev_private; - struct mlx5_shared_counter_conf conf = { - .dev = dev, - .id = id, - }; - union mlx5_l3t_data data = { - .dword = 0, - }; - - mlx5_l3t_prepare_entry(priv->sh->cnt_id_tbl, id, &data, - flow_dv_counter_alloc_shared_cb, &conf); - return data.dword; -} - /** * Get age param from counter index. * @@ -6366,16 +6283,6 @@ flow_dv_counter_free(struct rte_eth_dev *dev, uint32_t counter) if (pool->is_aged) { flow_dv_counter_remove_from_age(dev, counter, cnt); } else { - /* - * If the counter action is shared by ID, 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_LEGACY_SHARED_CNT(counter) && - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, - cnt->shared_info.id)) - return; /* * If the counter action is shared by indirect action API, * the atomic function reduces its references counter. @@ -6385,8 +6292,7 @@ flow_dv_counter_free(struct rte_eth_dev *dev, uint32_t counter) * indirect action API, shared info is 1 before the reduction, * so this condition is failed and function doesn't return here. */ - if (!IS_LEGACY_SHARED_CNT(counter) && - __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, + if (__atomic_sub_fetch(&cnt->shared_info.refcnt, 1, __ATOMIC_RELAXED)) return; } @@ -7275,7 +7181,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, } for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { int type = actions->type; - bool shared_count = false; if (!mlx5_flow_os_action_supported(type)) return rte_flow_error_set(error, ENOTSUP, @@ -7427,8 +7332,7 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, break; case MLX5_RTE_FLOW_ACTION_TYPE_COUNT: case RTE_FLOW_ACTION_TYPE_COUNT: - shared_count = is_shared_action_count(actions); - ret = flow_dv_validate_action_count(dev, shared_count, + ret = flow_dv_validate_action_count(dev, action_flags, error); if (ret < 0) @@ -7747,12 +7651,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, * mutual exclusion with share counter actions. */ if (!priv->sh->flow_hit_aso_en) { - if (shared_count) - return rte_flow_error_set - (error, EINVAL, - RTE_FLOW_ERROR_TYPE_ACTION, - NULL, - "old age and shared count combination is not supported"); if (sample_count) return rte_flow_error_set (error, EINVAL, @@ -10837,16 +10735,14 @@ flow_dv_translate_action_port_id(struct rte_eth_dev *dev, static uint32_t flow_dv_translate_create_counter(struct rte_eth_dev *dev, struct mlx5_flow *dev_flow, - const struct rte_flow_action_count *count, + const struct rte_flow_action_count *count + __rte_unused, const struct rte_flow_action_age *age) { uint32_t counter; struct mlx5_age_param *age_param; - if (count && count->shared) - counter = flow_dv_counter_get_shared(dev, count->id); - else - counter = flow_dv_counter_alloc(dev, !!age); + counter = flow_dv_counter_alloc(dev, !!age); if (!counter || age == NULL) return counter; age_param = flow_dv_counter_idx_get_age(dev, counter); @@ -13216,8 +13112,7 @@ flow_dv_translate(struct rte_eth_dev *dev, * when they are not shared. */ if (action_flags & MLX5_FLOW_ACTION_AGE) { - if ((non_shared_age && - count && !count->shared) || + if ((non_shared_age && count) || !(priv->sh->flow_hit_aso_en && (attr->group || attr->transfer))) { /* Creates age by counters. */ @@ -17469,19 +17364,7 @@ flow_dv_action_validate(struct rte_eth_dev *dev, "Indirect 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 indirect 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 and indirect counter is not supported"); - return flow_dv_validate_action_count(dev, true, 0, err); + return flow_dv_validate_action_count(dev, 0, err); case RTE_FLOW_ACTION_TYPE_CONNTRACK: if (!priv->sh->ct_aso_en) return rte_flow_error_set(err, ENOTSUP, diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c index b93fd4d2c9..1627c3905f 100644 --- a/drivers/net/mlx5/mlx5_flow_verbs.c +++ b/drivers/net/mlx5/mlx5_flow_verbs.c @@ -250,8 +250,6 @@ flow_verbs_counter_create(struct rte_eth_dev *dev, * * @param[in] dev * Pointer to the Ethernet device structure. - * @param[in] shared - * Indicate if this counter is shared with other flows. * @param[in] id * Counter identifier. * @@ -259,21 +257,17 @@ flow_verbs_counter_create(struct rte_eth_dev *dev, * Index to the counter, 0 otherwise and rte_errno is set. */ static uint32_t -flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) +flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t id __rte_unused) { struct mlx5_priv *priv = dev->data->dev_private; struct mlx5_flow_counter_mng *cmng = &priv->sh->cmng; struct mlx5_flow_counter_pool *pool = NULL; struct mlx5_flow_counter *cnt = NULL; - union mlx5_l3t_data data; uint32_t n_valid = cmng->n_valid; uint32_t pool_idx, cnt_idx; uint32_t i; int ret; - if (shared && !mlx5_l3t_get_entry(priv->sh->cnt_id_tbl, id, &data) && - data.dword) - return data.dword; for (pool_idx = 0; pool_idx < n_valid; ++pool_idx) { pool = cmng->pools[pool_idx]; if (!pool) @@ -320,13 +314,6 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) TAILQ_REMOVE(&pool->counters[0], cnt, next); i = MLX5_CNT_ARRAY_IDX(pool, cnt); cnt_idx = MLX5_MAKE_CNT_IDX(pool_idx, i); - if (shared) { - data.dword = cnt_idx; - if (mlx5_l3t_set_entry(priv->sh->cnt_id_tbl, id, &data)) - return 0; - cnt->shared_info.id = id; - cnt_idx |= MLX5_CNT_SHARED_OFFSET; - } /* Create counter with Verbs. */ ret = flow_verbs_counter_create(dev, cnt); if (!ret) { @@ -352,14 +339,10 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) static void flow_verbs_counter_release(struct rte_eth_dev *dev, uint32_t counter) { - struct mlx5_priv *priv = dev->data->dev_private; struct mlx5_flow_counter_pool *pool; struct mlx5_flow_counter *cnt; cnt = flow_verbs_counter_get_by_idx(dev, counter, &pool); - if (IS_LEGACY_SHARED_CNT(counter) && - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, cnt->shared_info.id)) - return; #if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) claim_zero(mlx5_glue->destroy_counter_set ((struct ibv_counter_set *)cnt->dcs_when_active)); @@ -1198,8 +1181,7 @@ flow_verbs_translate_action_count(struct mlx5_flow *dev_flow, #endif if (!flow->counter) { - flow->counter = flow_verbs_counter_new(dev, count->shared, - count->id); + flow->counter = flow_verbs_counter_new(dev, count->id); if (!flow->counter) return rte_flow_error_set(error, rte_errno, RTE_FLOW_ERROR_TYPE_ACTION, diff --git a/drivers/net/octeontx2/otx2_flow_parse.c b/drivers/net/octeontx2/otx2_flow_parse.c index 63a33142a5..30a232f033 100644 --- a/drivers/net/octeontx2/otx2_flow_parse.c +++ b/drivers/net/octeontx2/otx2_flow_parse.c @@ -901,7 +901,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, struct otx2_eth_dev *hw = dev->data->dev_private; struct otx2_npc_flow_info *npc = &hw->npc_flow; const struct rte_flow_action_port_id *port_act; - const struct rte_flow_action_count *act_count; const struct rte_flow_action_mark *act_mark; const struct rte_flow_action_queue *act_q; const struct rte_flow_action_vf *vf_act; @@ -947,15 +946,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, break; case RTE_FLOW_ACTION_TYPE_COUNT: - act_count = - (const struct rte_flow_action_count *) - actions->conf; - - if (act_count->shared == 1) { - errmsg = "Shared Counters not supported"; - errcode = ENOTSUP; - goto err_exit; - } /* Indicates, need a counter */ flow->ctr_id = 1; req_act |= OTX2_FLOW_ACT_COUNT; diff --git a/drivers/net/sfc/sfc_mae.c b/drivers/net/sfc/sfc_mae.c index 4b520bc619..5fcdf9c2f5 100644 --- a/drivers/net/sfc/sfc_mae.c +++ b/drivers/net/sfc/sfc_mae.c @@ -2802,16 +2802,12 @@ sfc_mae_rule_parse_action_mark(struct sfc_adapter *sa, static int sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, - const struct rte_flow_action_count *conf, + const struct rte_flow_action_count *conf + __rte_unused, efx_mae_actions_t *spec) { int rc; - if (conf->shared) { - rc = ENOTSUP; - goto fail_counter_shared; - } - if ((sa->counter_rxq.state & SFC_COUNTER_RXQ_INITIALIZED) == 0) { sfc_err(sa, "counter queue is not configured for COUNT action"); @@ -2837,7 +2833,6 @@ sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, fail_populate_count: fail_no_service_core: fail_counter_queue_uninit: -fail_counter_shared: return rc; } diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c b/drivers/net/softnic/rte_eth_softnic_flow.c index 27eaf380cd..39038d26d8 100644 --- a/drivers/net/softnic/rte_eth_softnic_flow.c +++ b/drivers/net/softnic/rte_eth_softnic_flow.c @@ -1448,13 +1448,6 @@ flow_rule_action_get(struct pmd_internals *softnic, action, "COUNT: Null configuration"); - if (conf->shared) - return rte_flow_error_set(error, - ENOTSUP, - RTE_FLOW_ERROR_TYPE_ACTION_CONF, - conf, - "COUNT: Shared counters not supported"); - if (n_count) return rte_flow_error_set(error, ENOTSUP, diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 7b1ed7f110..5306e8ca4b 100644 --- a/lib/ethdev/rte_flow.h +++ b/lib/ethdev/rte_flow.h @@ -75,7 +75,7 @@ extern "C" { * At least one direction must be specified. * * Specifying both directions at once for a given rule is not recommended - * but may be valid in a few cases (e.g. shared counter). + * but may be valid in a few cases. */ struct rte_flow_attr { uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,11 @@ struct rte_flow_query_age { * Counters can be retrieved and reset through ``rte_flow_query()``, see * ``struct rte_flow_query_count``. * - * @deprecated Shared attribute is deprecated, use generic - * RTE_FLOW_ACTION_TYPE_INDIRECT action. - * - * The shared flag indicates whether the counter is unique to the flow rule the - * action is specified with, or whether it is a shared counter. - * - * For a count action with the shared flag set, then then a global device - * namespace is assumed for the counter id, so that any matched flow rules using - * a count action with the same counter id on the same port will contribute to - * that counter. - * * For ports within the same switch domain then the counter id namespace extends * to all ports within that switch domain. */ struct rte_flow_action_count { - /** @deprecated Share counter ID with other flow rules. */ - uint32_t shared:1; - uint32_t reserved:31; /**< Reserved, must be zero. */ + uint32_t reserved; /**< Reserved, must be zero. */ uint32_t id; /**< Counter ID. */ }; -- 2.30.2
On Tue, 28 Sep 2021 18:23:00 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
> @@ -2498,24 +2498,11 @@ struct rte_flow_query_age {
> * Counters can be retrieved and reset through ``rte_flow_query()``, see
> * ``struct rte_flow_query_count``.
> *
> - * @deprecated Shared attribute is deprecated, use generic
> - * RTE_FLOW_ACTION_TYPE_INDIRECT action.
> - *
> - * The shared flag indicates whether the counter is unique to the flow rule the
> - * action is specified with, or whether it is a shared counter.
> - *
> - * For a count action with the shared flag set, then then a global device
> - * namespace is assumed for the counter id, so that any matched flow rules using
> - * a count action with the same counter id on the same port will contribute to
> - * that counter.
> - *
> * For ports within the same switch domain then the counter id namespace extends
> * to all ports within that switch domain.
> */
> struct rte_flow_action_count {
> - /** @deprecated Share counter ID with other flow rules. */
> - uint32_t shared:1;
> - uint32_t reserved:31; /**< Reserved, must be zero. */
> + uint32_t reserved; /**< Reserved, must be zero. */
> uint32_t id; /**< Counter ID. */
Reserved fields are often source of future problems.
You should change each driver to check that reserved field
return -ENOTSUP if non-zero. That way if reserved field is ever
used in future it won't break API/ABI.
The other option is to just remove the reserved field and take the API/ABI
hit now.
On 9/28/21 6:58 PM, Stephen Hemminger wrote:
> On Tue, 28 Sep 2021 18:23:00 +0300
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
>
>> @@ -2498,24 +2498,11 @@ struct rte_flow_query_age {
>> * Counters can be retrieved and reset through ``rte_flow_query()``, see
>> * ``struct rte_flow_query_count``.
>> *
>> - * @deprecated Shared attribute is deprecated, use generic
>> - * RTE_FLOW_ACTION_TYPE_INDIRECT action.
>> - *
>> - * The shared flag indicates whether the counter is unique to the flow rule the
>> - * action is specified with, or whether it is a shared counter.
>> - *
>> - * For a count action with the shared flag set, then then a global device
>> - * namespace is assumed for the counter id, so that any matched flow rules using
>> - * a count action with the same counter id on the same port will contribute to
>> - * that counter.
>> - *
>> * For ports within the same switch domain then the counter id namespace extends
>> * to all ports within that switch domain.
>> */
>> struct rte_flow_action_count {
>> - /** @deprecated Share counter ID with other flow rules. */
>> - uint32_t shared:1;
>> - uint32_t reserved:31; /**< Reserved, must be zero. */
>> + uint32_t reserved; /**< Reserved, must be zero. */
>> uint32_t id; /**< Counter ID. */
>
> Reserved fields are often source of future problems.
> You should change each driver to check that reserved field
> return -ENOTSUP if non-zero. That way if reserved field is ever
> used in future it won't break API/ABI.
>
> The other option is to just remove the reserved field and take the API/ABI
> hit now.
>
I agree. If there is no objections, I'll remove it in v2.
28/09/2021 17:23, Andrew Rybchenko:
> Indirect actions should be used to do shared counters.
>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Thanks Andrew, we need more maintainers like you :)
[-- Attachment #1: Type: text/plain, Size: 418 bytes --] On Tue, Oct 5, 2021 at 12:11 PM Thomas Monjalon <thomas@monjalon.net> wrote: > 28/09/2021 17:23, Andrew Rybchenko: > > Indirect actions should be used to do shared counters. > > > > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > > Acked-by: Thomas Monjalon <thomas@monjalon.net> > Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com> > > Thanks Andrew, we need more maintainers like you :) > > >
Hi Andrew Thank you for the big effort to adjust mlx5. I left some comments inside. If you feel it is too much, we can do a later patch to improve. Matan > From: Andrew Rybchenko > Indirect actions should be used to do shared counters. > > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > --- > app/test-pmd/cmdline_flow.c | 10 -- > doc/guides/prog_guide/rte_flow.rst | 19 +-- > doc/guides/rel_notes/deprecation.rst | 4 - > doc/guides/rel_notes/release_21_11.rst | 4 + > drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - > drivers/net/cnxk/cnxk_rte_flow.c | 8 -- > drivers/net/hns3/hns3_flow.c | 3 +- > drivers/net/ice/ice_fdir_filter.c | 4 +- > drivers/net/mlx5/mlx5.h | 7 -- > drivers/net/mlx5/mlx5_flow_dv.c | 135 ++------------------- > drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- > drivers/net/octeontx2/otx2_flow_parse.c | 10 -- > drivers/net/sfc/sfc_mae.c | 9 +- > drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- > lib/ethdev/rte_flow.h | 17 +-- > 15 files changed, 23 insertions(+), 241 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index bb22294dd3..0b5856c7d5 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -322,7 +322,6 @@ enum index { > ACTION_QUEUE_INDEX, > ACTION_DROP, > ACTION_COUNT, > - ACTION_COUNT_SHARED, > ACTION_COUNT_ID, > ACTION_RSS, > ACTION_RSS_FUNC, > @@ -1451,7 +1450,6 @@ static const enum index action_queue[] = { > > static const enum index action_count[] = { > ACTION_COUNT_ID, > - ACTION_COUNT_SHARED, > ACTION_NEXT, > ZERO, > }; > @@ -3712,14 +3710,6 @@ static const struct token token_list[] = { > .args = ARGS(ARGS_ENTRY(struct rte_flow_action_count, id)), > .call = parse_vc_conf, > }, > - [ACTION_COUNT_SHARED] = { > - .name = "shared", > - .help = "shared counter", > - .next = NEXT(action_count, NEXT_ENTRY(COMMON_BOOLEAN)), > - .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_action_count, > - shared, 1)), > - .call = parse_vc_conf, > - }, > [ACTION_RSS] = { > .name = "rss", > .help = "spread packets among several queues", diff --git > a/doc/guides/prog_guide/rte_flow.rst > b/doc/guides/prog_guide/rte_flow.rst > index 2b42d5ec8c..3cb014c1fa 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -158,7 +158,7 @@ Several pattern items and actions are valid and can be > used in both directions. At least one direction must be specified. > > Specifying both directions at once for a given rule is not recommended but - > may be valid in a few cases (e.g. shared counters). > +may be valid in a few cases. > > Attribute: Transfer > ^^^^^^^^^^^^^^^^^^^ > @@ -1491,9 +1491,7 @@ Actions are performed in list order: > +=======+========+============+=======+ > | 0 | MARK | ``mark`` | 0x2a | > +-------+--------+------------+-------+ > - | 1 | COUNT | ``shared`` | 0 | > - | | +------------+-------+ > - | | | ``id`` | 0 | > + | 1 | COUNT | ``id`` | 0 | > +-------+--------+------------+-------+ > | 2 | QUEUE | ``queue`` | 10 | > +-------+--------+------------+-------+ > @@ -1734,20 +1732,9 @@ action must specify a unique id. > Counters can be retrieved and reset through ``rte_flow_query()``, see > ``struct rte_flow_query_count``. > > -The shared flag indicates whether the counter is unique to the flow rule the > -action is specified with, or whether it is a shared counter. > - > -For a count action with the shared flag set, then a global device -namespace > is assumed for the counter id, so that any matched flow rules using -a count > action with the same counter id on the same port will contribute to -that > counter. > - > For ports within the same switch domain then the counter id namespace > extends to all ports within that switch domain. > > -The shared flag is DEPRECATED and ``INDIRECT`` ``COUNT`` action should be > used -to make shared counters. > - > .. _table_rte_flow_action_count: > > .. table:: COUNT > @@ -1755,8 +1742,6 @@ to make shared counters. > +------------+---------------------------------+ > | Field | Value | > +============+=================================+ > - | ``shared`` | DEPRECATED, shared counter flag | > - +------------+---------------------------------+ > | ``id`` | counter id | > +------------+---------------------------------+ > > diff --git a/doc/guides/rel_notes/deprecation.rst > b/doc/guides/rel_notes/deprecation.rst > index 59445a6f42..21ef39841f 100644 > --- a/doc/guides/rel_notes/deprecation.rst > +++ b/doc/guides/rel_notes/deprecation.rst > @@ -124,10 +124,6 @@ Deprecation Notices > to support modifying fields larger than 64 bits. > In addition, documentation will be updated to clarify byte order. > > -* ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count`` > - is deprecated and will be removed in DPDK 21.11. Shared counters should > - be managed using shared actions API (``rte_flow_shared_action_create`` > etc). > - > * ethdev: Definition of the flow API action > ``RTE_FLOW_ACTION_TYPE_PORT_ID`` > is ambiguous and needs clarification. > Structure ``rte_flow_action_port_id`` will be extended to specify diff --git > a/doc/guides/rel_notes/release_21_11.rst > b/doc/guides/rel_notes/release_21_11.rst > index a84c912f20..0d91ad5d7b 100644 > --- a/doc/guides/rel_notes/release_21_11.rst > +++ b/doc/guides/rel_notes/release_21_11.rst > @@ -126,6 +126,10 @@ Removed Items > blacklist/whitelist are removed. Users must use the new > block/allow list arguments. > > +* ethdev: Removed deprecated ``shared`` attribute of the > + ``struct rte_flow_action_count``. Shared counters should be managed > + using indirect actions API (``rte_flow_action_handle_create`` etc). > + > > API Changes > ----------- > diff --git a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > index 3a9c9bba27..f1e270af8b 100644 > --- a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > +++ b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > @@ -2111,11 +2111,6 @@ ulp_rte_count_act_handler(const struct > rte_flow_action *action_item, > > act_count = action_item->conf; > if (act_count) { > - if (act_count->shared) { > - BNXT_TF_DBG(ERR, > - "Parse Error:Shared count not supported\n"); > - return BNXT_TF_RC_PARSE_ERR; > - } > memcpy(&act_prop- > >act_details[BNXT_ULP_ACT_PROP_IDX_COUNT], > &act_count->id, > BNXT_ULP_ACT_PROP_SZ_COUNT); diff --git > a/drivers/net/cnxk/cnxk_rte_flow.c b/drivers/net/cnxk/cnxk_rte_flow.c > index 32c1b5dee5..27defd2fa9 100644 > --- a/drivers/net/cnxk/cnxk_rte_flow.c > +++ b/drivers/net/cnxk/cnxk_rte_flow.c > @@ -110,7 +110,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, > const struct rte_flow_attr *attr, > struct roc_npc_action in_actions[], uint32_t *flowkey_cfg) { > struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev); > - const struct rte_flow_action_count *act_count; > const struct rte_flow_action_queue *act_q; > int i = 0, rc = 0; > int rq; > @@ -131,13 +130,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, > const struct rte_flow_attr *attr, > break; > > case RTE_FLOW_ACTION_TYPE_COUNT: > - act_count = (const struct rte_flow_action_count *) > - actions->conf; > - > - if (act_count->shared == 1) { > - plt_npc_dbg("Shared counter is not supported"); > - goto err_exit; > - } > in_actions[i].type = ROC_NPC_ACTION_TYPE_COUNT; > break; > > diff --git a/drivers/net/hns3/hns3_flow.c b/drivers/net/hns3/hns3_flow.c > index 841e0b9da3..fe1a387526 100644 > --- a/drivers/net/hns3/hns3_flow.c > +++ b/drivers/net/hns3/hns3_flow.c > @@ -1782,8 +1782,7 @@ hns3_flow_create(struct rte_eth_dev *dev, const > struct rte_flow_attr *attr, > goto out; > > if (fdir_rule.flags & HNS3_RULE_FLAG_COUNTER) { > - ret = hns3_counter_new(dev, fdir_rule.act_cnt.shared, > - fdir_rule.act_cnt.id, error); > + ret = hns3_counter_new(dev, 0, fdir_rule.act_cnt.id, > + error); > if (ret) > goto out; > > diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fdir_filter.c > index e0cca7cb3c..14e4e069c4 100644 > --- a/drivers/net/ice/ice_fdir_filter.c > +++ b/drivers/net/ice/ice_fdir_filter.c > @@ -1339,9 +1339,7 @@ ice_fdir_create_filter(struct ice_adapter *ad, > if (filter->input.cnt_ena) { > struct rte_flow_action_count *act_count = &filter->act_count; > > - filter->counter = ice_fdir_counter_alloc(pf, > - act_count->shared, > - act_count->id); > + filter->counter = ice_fdir_counter_alloc(pf, 0, > + act_count->id); > if (!filter->counter) { > rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, NULL, diff --git > a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > 3581414b78..ba48a0fee2 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -324,7 +324,6 @@ struct mlx5_lb_ctx { #define > MLX5_MAX_PENDING_QUERIES 4 #define MLX5_CNT_CONTAINER_RESIZE > 64 #define MLX5_CNT_SHARED_OFFSET 0x80000000 -#define > IS_LEGACY_SHARED_CNT(cnt) (!!((cnt) & MLX5_CNT_SHARED_OFFSET)) > #define IS_BATCH_CNT(cnt) (((cnt) & (MLX5_CNT_SHARED_OFFSET - 1)) >= \ > MLX5_CNT_BATCH_OFFSET) #define MLX5_CNT_SIZE > (sizeof(struct mlx5_flow_counter)) @@ -392,12 +391,6 @@ struct > mlx5_flow_counter_shared { > }; > }; > > -/* Shared counter configuration. */ > -struct mlx5_shared_counter_conf { > - struct rte_eth_dev *dev; /* The device shared counter belongs to. */ > - uint32_t id; /* The shared counter ID. */ > -}; > - > struct mlx5_flow_counter_pool; > /* Generic counters information. */ > struct mlx5_flow_counter { > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c > b/drivers/net/mlx5/mlx5_flow_dv.c index b610ad3ef4..91314d5ea5 100644 > --- a/drivers/net/mlx5/mlx5_flow_dv.c > +++ b/drivers/net/mlx5/mlx5_flow_dv.c > @@ -3308,33 +3308,11 @@ flow_dv_validate_action_set_tag(struct > rte_eth_dev *dev, > return 0; > } > > -/** > - * Check if action counter is shared by either old or new mechanism. > - * > - * @param[in] action > - * Pointer to the action structure. > - * > - * @return > - * True when counter is shared, 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; > - > - if ((int)action->type == MLX5_RTE_FLOW_ACTION_TYPE_COUNT) > - return true; > - return !!(count && count->shared); > -} > - > /** > * Validate count action. > * > * @param[in] dev > * Pointer to rte_eth_dev structure. > - * @param[in] shared > - * Indicator if action is shared. > * @param[in] action_flags > * Holds the actions detected until now. > * @param[out] error > @@ -3344,7 +3322,7 @@ is_shared_action_count(const struct > rte_flow_action *action) > * 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, bool shared, > +flow_dv_validate_action_count(struct rte_eth_dev *dev, > uint64_t action_flags, > struct rte_flow_error *error) { @@ -3356,11 +3334,6 @@ > flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared, > return rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, NULL, > "duplicate count actions set"); > - if (shared && (action_flags & MLX5_FLOW_ACTION_AGE) && This check is relevant to the indirect action case in some places, see below. > - !priv->sh->flow_hit_aso_en) > - return rte_flow_error_set(error, EINVAL, > - RTE_FLOW_ERROR_TYPE_ACTION, NULL, > - "old age and shared count combination is not > supported"); > #ifdef HAVE_IBV_FLOW_DEVX_COUNTERS > return 0; > #endif > @@ -5658,9 +5631,7 @@ flow_dv_validate_action_sample(uint64_t > *action_flags, > break; > case RTE_FLOW_ACTION_TYPE_COUNT: > ret = flow_dv_validate_action_count > - (dev, is_shared_action_count(act), > - *action_flags | sub_action_flags, > - error); > + (dev, *action_flags | sub_action_flags, > + error); > if (ret < 0) > return ret; > *count = act->conf; @@ -6230,60 +6201,6 @@ > flow_dv_counter_alloc(struct rte_eth_dev *dev, uint32_t age) > return 0; > } > > -/** > - * Allocate a shared flow counter. > - * > - * @param[in] ctx > - * Pointer to the shared counter configuration. > - * @param[in] data > - * Pointer to save the allocated counter index. > - * > - * @return > - * Index to flow counter on success, 0 otherwise and rte_errno is set. > - */ > - > -static int32_t > -flow_dv_counter_alloc_shared_cb(void *ctx, union mlx5_l3t_data *data) -{ > - struct mlx5_shared_counter_conf *conf = ctx; > - struct rte_eth_dev *dev = conf->dev; > - struct mlx5_flow_counter *cnt; > - > - data->dword = flow_dv_counter_alloc(dev, 0); > - data->dword |= MLX5_CNT_SHARED_OFFSET; > - cnt = flow_dv_counter_get_by_idx(dev, data->dword, NULL); > - cnt->shared_info.id = conf->id; > - return 0; > -} > - > -/** > - * Get a shared flow counter. > - * > - * @param[in] dev > - * Pointer to the Ethernet device structure. > - * @param[in] id > - * Counter identifier. > - * > - * @return > - * Index to flow counter on success, 0 otherwise and rte_errno is set. > - */ > -static uint32_t > -flow_dv_counter_get_shared(struct rte_eth_dev *dev, uint32_t id) -{ > - struct mlx5_priv *priv = dev->data->dev_private; > - struct mlx5_shared_counter_conf conf = { > - .dev = dev, > - .id = id, > - }; > - union mlx5_l3t_data data = { > - .dword = 0, > - }; > - > - mlx5_l3t_prepare_entry(priv->sh->cnt_id_tbl, id, &data, > - flow_dv_counter_alloc_shared_cb, &conf); > - return data.dword; > -} > - Need to remove cnt_id_tbl and from sh and all of its management, no? > /** > * Get age param from counter index. > * > @@ -6366,16 +6283,6 @@ flow_dv_counter_free(struct rte_eth_dev *dev, > uint32_t counter) > if (pool->is_aged) { > flow_dv_counter_remove_from_age(dev, counter, cnt); > } else { > - /* > - * If the counter action is shared by ID, 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_LEGACY_SHARED_CNT(counter) && > - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, > - cnt->shared_info.id)) > - return; > /* > * If the counter action is shared by indirect action API, > * the atomic function reduces its references counter. > @@ -6385,8 +6292,7 @@ flow_dv_counter_free(struct rte_eth_dev *dev, > uint32_t counter) You can remove the ID sharing notice in this comment. > * indirect action API, shared info is 1 before the reduction, > * so this condition is failed and function doesn't return here. > */ > - if (!IS_LEGACY_SHARED_CNT(counter) && > - __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, > + if (__atomic_sub_fetch(&cnt->shared_info.refcnt, 1, > __ATOMIC_RELAXED)) > return; > } > @@ -7275,7 +7181,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const > struct rte_flow_attr *attr, > } > for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > int type = actions->type; > - bool shared_count = false; > > if (!mlx5_flow_os_action_supported(type)) > return rte_flow_error_set(error, ENOTSUP, @@ -7427,8 +7332,7 > @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr > *attr, > break; > case MLX5_RTE_FLOW_ACTION_TYPE_COUNT: > case RTE_FLOW_ACTION_TYPE_COUNT: > - shared_count = is_shared_action_count(actions); We need shared_count for the indirect count case(which can be shared with different flows), So, if the action is MLX5_RTE_FLOW_ACTION_TYPE_COUNT, shared_count should be true. It is relevant to the validate function below and for the check at the end of the function. > - ret = flow_dv_validate_action_count(dev, shared_count, > + ret = flow_dv_validate_action_count(dev, > action_flags, > error); > if (ret < 0) > @@ -7747,12 +7651,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const > struct rte_flow_attr *attr, > * mutual exclusion with share counter actions. > */ > if (!priv->sh->flow_hit_aso_en) { > - if (shared_count) > - return rte_flow_error_set > - (error, EINVAL, > - RTE_FLOW_ERROR_TYPE_ACTION, > - NULL, > - "old age and shared count combination is not > supported"); > if (sample_count) > return rte_flow_error_set > (error, EINVAL, @@ -10837,16 +10735,14 @@ > flow_dv_translate_action_port_id(struct rte_eth_dev *dev, static uint32_t > flow_dv_translate_create_counter(struct rte_eth_dev *dev, > struct mlx5_flow *dev_flow, > - const struct rte_flow_action_count *count, > + const struct rte_flow_action_count *count > + __rte_unused, > const struct rte_flow_action_age *age) { > uint32_t counter; > struct mlx5_age_param *age_param; > > - if (count && count->shared) > - counter = flow_dv_counter_get_shared(dev, count->id); > - else > - counter = flow_dv_counter_alloc(dev, !!age); > + counter = flow_dv_counter_alloc(dev, !!age); > if (!counter || age == NULL) > return counter; > age_param = flow_dv_counter_idx_get_age(dev, counter); @@ - > 13216,8 +13112,7 @@ flow_dv_translate(struct rte_eth_dev *dev, > * when they are not shared. > */ > if (action_flags & MLX5_FLOW_ACTION_AGE) { > - if ((non_shared_age && > - count && !count->shared) || > + if ((non_shared_age && count) || > !(priv->sh->flow_hit_aso_en && > (attr->group || attr->transfer))) { > /* Creates age by counters. */ @@ -17469,19 +17364,7 > @@ flow_dv_action_validate(struct rte_eth_dev *dev, > "Indirect 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 indirect 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 and indirect counter is not supported"); > - return flow_dv_validate_action_count(dev, true, 0, err); > + return flow_dv_validate_action_count(dev, 0, err); Did you also consider improving struct mlx5_flow_counter_shared? Then, maybe we don't need it anymore. > case RTE_FLOW_ACTION_TYPE_CONNTRACK: > if (!priv->sh->ct_aso_en) > return rte_flow_error_set(err, ENOTSUP, diff --git > a/drivers/net/mlx5/mlx5_flow_verbs.c > b/drivers/net/mlx5/mlx5_flow_verbs.c > index b93fd4d2c9..1627c3905f 100644 > --- a/drivers/net/mlx5/mlx5_flow_verbs.c > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c > @@ -250,8 +250,6 @@ flow_verbs_counter_create(struct rte_eth_dev > *dev, > * > * @param[in] dev > * Pointer to the Ethernet device structure. > - * @param[in] shared > - * Indicate if this counter is shared with other flows. > * @param[in] id > * Counter identifier. > * > @@ -259,21 +257,17 @@ flow_verbs_counter_create(struct rte_eth_dev > *dev, > * Index to the counter, 0 otherwise and rte_errno is set. > */ > static uint32_t > -flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, > uint32_t id) > +flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t id > +__rte_unused) > { > struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_flow_counter_mng *cmng = &priv->sh->cmng; > struct mlx5_flow_counter_pool *pool = NULL; > struct mlx5_flow_counter *cnt = NULL; > - union mlx5_l3t_data data; > uint32_t n_valid = cmng->n_valid; > uint32_t pool_idx, cnt_idx; > uint32_t i; > int ret; > > - if (shared && !mlx5_l3t_get_entry(priv->sh->cnt_id_tbl, id, &data) && > - data.dword) > - return data.dword; > for (pool_idx = 0; pool_idx < n_valid; ++pool_idx) { > pool = cmng->pools[pool_idx]; > if (!pool) > @@ -320,13 +314,6 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, > uint32_t shared, uint32_t id) > TAILQ_REMOVE(&pool->counters[0], cnt, next); > i = MLX5_CNT_ARRAY_IDX(pool, cnt); > cnt_idx = MLX5_MAKE_CNT_IDX(pool_idx, i); > - if (shared) { > - data.dword = cnt_idx; > - if (mlx5_l3t_set_entry(priv->sh->cnt_id_tbl, id, &data)) > - return 0; > - cnt->shared_info.id = id; > - cnt_idx |= MLX5_CNT_SHARED_OFFSET; > - } > /* Create counter with Verbs. */ > ret = flow_verbs_counter_create(dev, cnt); > if (!ret) { > @@ -352,14 +339,10 @@ flow_verbs_counter_new(struct rte_eth_dev > *dev, uint32_t shared, uint32_t id) static void > flow_verbs_counter_release(struct rte_eth_dev *dev, uint32_t counter) { > - struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_flow_counter_pool *pool; > struct mlx5_flow_counter *cnt; > > cnt = flow_verbs_counter_get_by_idx(dev, counter, &pool); > - if (IS_LEGACY_SHARED_CNT(counter) && > - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, cnt->shared_info.id)) > - return; > #if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) > claim_zero(mlx5_glue->destroy_counter_set > ((struct ibv_counter_set *)cnt->dcs_when_active)); @@ -1198,8 > +1181,7 @@ flow_verbs_translate_action_count(struct mlx5_flow > *dev_flow, #endif > > if (!flow->counter) { > - flow->counter = flow_verbs_counter_new(dev, count->shared, > - count->id); > + flow->counter = flow_verbs_counter_new(dev, count->id); > if (!flow->counter) > return rte_flow_error_set(error, rte_errno, > RTE_FLOW_ERROR_TYPE_ACTION, diff --git > a/drivers/net/octeontx2/otx2_flow_parse.c > b/drivers/net/octeontx2/otx2_flow_parse.c > index 63a33142a5..30a232f033 100644 > --- a/drivers/net/octeontx2/otx2_flow_parse.c > +++ b/drivers/net/octeontx2/otx2_flow_parse.c > @@ -901,7 +901,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, > struct otx2_eth_dev *hw = dev->data->dev_private; > struct otx2_npc_flow_info *npc = &hw->npc_flow; > const struct rte_flow_action_port_id *port_act; > - const struct rte_flow_action_count *act_count; > const struct rte_flow_action_mark *act_mark; > const struct rte_flow_action_queue *act_q; > const struct rte_flow_action_vf *vf_act; @@ -947,15 +946,6 @@ > otx2_flow_parse_actions(struct rte_eth_dev *dev, > break; > > case RTE_FLOW_ACTION_TYPE_COUNT: > - act_count = > - (const struct rte_flow_action_count *) > - actions->conf; > - > - if (act_count->shared == 1) { > - errmsg = "Shared Counters not supported"; > - errcode = ENOTSUP; > - goto err_exit; > - } > /* Indicates, need a counter */ > flow->ctr_id = 1; > req_act |= OTX2_FLOW_ACT_COUNT; diff --git > a/drivers/net/sfc/sfc_mae.c b/drivers/net/sfc/sfc_mae.c index > 4b520bc619..5fcdf9c2f5 100644 > --- a/drivers/net/sfc/sfc_mae.c > +++ b/drivers/net/sfc/sfc_mae.c > @@ -2802,16 +2802,12 @@ sfc_mae_rule_parse_action_mark(struct > sfc_adapter *sa, > > static int > sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, > - const struct rte_flow_action_count *conf, > + const struct rte_flow_action_count *conf > + __rte_unused, > efx_mae_actions_t *spec) { > int rc; > > - if (conf->shared) { > - rc = ENOTSUP; > - goto fail_counter_shared; > - } > - > if ((sa->counter_rxq.state & SFC_COUNTER_RXQ_INITIALIZED) == 0) { > sfc_err(sa, > "counter queue is not configured for COUNT action"); @@ - > 2837,7 +2833,6 @@ sfc_mae_rule_parse_action_count(struct sfc_adapter > *sa, > fail_populate_count: > fail_no_service_core: > fail_counter_queue_uninit: > -fail_counter_shared: > > return rc; > } > diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c > b/drivers/net/softnic/rte_eth_softnic_flow.c > index 27eaf380cd..39038d26d8 100644 > --- a/drivers/net/softnic/rte_eth_softnic_flow.c > +++ b/drivers/net/softnic/rte_eth_softnic_flow.c > @@ -1448,13 +1448,6 @@ flow_rule_action_get(struct pmd_internals > *softnic, > action, > "COUNT: Null configuration"); > > - if (conf->shared) > - return rte_flow_error_set(error, > - ENOTSUP, > - RTE_FLOW_ERROR_TYPE_ACTION_CONF, > - conf, > - "COUNT: Shared counters not supported"); > - > if (n_count) > return rte_flow_error_set(error, > ENOTSUP, diff --git a/lib/ethdev/rte_flow.h > b/lib/ethdev/rte_flow.h index 7b1ed7f110..5306e8ca4b 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -75,7 +75,7 @@ extern "C" { > * At least one direction must be specified. > * > * Specifying both directions at once for a given rule is not recommended > - * but may be valid in a few cases (e.g. shared counter). > + * but may be valid in a few cases. > */ > struct rte_flow_attr { > uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,11 @@ struct > rte_flow_query_age { > * Counters can be retrieved and reset through ``rte_flow_query()``, see > * ``struct rte_flow_query_count``. > * > - * @deprecated Shared attribute is deprecated, use generic > - * RTE_FLOW_ACTION_TYPE_INDIRECT action. > - * > - * The shared flag indicates whether the counter is unique to the flow rule > the > - * action is specified with, or whether it is a shared counter. > - * > - * For a count action with the shared flag set, then then a global device > - * namespace is assumed for the counter id, so that any matched flow rules > using > - * a count action with the same counter id on the same port will contribute > to > - * that counter. > - * > * For ports within the same switch domain then the counter id namespace > extends > * to all ports within that switch domain. > */ > struct rte_flow_action_count { > - /** @deprecated Share counter ID with other flow rules. */ > - uint32_t shared:1; > - uint32_t reserved:31; /**< Reserved, must be zero. */ > + uint32_t reserved; /**< Reserved, must be zero. */ > uint32_t id; /**< Counter ID. */ }; > > -- > 2.30.2
[-- Attachment #1: Type: text/plain, Size: 30679 bytes --] On Tue, Sep 28, 2021 at 8:53 PM Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote: > > Indirect actions should be used to do shared counters. > > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > --- > app/test-pmd/cmdline_flow.c | 10 -- > doc/guides/prog_guide/rte_flow.rst | 19 +-- > doc/guides/rel_notes/deprecation.rst | 4 - > doc/guides/rel_notes/release_21_11.rst | 4 + > drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - > drivers/net/cnxk/cnxk_rte_flow.c | 8 -- > drivers/net/hns3/hns3_flow.c | 3 +- > drivers/net/ice/ice_fdir_filter.c | 4 +- > drivers/net/mlx5/mlx5.h | 7 -- > drivers/net/mlx5/mlx5_flow_dv.c | 135 ++------------------- > drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- > drivers/net/octeontx2/otx2_flow_parse.c | 10 -- > drivers/net/sfc/sfc_mae.c | 9 +- > drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- > lib/ethdev/rte_flow.h | 17 +-- > 15 files changed, 23 insertions(+), 241 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index bb22294dd3..0b5856c7d5 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -322,7 +322,6 @@ enum index { > ACTION_QUEUE_INDEX, > ACTION_DROP, > ACTION_COUNT, > - ACTION_COUNT_SHARED, > ACTION_COUNT_ID, > ACTION_RSS, > ACTION_RSS_FUNC, > @@ -1451,7 +1450,6 @@ static const enum index action_queue[] = { > > static const enum index action_count[] = { > ACTION_COUNT_ID, > - ACTION_COUNT_SHARED, > ACTION_NEXT, > ZERO, > }; > @@ -3712,14 +3710,6 @@ static const struct token token_list[] = { > .args = ARGS(ARGS_ENTRY(struct rte_flow_action_count, id)), > .call = parse_vc_conf, > }, > - [ACTION_COUNT_SHARED] = { > - .name = "shared", > - .help = "shared counter", > - .next = NEXT(action_count, NEXT_ENTRY(COMMON_BOOLEAN)), > - .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_action_count, > - shared, 1)), > - .call = parse_vc_conf, > - }, > [ACTION_RSS] = { > .name = "rss", > .help = "spread packets among several queues", > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index 2b42d5ec8c..3cb014c1fa 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -158,7 +158,7 @@ Several pattern items and actions are valid and can be used in both > directions. At least one direction must be specified. > > Specifying both directions at once for a given rule is not recommended but > -may be valid in a few cases (e.g. shared counters). > +may be valid in a few cases. > > Attribute: Transfer > ^^^^^^^^^^^^^^^^^^^ > @@ -1491,9 +1491,7 @@ Actions are performed in list order: > +=======+========+============+=======+ > | 0 | MARK | ``mark`` | 0x2a | > +-------+--------+------------+-------+ > - | 1 | COUNT | ``shared`` | 0 | > - | | +------------+-------+ > - | | | ``id`` | 0 | > + | 1 | COUNT | ``id`` | 0 | > +-------+--------+------------+-------+ > | 2 | QUEUE | ``queue`` | 10 | > +-------+--------+------------+-------+ > @@ -1734,20 +1732,9 @@ action must specify a unique id. > Counters can be retrieved and reset through ``rte_flow_query()``, see > ``struct rte_flow_query_count``. > > -The shared flag indicates whether the counter is unique to the flow rule the > -action is specified with, or whether it is a shared counter. > - > -For a count action with the shared flag set, then a global device > -namespace is assumed for the counter id, so that any matched flow rules using > -a count action with the same counter id on the same port will contribute to > -that counter. > - > For ports within the same switch domain then the counter id namespace extends > to all ports within that switch domain. > > -The shared flag is DEPRECATED and ``INDIRECT`` ``COUNT`` action should be used > -to make shared counters. > - > .. _table_rte_flow_action_count: > > .. table:: COUNT > @@ -1755,8 +1742,6 @@ to make shared counters. > +------------+---------------------------------+ > | Field | Value | > +============+=================================+ > - | ``shared`` | DEPRECATED, shared counter flag | > - +------------+---------------------------------+ > | ``id`` | counter id | > +------------+---------------------------------+ > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst > index 59445a6f42..21ef39841f 100644 > --- a/doc/guides/rel_notes/deprecation.rst > +++ b/doc/guides/rel_notes/deprecation.rst > @@ -124,10 +124,6 @@ Deprecation Notices > to support modifying fields larger than 64 bits. > In addition, documentation will be updated to clarify byte order. > > -* ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count`` > - is deprecated and will be removed in DPDK 21.11. Shared counters should > - be managed using shared actions API (``rte_flow_shared_action_create`` etc). > - > * ethdev: Definition of the flow API action ``RTE_FLOW_ACTION_TYPE_PORT_ID`` > is ambiguous and needs clarification. > Structure ``rte_flow_action_port_id`` will be extended to specify > diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst > index a84c912f20..0d91ad5d7b 100644 > --- a/doc/guides/rel_notes/release_21_11.rst > +++ b/doc/guides/rel_notes/release_21_11.rst > @@ -126,6 +126,10 @@ Removed Items > blacklist/whitelist are removed. Users must use the new > block/allow list arguments. > > +* ethdev: Removed deprecated ``shared`` attribute of the > + ``struct rte_flow_action_count``. Shared counters should be managed > + using indirect actions API (``rte_flow_action_handle_create`` etc). > + > > API Changes > ----------- > diff --git a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > index 3a9c9bba27..f1e270af8b 100644 > --- a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > +++ b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c > @@ -2111,11 +2111,6 @@ ulp_rte_count_act_handler(const struct rte_flow_action *action_item, > > act_count = action_item->conf; > if (act_count) { > - if (act_count->shared) { > - BNXT_TF_DBG(ERR, > - "Parse Error:Shared count not supported\n"); > - return BNXT_TF_RC_PARSE_ERR; > - } > memcpy(&act_prop->act_details[BNXT_ULP_ACT_PROP_IDX_COUNT], > &act_count->id, > BNXT_ULP_ACT_PROP_SZ_COUNT); > diff --git a/drivers/net/cnxk/cnxk_rte_flow.c b/drivers/net/cnxk/cnxk_rte_flow.c > index 32c1b5dee5..27defd2fa9 100644 > --- a/drivers/net/cnxk/cnxk_rte_flow.c > +++ b/drivers/net/cnxk/cnxk_rte_flow.c > @@ -110,7 +110,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr, > struct roc_npc_action in_actions[], uint32_t *flowkey_cfg) > { > struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev); > - const struct rte_flow_action_count *act_count; > const struct rte_flow_action_queue *act_q; > int i = 0, rc = 0; > int rq; > @@ -131,13 +130,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr, > break; > > case RTE_FLOW_ACTION_TYPE_COUNT: > - act_count = (const struct rte_flow_action_count *) > - actions->conf; > - > - if (act_count->shared == 1) { > - plt_npc_dbg("Shared counter is not supported"); > - goto err_exit; > - } > in_actions[i].type = ROC_NPC_ACTION_TYPE_COUNT; > break; > > diff --git a/drivers/net/hns3/hns3_flow.c b/drivers/net/hns3/hns3_flow.c > index 841e0b9da3..fe1a387526 100644 > --- a/drivers/net/hns3/hns3_flow.c > +++ b/drivers/net/hns3/hns3_flow.c > @@ -1782,8 +1782,7 @@ hns3_flow_create(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, > goto out; > > if (fdir_rule.flags & HNS3_RULE_FLAG_COUNTER) { > - ret = hns3_counter_new(dev, fdir_rule.act_cnt.shared, > - fdir_rule.act_cnt.id, error); > + ret = hns3_counter_new(dev, 0, fdir_rule.act_cnt.id, error); > if (ret) > goto out; > > diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fdir_filter.c > index e0cca7cb3c..14e4e069c4 100644 > --- a/drivers/net/ice/ice_fdir_filter.c > +++ b/drivers/net/ice/ice_fdir_filter.c > @@ -1339,9 +1339,7 @@ ice_fdir_create_filter(struct ice_adapter *ad, > if (filter->input.cnt_ena) { > struct rte_flow_action_count *act_count = &filter->act_count; > > - filter->counter = ice_fdir_counter_alloc(pf, > - act_count->shared, > - act_count->id); > + filter->counter = ice_fdir_counter_alloc(pf, 0, act_count->id); > if (!filter->counter) { > rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, NULL, > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h > index 3581414b78..ba48a0fee2 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -324,7 +324,6 @@ struct mlx5_lb_ctx { > #define MLX5_MAX_PENDING_QUERIES 4 > #define MLX5_CNT_CONTAINER_RESIZE 64 > #define MLX5_CNT_SHARED_OFFSET 0x80000000 > -#define IS_LEGACY_SHARED_CNT(cnt) (!!((cnt) & MLX5_CNT_SHARED_OFFSET)) > #define IS_BATCH_CNT(cnt) (((cnt) & (MLX5_CNT_SHARED_OFFSET - 1)) >= \ > MLX5_CNT_BATCH_OFFSET) > #define MLX5_CNT_SIZE (sizeof(struct mlx5_flow_counter)) > @@ -392,12 +391,6 @@ struct mlx5_flow_counter_shared { > }; > }; > > -/* Shared counter configuration. */ > -struct mlx5_shared_counter_conf { > - struct rte_eth_dev *dev; /* The device shared counter belongs to. */ > - uint32_t id; /* The shared counter ID. */ > -}; > - > struct mlx5_flow_counter_pool; > /* Generic counters information. */ > struct mlx5_flow_counter { > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c > index b610ad3ef4..91314d5ea5 100644 > --- a/drivers/net/mlx5/mlx5_flow_dv.c > +++ b/drivers/net/mlx5/mlx5_flow_dv.c > @@ -3308,33 +3308,11 @@ flow_dv_validate_action_set_tag(struct rte_eth_dev *dev, > return 0; > } > > -/** > - * Check if action counter is shared by either old or new mechanism. > - * > - * @param[in] action > - * Pointer to the action structure. > - * > - * @return > - * True when counter is shared, 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; > - > - if ((int)action->type == MLX5_RTE_FLOW_ACTION_TYPE_COUNT) > - return true; > - return !!(count && count->shared); > -} > - > /** > * Validate count action. > * > * @param[in] dev > * Pointer to rte_eth_dev structure. > - * @param[in] shared > - * Indicator if action is shared. > * @param[in] action_flags > * Holds the actions detected until now. > * @param[out] error > @@ -3344,7 +3322,7 @@ is_shared_action_count(const struct rte_flow_action *action) > * 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, bool shared, > +flow_dv_validate_action_count(struct rte_eth_dev *dev, > uint64_t action_flags, > struct rte_flow_error *error) > { > @@ -3356,11 +3334,6 @@ flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared, > return rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, NULL, > "duplicate count actions set"); > - 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, > - "old age and shared count combination is not supported"); > #ifdef HAVE_IBV_FLOW_DEVX_COUNTERS > return 0; > #endif > @@ -5658,9 +5631,7 @@ flow_dv_validate_action_sample(uint64_t *action_flags, > break; > case RTE_FLOW_ACTION_TYPE_COUNT: > ret = flow_dv_validate_action_count > - (dev, is_shared_action_count(act), > - *action_flags | sub_action_flags, > - error); > + (dev, *action_flags | sub_action_flags, error); > if (ret < 0) > return ret; > *count = act->conf; > @@ -6230,60 +6201,6 @@ flow_dv_counter_alloc(struct rte_eth_dev *dev, uint32_t age) > return 0; > } > > -/** > - * Allocate a shared flow counter. > - * > - * @param[in] ctx > - * Pointer to the shared counter configuration. > - * @param[in] data > - * Pointer to save the allocated counter index. > - * > - * @return > - * Index to flow counter on success, 0 otherwise and rte_errno is set. > - */ > - > -static int32_t > -flow_dv_counter_alloc_shared_cb(void *ctx, union mlx5_l3t_data *data) > -{ > - struct mlx5_shared_counter_conf *conf = ctx; > - struct rte_eth_dev *dev = conf->dev; > - struct mlx5_flow_counter *cnt; > - > - data->dword = flow_dv_counter_alloc(dev, 0); > - data->dword |= MLX5_CNT_SHARED_OFFSET; > - cnt = flow_dv_counter_get_by_idx(dev, data->dword, NULL); > - cnt->shared_info.id = conf->id; > - return 0; > -} > - > -/** > - * Get a shared flow counter. > - * > - * @param[in] dev > - * Pointer to the Ethernet device structure. > - * @param[in] id > - * Counter identifier. > - * > - * @return > - * Index to flow counter on success, 0 otherwise and rte_errno is set. > - */ > -static uint32_t > -flow_dv_counter_get_shared(struct rte_eth_dev *dev, uint32_t id) > -{ > - struct mlx5_priv *priv = dev->data->dev_private; > - struct mlx5_shared_counter_conf conf = { > - .dev = dev, > - .id = id, > - }; > - union mlx5_l3t_data data = { > - .dword = 0, > - }; > - > - mlx5_l3t_prepare_entry(priv->sh->cnt_id_tbl, id, &data, > - flow_dv_counter_alloc_shared_cb, &conf); > - return data.dword; > -} > - > /** > * Get age param from counter index. > * > @@ -6366,16 +6283,6 @@ flow_dv_counter_free(struct rte_eth_dev *dev, uint32_t counter) > if (pool->is_aged) { > flow_dv_counter_remove_from_age(dev, counter, cnt); > } else { > - /* > - * If the counter action is shared by ID, 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_LEGACY_SHARED_CNT(counter) && > - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, > - cnt->shared_info.id)) > - return; > /* > * If the counter action is shared by indirect action API, > * the atomic function reduces its references counter. > @@ -6385,8 +6292,7 @@ flow_dv_counter_free(struct rte_eth_dev *dev, uint32_t counter) > * indirect action API, shared info is 1 before the reduction, > * so this condition is failed and function doesn't return here. > */ > - if (!IS_LEGACY_SHARED_CNT(counter) && > - __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, > + if (__atomic_sub_fetch(&cnt->shared_info.refcnt, 1, > __ATOMIC_RELAXED)) > return; > } > @@ -7275,7 +7181,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, > } > for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > int type = actions->type; > - bool shared_count = false; > > if (!mlx5_flow_os_action_supported(type)) > return rte_flow_error_set(error, ENOTSUP, > @@ -7427,8 +7332,7 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, > break; > case MLX5_RTE_FLOW_ACTION_TYPE_COUNT: > case RTE_FLOW_ACTION_TYPE_COUNT: > - shared_count = is_shared_action_count(actions); > - ret = flow_dv_validate_action_count(dev, shared_count, > + ret = flow_dv_validate_action_count(dev, > action_flags, > error); > if (ret < 0) > @@ -7747,12 +7651,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, > * mutual exclusion with share counter actions. > */ > if (!priv->sh->flow_hit_aso_en) { > - if (shared_count) > - return rte_flow_error_set > - (error, EINVAL, > - RTE_FLOW_ERROR_TYPE_ACTION, > - NULL, > - "old age and shared count combination is not supported"); > if (sample_count) > return rte_flow_error_set > (error, EINVAL, > @@ -10837,16 +10735,14 @@ flow_dv_translate_action_port_id(struct rte_eth_dev *dev, > static uint32_t > flow_dv_translate_create_counter(struct rte_eth_dev *dev, > struct mlx5_flow *dev_flow, > - const struct rte_flow_action_count *count, > + const struct rte_flow_action_count *count > + __rte_unused, > const struct rte_flow_action_age *age) > { > uint32_t counter; > struct mlx5_age_param *age_param; > > - if (count && count->shared) > - counter = flow_dv_counter_get_shared(dev, count->id); > - else > - counter = flow_dv_counter_alloc(dev, !!age); > + counter = flow_dv_counter_alloc(dev, !!age); > if (!counter || age == NULL) > return counter; > age_param = flow_dv_counter_idx_get_age(dev, counter); > @@ -13216,8 +13112,7 @@ flow_dv_translate(struct rte_eth_dev *dev, > * when they are not shared. > */ > if (action_flags & MLX5_FLOW_ACTION_AGE) { > - if ((non_shared_age && > - count && !count->shared) || > + if ((non_shared_age && count) || > !(priv->sh->flow_hit_aso_en && > (attr->group || attr->transfer))) { > /* Creates age by counters. */ > @@ -17469,19 +17364,7 @@ flow_dv_action_validate(struct rte_eth_dev *dev, > "Indirect 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 indirect 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 and indirect counter is not supported"); > - return flow_dv_validate_action_count(dev, true, 0, err); > + return flow_dv_validate_action_count(dev, 0, err); > case RTE_FLOW_ACTION_TYPE_CONNTRACK: > if (!priv->sh->ct_aso_en) > return rte_flow_error_set(err, ENOTSUP, > diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c > index b93fd4d2c9..1627c3905f 100644 > --- a/drivers/net/mlx5/mlx5_flow_verbs.c > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c > @@ -250,8 +250,6 @@ flow_verbs_counter_create(struct rte_eth_dev *dev, > * > * @param[in] dev > * Pointer to the Ethernet device structure. > - * @param[in] shared > - * Indicate if this counter is shared with other flows. > * @param[in] id > * Counter identifier. > * > @@ -259,21 +257,17 @@ flow_verbs_counter_create(struct rte_eth_dev *dev, > * Index to the counter, 0 otherwise and rte_errno is set. > */ > static uint32_t > -flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) > +flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t id __rte_unused) > { > struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_flow_counter_mng *cmng = &priv->sh->cmng; > struct mlx5_flow_counter_pool *pool = NULL; > struct mlx5_flow_counter *cnt = NULL; > - union mlx5_l3t_data data; > uint32_t n_valid = cmng->n_valid; > uint32_t pool_idx, cnt_idx; > uint32_t i; > int ret; > > - if (shared && !mlx5_l3t_get_entry(priv->sh->cnt_id_tbl, id, &data) && > - data.dword) > - return data.dword; > for (pool_idx = 0; pool_idx < n_valid; ++pool_idx) { > pool = cmng->pools[pool_idx]; > if (!pool) > @@ -320,13 +314,6 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) > TAILQ_REMOVE(&pool->counters[0], cnt, next); > i = MLX5_CNT_ARRAY_IDX(pool, cnt); > cnt_idx = MLX5_MAKE_CNT_IDX(pool_idx, i); > - if (shared) { > - data.dword = cnt_idx; > - if (mlx5_l3t_set_entry(priv->sh->cnt_id_tbl, id, &data)) > - return 0; > - cnt->shared_info.id = id; > - cnt_idx |= MLX5_CNT_SHARED_OFFSET; > - } > /* Create counter with Verbs. */ > ret = flow_verbs_counter_create(dev, cnt); > if (!ret) { > @@ -352,14 +339,10 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) > static void > flow_verbs_counter_release(struct rte_eth_dev *dev, uint32_t counter) > { > - struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_flow_counter_pool *pool; > struct mlx5_flow_counter *cnt; > > cnt = flow_verbs_counter_get_by_idx(dev, counter, &pool); > - if (IS_LEGACY_SHARED_CNT(counter) && > - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, cnt->shared_info.id)) > - return; > #if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) > claim_zero(mlx5_glue->destroy_counter_set > ((struct ibv_counter_set *)cnt->dcs_when_active)); > @@ -1198,8 +1181,7 @@ flow_verbs_translate_action_count(struct mlx5_flow *dev_flow, > #endif > > if (!flow->counter) { > - flow->counter = flow_verbs_counter_new(dev, count->shared, > - count->id); > + flow->counter = flow_verbs_counter_new(dev, count->id); > if (!flow->counter) > return rte_flow_error_set(error, rte_errno, > RTE_FLOW_ERROR_TYPE_ACTION, > diff --git a/drivers/net/octeontx2/otx2_flow_parse.c b/drivers/net/octeontx2/otx2_flow_parse.c > index 63a33142a5..30a232f033 100644 > --- a/drivers/net/octeontx2/otx2_flow_parse.c > +++ b/drivers/net/octeontx2/otx2_flow_parse.c > @@ -901,7 +901,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, > struct otx2_eth_dev *hw = dev->data->dev_private; > struct otx2_npc_flow_info *npc = &hw->npc_flow; > const struct rte_flow_action_port_id *port_act; > - const struct rte_flow_action_count *act_count; > const struct rte_flow_action_mark *act_mark; > const struct rte_flow_action_queue *act_q; > const struct rte_flow_action_vf *vf_act; > @@ -947,15 +946,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, > break; > > case RTE_FLOW_ACTION_TYPE_COUNT: > - act_count = > - (const struct rte_flow_action_count *) > - actions->conf; > - > - if (act_count->shared == 1) { > - errmsg = "Shared Counters not supported"; > - errcode = ENOTSUP; > - goto err_exit; > - } > /* Indicates, need a counter */ > flow->ctr_id = 1; > req_act |= OTX2_FLOW_ACT_COUNT; > diff --git a/drivers/net/sfc/sfc_mae.c b/drivers/net/sfc/sfc_mae.c > index 4b520bc619..5fcdf9c2f5 100644 > --- a/drivers/net/sfc/sfc_mae.c > +++ b/drivers/net/sfc/sfc_mae.c > @@ -2802,16 +2802,12 @@ sfc_mae_rule_parse_action_mark(struct sfc_adapter *sa, > > static int > sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, > - const struct rte_flow_action_count *conf, > + const struct rte_flow_action_count *conf > + __rte_unused, > efx_mae_actions_t *spec) > { > int rc; > > - if (conf->shared) { > - rc = ENOTSUP; > - goto fail_counter_shared; > - } > - > if ((sa->counter_rxq.state & SFC_COUNTER_RXQ_INITIALIZED) == 0) { > sfc_err(sa, > "counter queue is not configured for COUNT action"); > @@ -2837,7 +2833,6 @@ sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, > fail_populate_count: > fail_no_service_core: > fail_counter_queue_uninit: > -fail_counter_shared: > > return rc; > } > diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c b/drivers/net/softnic/rte_eth_softnic_flow.c > index 27eaf380cd..39038d26d8 100644 > --- a/drivers/net/softnic/rte_eth_softnic_flow.c > +++ b/drivers/net/softnic/rte_eth_softnic_flow.c > @@ -1448,13 +1448,6 @@ flow_rule_action_get(struct pmd_internals *softnic, > action, > "COUNT: Null configuration"); > > - if (conf->shared) > - return rte_flow_error_set(error, > - ENOTSUP, > - RTE_FLOW_ERROR_TYPE_ACTION_CONF, > - conf, > - "COUNT: Shared counters not supported"); > - > if (n_count) > return rte_flow_error_set(error, > ENOTSUP, > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h > index 7b1ed7f110..5306e8ca4b 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -75,7 +75,7 @@ extern "C" { > * At least one direction must be specified. > * > * Specifying both directions at once for a given rule is not recommended > - * but may be valid in a few cases (e.g. shared counter). > + * but may be valid in a few cases. > */ > struct rte_flow_attr { > uint32_t group; /**< Priority group. */ > @@ -2498,24 +2498,11 @@ struct rte_flow_query_age { > * Counters can be retrieved and reset through ``rte_flow_query()``, see > * ``struct rte_flow_query_count``. > * > - * @deprecated Shared attribute is deprecated, use generic > - * RTE_FLOW_ACTION_TYPE_INDIRECT action. > - * > - * The shared flag indicates whether the counter is unique to the flow rule the > - * action is specified with, or whether it is a shared counter. > - * > - * For a count action with the shared flag set, then then a global device > - * namespace is assumed for the counter id, so that any matched flow rules using > - * a count action with the same counter id on the same port will contribute to > - * that counter. > - * > * For ports within the same switch domain then the counter id namespace extends > * to all ports within that switch domain. > */ > struct rte_flow_action_count { > - /** @deprecated Share counter ID with other flow rules. */ > - uint32_t shared:1; > - uint32_t reserved:31; /**< Reserved, must be zero. */ > + uint32_t reserved; /**< Reserved, must be zero. */ > uint32_t id; /**< Counter ID. */ > }; > > -- > 2.30.2 > Acked-by: Somnath Kotur <somnath.kotur@broadcom.com>
Hi Andrew,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> Sent: Tuesday, September 28, 2021 6:23 PM
> Subject: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter
> attribute
>
> Indirect actions should be used to do shared counters.
>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> app/test-pmd/cmdline_flow.c | 10 --
> doc/guides/prog_guide/rte_flow.rst | 19 +--
> doc/guides/rel_notes/deprecation.rst | 4 -
> doc/guides/rel_notes/release_21_11.rst | 4 +
> drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 -
> drivers/net/cnxk/cnxk_rte_flow.c | 8 --
> drivers/net/hns3/hns3_flow.c | 3 +-
> drivers/net/ice/ice_fdir_filter.c | 4 +-
> drivers/net/mlx5/mlx5.h | 7 --
> drivers/net/mlx5/mlx5_flow_dv.c | 135 ++-------------------
> drivers/net/mlx5/mlx5_flow_verbs.c | 22 +---
> drivers/net/octeontx2/otx2_flow_parse.c | 10 --
> drivers/net/sfc/sfc_mae.c | 9 +-
> drivers/net/softnic/rte_eth_softnic_flow.c | 7 --
> lib/ethdev/rte_flow.h | 17 +--
> 15 files changed, 23 insertions(+), 241 deletions(-)
>
Acked-by: Ori Kam <orika@nvidia.com>
For the rte_flow level and testpmd part.
Best,
Ori
Hi Matan, Many thanks for review. See below. Andrew. On 10/6/21 12:45 PM, Matan Azrad wrote: > Hi Andrew > > Thank you for the big effort to adjust mlx5. > I left some comments inside. > If you feel it is too much, we can do a later patch to improve. > > Matan > > >> From: Andrew Rybchenko >> Indirect actions should be used to do shared counters. >> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> [snip] >> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c >> b/drivers/net/mlx5/mlx5_flow_dv.c index b610ad3ef4..91314d5ea5 100644 >> --- a/drivers/net/mlx5/mlx5_flow_dv.c >> +++ b/drivers/net/mlx5/mlx5_flow_dv.c >> @@ -3308,33 +3308,11 @@ flow_dv_validate_action_set_tag(struct >> rte_eth_dev *dev, >> return 0; >> } >> >> -/** >> - * Check if action counter is shared by either old or new mechanism. >> - * >> - * @param[in] action >> - * Pointer to the action structure. >> - * >> - * @return >> - * True when counter is shared, 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; >> - >> - if ((int)action->type == MLX5_RTE_FLOW_ACTION_TYPE_COUNT) >> - return true; So, it is shared if and only if type is MLX5_RTE_FLOW_ACTION_TYPE_COUNT. >> - return !!(count && count->shared); >> -} >> - >> /** >> * Validate count action. >> * >> * @param[in] dev >> * Pointer to rte_eth_dev structure. >> - * @param[in] shared >> - * Indicator if action is shared. >> * @param[in] action_flags >> * Holds the actions detected until now. >> * @param[out] error >> @@ -3344,7 +3322,7 @@ is_shared_action_count(const struct >> rte_flow_action *action) >> * 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, bool shared, >> +flow_dv_validate_action_count(struct rte_eth_dev *dev, >> uint64_t action_flags, >> struct rte_flow_error *error) { @@ -3356,11 +3334,6 @@ >> flow_dv_validate_action_count(struct rte_eth_dev *dev, bool shared, >> return rte_flow_error_set(error, EINVAL, >> RTE_FLOW_ERROR_TYPE_ACTION, NULL, >> "duplicate count actions set"); >> - if (shared && (action_flags & MLX5_FLOW_ACTION_AGE) && > > This check is relevant to the indirect action case in some places, see below. Yes, this if should be preserved. Thanks. > > >> - !priv->sh->flow_hit_aso_en) >> - return rte_flow_error_set(error, EINVAL, >> - RTE_FLOW_ERROR_TYPE_ACTION, NULL, >> - "old age and shared count combination is not >> supported"); >> #ifdef HAVE_IBV_FLOW_DEVX_COUNTERS >> return 0; >> #endif >> @@ -5658,9 +5631,7 @@ flow_dv_validate_action_sample(uint64_t >> *action_flags, >> break; >> case RTE_FLOW_ACTION_TYPE_COUNT: >> ret = flow_dv_validate_action_count >> - (dev, is_shared_action_count(act), >> - *action_flags | sub_action_flags, >> - error); >> + (dev, *action_flags | sub_action_flags, >> + error); >> if (ret < 0) >> return ret; >> *count = act->conf; @@ -6230,60 +6201,6 @@ >> flow_dv_counter_alloc(struct rte_eth_dev *dev, uint32_t age) >> return 0; >> } >> >> -/** >> - * Allocate a shared flow counter. >> - * >> - * @param[in] ctx >> - * Pointer to the shared counter configuration. >> - * @param[in] data >> - * Pointer to save the allocated counter index. >> - * >> - * @return >> - * Index to flow counter on success, 0 otherwise and rte_errno is set. >> - */ >> - >> -static int32_t >> -flow_dv_counter_alloc_shared_cb(void *ctx, union mlx5_l3t_data *data) -{ >> - struct mlx5_shared_counter_conf *conf = ctx; >> - struct rte_eth_dev *dev = conf->dev; >> - struct mlx5_flow_counter *cnt; >> - >> - data->dword = flow_dv_counter_alloc(dev, 0); >> - data->dword |= MLX5_CNT_SHARED_OFFSET; >> - cnt = flow_dv_counter_get_by_idx(dev, data->dword, NULL); >> - cnt->shared_info.id = conf->id; >> - return 0; >> -} >> - >> -/** >> - * Get a shared flow counter. >> - * >> - * @param[in] dev >> - * Pointer to the Ethernet device structure. >> - * @param[in] id >> - * Counter identifier. >> - * >> - * @return >> - * Index to flow counter on success, 0 otherwise and rte_errno is set. >> - */ >> -static uint32_t >> -flow_dv_counter_get_shared(struct rte_eth_dev *dev, uint32_t id) -{ >> - struct mlx5_priv *priv = dev->data->dev_private; >> - struct mlx5_shared_counter_conf conf = { >> - .dev = dev, >> - .id = id, >> - }; >> - union mlx5_l3t_data data = { >> - .dword = 0, >> - }; >> - >> - mlx5_l3t_prepare_entry(priv->sh->cnt_id_tbl, id, &data, >> - flow_dv_counter_alloc_shared_cb, &conf); >> - return data.dword; >> -} >> - > > Need to remove cnt_id_tbl and from sh and all of its management, no? Looks. Will do in v2. > >> /** >> * Get age param from counter index. >> * >> @@ -6366,16 +6283,6 @@ flow_dv_counter_free(struct rte_eth_dev *dev, >> uint32_t counter) >> if (pool->is_aged) { >> flow_dv_counter_remove_from_age(dev, counter, cnt); >> } else { >> - /* >> - * If the counter action is shared by ID, 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_LEGACY_SHARED_CNT(counter) && >> - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, >> - cnt->shared_info.id)) >> - return; >> /* >> * If the counter action is shared by indirect action API, >> * the atomic function reduces its references counter. >> @@ -6385,8 +6292,7 @@ flow_dv_counter_free(struct rte_eth_dev *dev, >> uint32_t counter) > > You can remove the ID sharing notice in this comment. OK, will do. > >> * indirect action API, shared info is 1 before the reduction, >> * so this condition is failed and function doesn't return here. >> */ >> - if (!IS_LEGACY_SHARED_CNT(counter) && >> - __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, >> + if (__atomic_sub_fetch(&cnt->shared_info.refcnt, 1, >> __ATOMIC_RELAXED)) >> return; >> } >> @@ -7275,7 +7181,6 @@ flow_dv_validate(struct rte_eth_dev *dev, const >> struct rte_flow_attr *attr, >> } >> for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { >> int type = actions->type; >> - bool shared_count = false; >> >> if (!mlx5_flow_os_action_supported(type)) >> return rte_flow_error_set(error, ENOTSUP, @@ -7427,8 +7332,7 >> @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr >> *attr, >> break; >> case MLX5_RTE_FLOW_ACTION_TYPE_COUNT: >> case RTE_FLOW_ACTION_TYPE_COUNT: >> - shared_count = is_shared_action_count(actions); > > We need shared_count for the indirect count case(which can be shared with different flows), > So, if the action is MLX5_RTE_FLOW_ACTION_TYPE_COUNT, shared_count should be true. > It is relevant to the validate function below and for the check at the end of the function. Thanks, will fix in v2. > > >> - ret = flow_dv_validate_action_count(dev, shared_count, >> + ret = flow_dv_validate_action_count(dev, >> action_flags, >> error); >> if (ret < 0) >> @@ -17469,19 +17364,7 @@ flow_dv_action_validate(struct rte_eth_dev *dev, >> "Indirect 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 indirect 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 and indirect counter is not supported"); >> - return flow_dv_validate_action_count(dev, true, 0, err); >> + return flow_dv_validate_action_count(dev, 0, err); > > > Did you also consider improving struct mlx5_flow_counter_shared? Then, maybe we don't need it anymore. Comments say that reference counters there are used for indirect actions as well. So, I'll keep it for follow up cleanups. [snip]
Indirect actions should be used to do shared counters. Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> Acked-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com> Acked-by: Somnath Kotur <somnath.kotur@broadcom.com> Acked-by: Ori Kam <orika@nvidia.com> --- v2: - remove reserved field from count structure (review from Stephen) - apply mlx5 review notes from Matan app/test-pmd/cmdline_flow.c | 10 -- doc/guides/prog_guide/rte_flow.rst | 19 +--- doc/guides/rel_notes/deprecation.rst | 4 - doc/guides/rel_notes/release_21_11.rst | 4 + drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - drivers/net/cnxk/cnxk_rte_flow.c | 8 -- drivers/net/hns3/hns3_flow.c | 3 +- drivers/net/ice/ice_fdir_filter.c | 4 +- drivers/net/mlx5/mlx5.c | 11 -- drivers/net/mlx5/mlx5.h | 9 -- drivers/net/mlx5/mlx5_flow_dv.c | 118 ++------------------- drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- drivers/net/octeontx2/otx2_flow_parse.c | 10 -- drivers/net/sfc/sfc_mae.c | 9 +- drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- lib/ethdev/rte_flow.h | 16 +-- 16 files changed, 22 insertions(+), 237 deletions(-) diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index bb22294dd3..0b5856c7d5 100644 --- a/app/test-pmd/cmdline_flow.c +++ b/app/test-pmd/cmdline_flow.c @@ -322,7 +322,6 @@ enum index { ACTION_QUEUE_INDEX, ACTION_DROP, ACTION_COUNT, - ACTION_COUNT_SHARED, ACTION_COUNT_ID, ACTION_RSS, ACTION_RSS_FUNC, @@ -1451,7 +1450,6 @@ static const enum index action_queue[] = { static const enum index action_count[] = { ACTION_COUNT_ID, - ACTION_COUNT_SHARED, ACTION_NEXT, ZERO, }; @@ -3712,14 +3710,6 @@ static const struct token token_list[] = { .args = ARGS(ARGS_ENTRY(struct rte_flow_action_count, id)), .call = parse_vc_conf, }, - [ACTION_COUNT_SHARED] = { - .name = "shared", - .help = "shared counter", - .next = NEXT(action_count, NEXT_ENTRY(COMMON_BOOLEAN)), - .args = ARGS(ARGS_ENTRY_BF(struct rte_flow_action_count, - shared, 1)), - .call = parse_vc_conf, - }, [ACTION_RSS] = { .name = "rss", .help = "spread packets among several queues", diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst index 2b42d5ec8c..3cb014c1fa 100644 --- a/doc/guides/prog_guide/rte_flow.rst +++ b/doc/guides/prog_guide/rte_flow.rst @@ -158,7 +158,7 @@ Several pattern items and actions are valid and can be used in both directions. At least one direction must be specified. Specifying both directions at once for a given rule is not recommended but -may be valid in a few cases (e.g. shared counters). +may be valid in a few cases. Attribute: Transfer ^^^^^^^^^^^^^^^^^^^ @@ -1491,9 +1491,7 @@ Actions are performed in list order: +=======+========+============+=======+ | 0 | MARK | ``mark`` | 0x2a | +-------+--------+------------+-------+ - | 1 | COUNT | ``shared`` | 0 | - | | +------------+-------+ - | | | ``id`` | 0 | + | 1 | COUNT | ``id`` | 0 | +-------+--------+------------+-------+ | 2 | QUEUE | ``queue`` | 10 | +-------+--------+------------+-------+ @@ -1734,20 +1732,9 @@ action must specify a unique id. Counters can be retrieved and reset through ``rte_flow_query()``, see ``struct rte_flow_query_count``. -The shared flag indicates whether the counter is unique to the flow rule the -action is specified with, or whether it is a shared counter. - -For a count action with the shared flag set, then a global device -namespace is assumed for the counter id, so that any matched flow rules using -a count action with the same counter id on the same port will contribute to -that counter. - For ports within the same switch domain then the counter id namespace extends to all ports within that switch domain. -The shared flag is DEPRECATED and ``INDIRECT`` ``COUNT`` action should be used -to make shared counters. - .. _table_rte_flow_action_count: .. table:: COUNT @@ -1755,8 +1742,6 @@ to make shared counters. +------------+---------------------------------+ | Field | Value | +============+=================================+ - | ``shared`` | DEPRECATED, shared counter flag | - +------------+---------------------------------+ | ``id`` | counter id | +------------+---------------------------------+ diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst index b86147dda1..531038f589 100644 --- a/doc/guides/rel_notes/deprecation.rst +++ b/doc/guides/rel_notes/deprecation.rst @@ -118,10 +118,6 @@ Deprecation Notices to support modifying fields larger than 64 bits. In addition, documentation will be updated to clarify byte order. -* ethdev: Attribute ``shared`` of the ``struct rte_flow_action_count`` - is deprecated and will be removed in DPDK 21.11. Shared counters should - be managed using shared actions API (``rte_flow_shared_action_create`` etc). - * ethdev: Definition of the flow API action ``RTE_FLOW_ACTION_TYPE_PORT_ID`` is ambiguous and needs clarification. Structure ``rte_flow_action_port_id`` will be extended to specify diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst index 93082723cf..fe95c4aad0 100644 --- a/doc/guides/rel_notes/release_21_11.rst +++ b/doc/guides/rel_notes/release_21_11.rst @@ -164,6 +164,10 @@ Removed Items ``rte_eth_mirror_rule_reset`` along with the associated macros ``ETH_MIRROR_*`` are removed. +* ethdev: Removed deprecated ``shared`` attribute of the + ``struct rte_flow_action_count``. Shared counters should be managed + using indirect actions API (``rte_flow_action_handle_create`` etc). + * i40e: Removed i40evf driver. iavf already became the default VF driver for i40e devices, so there is no need to maintain i40evf. diff --git a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c index 3a9c9bba27..f1e270af8b 100644 --- a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +++ b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c @@ -2111,11 +2111,6 @@ ulp_rte_count_act_handler(const struct rte_flow_action *action_item, act_count = action_item->conf; if (act_count) { - if (act_count->shared) { - BNXT_TF_DBG(ERR, - "Parse Error:Shared count not supported\n"); - return BNXT_TF_RC_PARSE_ERR; - } memcpy(&act_prop->act_details[BNXT_ULP_ACT_PROP_IDX_COUNT], &act_count->id, BNXT_ULP_ACT_PROP_SZ_COUNT); diff --git a/drivers/net/cnxk/cnxk_rte_flow.c b/drivers/net/cnxk/cnxk_rte_flow.c index 32c1b5dee5..27defd2fa9 100644 --- a/drivers/net/cnxk/cnxk_rte_flow.c +++ b/drivers/net/cnxk/cnxk_rte_flow.c @@ -110,7 +110,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr, struct roc_npc_action in_actions[], uint32_t *flowkey_cfg) { struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev); - const struct rte_flow_action_count *act_count; const struct rte_flow_action_queue *act_q; int i = 0, rc = 0; int rq; @@ -131,13 +130,6 @@ cnxk_map_actions(struct rte_eth_dev *eth_dev, const struct rte_flow_attr *attr, break; case RTE_FLOW_ACTION_TYPE_COUNT: - act_count = (const struct rte_flow_action_count *) - actions->conf; - - if (act_count->shared == 1) { - plt_npc_dbg("Shared counter is not supported"); - goto err_exit; - } in_actions[i].type = ROC_NPC_ACTION_TYPE_COUNT; break; diff --git a/drivers/net/hns3/hns3_flow.c b/drivers/net/hns3/hns3_flow.c index 841e0b9da3..fe1a387526 100644 --- a/drivers/net/hns3/hns3_flow.c +++ b/drivers/net/hns3/hns3_flow.c @@ -1782,8 +1782,7 @@ hns3_flow_create(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, goto out; if (fdir_rule.flags & HNS3_RULE_FLAG_COUNTER) { - ret = hns3_counter_new(dev, fdir_rule.act_cnt.shared, - fdir_rule.act_cnt.id, error); + ret = hns3_counter_new(dev, 0, fdir_rule.act_cnt.id, error); if (ret) goto out; diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fdir_filter.c index afc956e0a2..bd627e3aa8 100644 --- a/drivers/net/ice/ice_fdir_filter.c +++ b/drivers/net/ice/ice_fdir_filter.c @@ -1337,9 +1337,7 @@ ice_fdir_create_filter(struct ice_adapter *ad, if (filter->input.cnt_ena) { struct rte_flow_action_count *act_count = &filter->act_count; - filter->counter = ice_fdir_counter_alloc(pf, - act_count->shared, - act_count->id); + filter->counter = ice_fdir_counter_alloc(pf, 0, act_count->id); if (!filter->counter) { rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ACTION, NULL, diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index 45ccfe2784..c9cc976cd5 100644 --- a/drivers/net/mlx5/mlx5.c +++ b/drivers/net/mlx5/mlx5.c @@ -1227,11 +1227,6 @@ mlx5_alloc_shared_dev_ctx(const struct mlx5_dev_spawn_data *spawn, mlx5_os_set_reg_mr_cb(&sh->share_cache.reg_mr_cb, &sh->share_cache.dereg_mr_cb); mlx5_os_dev_shared_handler_install(sh); - sh->cnt_id_tbl = mlx5_l3t_create(MLX5_L3T_TYPE_DWORD); - if (!sh->cnt_id_tbl) { - err = rte_errno; - goto error; - } if (LIST_EMPTY(&mlx5_dev_ctx_list)) { err = mlx5_flow_os_init_workspace_once(); if (err) @@ -1255,8 +1250,6 @@ mlx5_alloc_shared_dev_ctx(const struct mlx5_dev_spawn_data *spawn, pthread_mutex_destroy(&sh->txpp.mutex); pthread_mutex_unlock(&mlx5_dev_ctx_list_mutex); MLX5_ASSERT(sh); - if (sh->cnt_id_tbl) - mlx5_l3t_destroy(sh->cnt_id_tbl); if (sh->share_cache.cache.table) mlx5_mr_btree_free(&sh->share_cache.cache); if (sh->tis) @@ -1332,10 +1325,6 @@ mlx5_free_shared_dev_ctx(struct mlx5_dev_ctx_shared *sh) mlx5_aso_flow_mtrs_mng_close(sh); mlx5_flow_ipool_destroy(sh); mlx5_os_dev_shared_handler_uninstall(sh); - if (sh->cnt_id_tbl) { - mlx5_l3t_destroy(sh->cnt_id_tbl); - sh->cnt_id_tbl = NULL; - } if (sh->tx_uar) { mlx5_glue->devx_free_uar(sh->tx_uar); sh->tx_uar = NULL; diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index 3581414b78..f04725fc6e 100644 --- a/drivers/net/mlx5/mlx5.h +++ b/drivers/net/mlx5/mlx5.h @@ -324,7 +324,6 @@ struct mlx5_lb_ctx { #define MLX5_MAX_PENDING_QUERIES 4 #define MLX5_CNT_CONTAINER_RESIZE 64 #define MLX5_CNT_SHARED_OFFSET 0x80000000 -#define IS_LEGACY_SHARED_CNT(cnt) (!!((cnt) & MLX5_CNT_SHARED_OFFSET)) #define IS_BATCH_CNT(cnt) (((cnt) & (MLX5_CNT_SHARED_OFFSET - 1)) >= \ MLX5_CNT_BATCH_OFFSET) #define MLX5_CNT_SIZE (sizeof(struct mlx5_flow_counter)) @@ -392,12 +391,6 @@ struct mlx5_flow_counter_shared { }; }; -/* Shared counter configuration. */ -struct mlx5_shared_counter_conf { - struct rte_eth_dev *dev; /* The device shared counter belongs to. */ - uint32_t id; /* The shared counter ID. */ -}; - struct mlx5_flow_counter_pool; /* Generic counters information. */ struct mlx5_flow_counter { @@ -1181,8 +1174,6 @@ struct mlx5_dev_ctx_shared { void *default_miss_action; /* Default miss action. */ struct mlx5_indexed_pool *ipool[MLX5_IPOOL_MAX]; struct mlx5_indexed_pool *mdh_ipools[MLX5_MAX_MODIFY_NUM]; - /* Memory Pool for mlx5 flow resources. */ - struct mlx5_l3t_tbl *cnt_id_tbl; /* Shared counter lookup table. */ /* Shared interrupt handler section. */ struct rte_intr_handle intr_handle; /* Interrupt handler for device. */ struct rte_intr_handle intr_handle_devx; /* DEVX interrupt handler. */ diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c index c6370cd1d6..d5d9fed6c4 100644 --- a/drivers/net/mlx5/mlx5_flow_dv.c +++ b/drivers/net/mlx5/mlx5_flow_dv.c @@ -3308,26 +3308,6 @@ flow_dv_validate_action_set_tag(struct rte_eth_dev *dev, return 0; } -/** - * Check if action counter is shared by either old or new mechanism. - * - * @param[in] action - * Pointer to the action structure. - * - * @return - * True when counter is shared, 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; - - if ((int)action->type == MLX5_RTE_FLOW_ACTION_TYPE_COUNT) - return true; - return !!(count && count->shared); -} - /** * Validate count action. * @@ -5658,8 +5638,7 @@ flow_dv_validate_action_sample(uint64_t *action_flags, break; case RTE_FLOW_ACTION_TYPE_COUNT: ret = flow_dv_validate_action_count - (dev, is_shared_action_count(act), - *action_flags | sub_action_flags, + (dev, false, *action_flags | sub_action_flags, error); if (ret < 0) return ret; @@ -6230,60 +6209,6 @@ flow_dv_counter_alloc(struct rte_eth_dev *dev, uint32_t age) return 0; } -/** - * Allocate a shared flow counter. - * - * @param[in] ctx - * Pointer to the shared counter configuration. - * @param[in] data - * Pointer to save the allocated counter index. - * - * @return - * Index to flow counter on success, 0 otherwise and rte_errno is set. - */ - -static int32_t -flow_dv_counter_alloc_shared_cb(void *ctx, union mlx5_l3t_data *data) -{ - struct mlx5_shared_counter_conf *conf = ctx; - struct rte_eth_dev *dev = conf->dev; - struct mlx5_flow_counter *cnt; - - data->dword = flow_dv_counter_alloc(dev, 0); - data->dword |= MLX5_CNT_SHARED_OFFSET; - cnt = flow_dv_counter_get_by_idx(dev, data->dword, NULL); - cnt->shared_info.id = conf->id; - return 0; -} - -/** - * Get a shared flow counter. - * - * @param[in] dev - * Pointer to the Ethernet device structure. - * @param[in] id - * Counter identifier. - * - * @return - * Index to flow counter on success, 0 otherwise and rte_errno is set. - */ -static uint32_t -flow_dv_counter_get_shared(struct rte_eth_dev *dev, uint32_t id) -{ - struct mlx5_priv *priv = dev->data->dev_private; - struct mlx5_shared_counter_conf conf = { - .dev = dev, - .id = id, - }; - union mlx5_l3t_data data = { - .dword = 0, - }; - - mlx5_l3t_prepare_entry(priv->sh->cnt_id_tbl, id, &data, - flow_dv_counter_alloc_shared_cb, &conf); - return data.dword; -} - /** * Get age param from counter index. * @@ -6366,27 +6291,16 @@ flow_dv_counter_free(struct rte_eth_dev *dev, uint32_t counter) if (pool->is_aged) { flow_dv_counter_remove_from_age(dev, counter, cnt); } else { - /* - * If the counter action is shared by ID, 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_LEGACY_SHARED_CNT(counter) && - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, - cnt->shared_info.id)) - return; /* * If the counter action is shared by indirect action API, * 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. - * When the counter action is not shared neither by ID nor by + * When the counter action is not shared by * indirect action API, shared info is 1 before the reduction, * so this condition is failed and function doesn't return here. */ - if (!IS_LEGACY_SHARED_CNT(counter) && - __atomic_sub_fetch(&cnt->shared_info.refcnt, 1, + if (__atomic_sub_fetch(&cnt->shared_info.refcnt, 1, __ATOMIC_RELAXED)) return; } @@ -7426,8 +7340,9 @@ flow_dv_validate(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, ++actions_n; break; case MLX5_RTE_FLOW_ACTION_TYPE_COUNT: + shared_count = true; + /* fall-through. */ case RTE_FLOW_ACTION_TYPE_COUNT: - shared_count = is_shared_action_count(actions); ret = flow_dv_validate_action_count(dev, shared_count, action_flags, error); @@ -10837,16 +10752,14 @@ flow_dv_translate_action_port_id(struct rte_eth_dev *dev, static uint32_t flow_dv_translate_create_counter(struct rte_eth_dev *dev, struct mlx5_flow *dev_flow, - const struct rte_flow_action_count *count, + const struct rte_flow_action_count *count + __rte_unused, const struct rte_flow_action_age *age) { uint32_t counter; struct mlx5_age_param *age_param; - if (count && count->shared) - counter = flow_dv_counter_get_shared(dev, count->id); - else - counter = flow_dv_counter_alloc(dev, !!age); + counter = flow_dv_counter_alloc(dev, !!age); if (!counter || age == NULL) return counter; age_param = flow_dv_counter_idx_get_age(dev, counter); @@ -13216,8 +13129,7 @@ flow_dv_translate(struct rte_eth_dev *dev, * when they are not shared. */ if (action_flags & MLX5_FLOW_ACTION_AGE) { - if ((non_shared_age && - count && !count->shared) || + if ((non_shared_age && count) || !(priv->sh->flow_hit_aso_en && (attr->group || attr->transfer))) { /* Creates age by counters. */ @@ -17469,18 +17381,6 @@ flow_dv_action_validate(struct rte_eth_dev *dev, "Indirect 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 indirect 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 and indirect counter is not supported"); return flow_dv_validate_action_count(dev, true, 0, err); case RTE_FLOW_ACTION_TYPE_CONNTRACK: if (!priv->sh->ct_aso_en) diff --git a/drivers/net/mlx5/mlx5_flow_verbs.c b/drivers/net/mlx5/mlx5_flow_verbs.c index b93fd4d2c9..1627c3905f 100644 --- a/drivers/net/mlx5/mlx5_flow_verbs.c +++ b/drivers/net/mlx5/mlx5_flow_verbs.c @@ -250,8 +250,6 @@ flow_verbs_counter_create(struct rte_eth_dev *dev, * * @param[in] dev * Pointer to the Ethernet device structure. - * @param[in] shared - * Indicate if this counter is shared with other flows. * @param[in] id * Counter identifier. * @@ -259,21 +257,17 @@ flow_verbs_counter_create(struct rte_eth_dev *dev, * Index to the counter, 0 otherwise and rte_errno is set. */ static uint32_t -flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) +flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t id __rte_unused) { struct mlx5_priv *priv = dev->data->dev_private; struct mlx5_flow_counter_mng *cmng = &priv->sh->cmng; struct mlx5_flow_counter_pool *pool = NULL; struct mlx5_flow_counter *cnt = NULL; - union mlx5_l3t_data data; uint32_t n_valid = cmng->n_valid; uint32_t pool_idx, cnt_idx; uint32_t i; int ret; - if (shared && !mlx5_l3t_get_entry(priv->sh->cnt_id_tbl, id, &data) && - data.dword) - return data.dword; for (pool_idx = 0; pool_idx < n_valid; ++pool_idx) { pool = cmng->pools[pool_idx]; if (!pool) @@ -320,13 +314,6 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) TAILQ_REMOVE(&pool->counters[0], cnt, next); i = MLX5_CNT_ARRAY_IDX(pool, cnt); cnt_idx = MLX5_MAKE_CNT_IDX(pool_idx, i); - if (shared) { - data.dword = cnt_idx; - if (mlx5_l3t_set_entry(priv->sh->cnt_id_tbl, id, &data)) - return 0; - cnt->shared_info.id = id; - cnt_idx |= MLX5_CNT_SHARED_OFFSET; - } /* Create counter with Verbs. */ ret = flow_verbs_counter_create(dev, cnt); if (!ret) { @@ -352,14 +339,10 @@ flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, uint32_t id) static void flow_verbs_counter_release(struct rte_eth_dev *dev, uint32_t counter) { - struct mlx5_priv *priv = dev->data->dev_private; struct mlx5_flow_counter_pool *pool; struct mlx5_flow_counter *cnt; cnt = flow_verbs_counter_get_by_idx(dev, counter, &pool); - if (IS_LEGACY_SHARED_CNT(counter) && - mlx5_l3t_clear_entry(priv->sh->cnt_id_tbl, cnt->shared_info.id)) - return; #if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) claim_zero(mlx5_glue->destroy_counter_set ((struct ibv_counter_set *)cnt->dcs_when_active)); @@ -1198,8 +1181,7 @@ flow_verbs_translate_action_count(struct mlx5_flow *dev_flow, #endif if (!flow->counter) { - flow->counter = flow_verbs_counter_new(dev, count->shared, - count->id); + flow->counter = flow_verbs_counter_new(dev, count->id); if (!flow->counter) return rte_flow_error_set(error, rte_errno, RTE_FLOW_ERROR_TYPE_ACTION, diff --git a/drivers/net/octeontx2/otx2_flow_parse.c b/drivers/net/octeontx2/otx2_flow_parse.c index 63a33142a5..30a232f033 100644 --- a/drivers/net/octeontx2/otx2_flow_parse.c +++ b/drivers/net/octeontx2/otx2_flow_parse.c @@ -901,7 +901,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, struct otx2_eth_dev *hw = dev->data->dev_private; struct otx2_npc_flow_info *npc = &hw->npc_flow; const struct rte_flow_action_port_id *port_act; - const struct rte_flow_action_count *act_count; const struct rte_flow_action_mark *act_mark; const struct rte_flow_action_queue *act_q; const struct rte_flow_action_vf *vf_act; @@ -947,15 +946,6 @@ otx2_flow_parse_actions(struct rte_eth_dev *dev, break; case RTE_FLOW_ACTION_TYPE_COUNT: - act_count = - (const struct rte_flow_action_count *) - actions->conf; - - if (act_count->shared == 1) { - errmsg = "Shared Counters not supported"; - errcode = ENOTSUP; - goto err_exit; - } /* Indicates, need a counter */ flow->ctr_id = 1; req_act |= OTX2_FLOW_ACT_COUNT; diff --git a/drivers/net/sfc/sfc_mae.c b/drivers/net/sfc/sfc_mae.c index 4b520bc619..5fcdf9c2f5 100644 --- a/drivers/net/sfc/sfc_mae.c +++ b/drivers/net/sfc/sfc_mae.c @@ -2802,16 +2802,12 @@ sfc_mae_rule_parse_action_mark(struct sfc_adapter *sa, static int sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, - const struct rte_flow_action_count *conf, + const struct rte_flow_action_count *conf + __rte_unused, efx_mae_actions_t *spec) { int rc; - if (conf->shared) { - rc = ENOTSUP; - goto fail_counter_shared; - } - if ((sa->counter_rxq.state & SFC_COUNTER_RXQ_INITIALIZED) == 0) { sfc_err(sa, "counter queue is not configured for COUNT action"); @@ -2837,7 +2833,6 @@ sfc_mae_rule_parse_action_count(struct sfc_adapter *sa, fail_populate_count: fail_no_service_core: fail_counter_queue_uninit: -fail_counter_shared: return rc; } diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c b/drivers/net/softnic/rte_eth_softnic_flow.c index 7d054c38d2..ca70eab678 100644 --- a/drivers/net/softnic/rte_eth_softnic_flow.c +++ b/drivers/net/softnic/rte_eth_softnic_flow.c @@ -1448,13 +1448,6 @@ flow_rule_action_get(struct pmd_internals *softnic, action, "COUNT: Null configuration"); - if (conf->shared) - return rte_flow_error_set(error, - ENOTSUP, - RTE_FLOW_ERROR_TYPE_ACTION_CONF, - conf, - "COUNT: Shared counters not supported"); - if (n_count) return rte_flow_error_set(error, ENOTSUP, diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 7b1ed7f110..9819c25d2f 100644 --- a/lib/ethdev/rte_flow.h +++ b/lib/ethdev/rte_flow.h @@ -75,7 +75,7 @@ extern "C" { * At least one direction must be specified. * * Specifying both directions at once for a given rule is not recommended - * but may be valid in a few cases (e.g. shared counter). + * but may be valid in a few cases. */ struct rte_flow_attr { uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,10 @@ struct rte_flow_query_age { * Counters can be retrieved and reset through ``rte_flow_query()``, see * ``struct rte_flow_query_count``. * - * @deprecated Shared attribute is deprecated, use generic - * RTE_FLOW_ACTION_TYPE_INDIRECT action. - * - * The shared flag indicates whether the counter is unique to the flow rule the - * action is specified with, or whether it is a shared counter. - * - * For a count action with the shared flag set, then then a global device - * namespace is assumed for the counter id, so that any matched flow rules using - * a count action with the same counter id on the same port will contribute to - * that counter. - * * For ports within the same switch domain then the counter id namespace extends * to all ports within that switch domain. */ struct rte_flow_action_count { - /** @deprecated Share counter ID with other flow rules. */ - uint32_t shared:1; - uint32_t reserved:31; /**< Reserved, must be zero. */ uint32_t id; /**< Counter ID. */ }; -- 2.30.2
Hi Andrew, Sorry but I think I missed something. > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko > Sent: Friday, October 8, 2021 1:26 PM > > Indirect actions should be used to do shared counters. > > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > Acked-by: Thomas Monjalon <thomas@monjalon.net> > Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com> > Acked-by: Somnath Kotur <somnath.kotur@broadcom.com> > Acked-by: Ori Kam <orika@nvidia.com> > --- > v2: > - remove reserved field from count structure (review from Stephen) > - apply mlx5 review notes from Matan > > app/test-pmd/cmdline_flow.c | 10 -- > doc/guides/prog_guide/rte_flow.rst | 19 +--- > doc/guides/rel_notes/deprecation.rst | 4 - > doc/guides/rel_notes/release_21_11.rst | 4 + > drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - > drivers/net/cnxk/cnxk_rte_flow.c | 8 -- > drivers/net/hns3/hns3_flow.c | 3 +- > drivers/net/ice/ice_fdir_filter.c | 4 +- > drivers/net/mlx5/mlx5.c | 11 -- > drivers/net/mlx5/mlx5.h | 9 -- > drivers/net/mlx5/mlx5_flow_dv.c | 118 ++------------------- > drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- > drivers/net/octeontx2/otx2_flow_parse.c | 10 -- > drivers/net/sfc/sfc_mae.c | 9 +- > drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- > lib/ethdev/rte_flow.h | 16 +-- > 16 files changed, 22 insertions(+), 237 deletions(-) > [Snip] > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 7b1ed7f110..9819c25d2f 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -75,7 +75,7 @@ extern "C" { > * At least one direction must be specified. > * > * Specifying both directions at once for a given rule is not recommended > - * but may be valid in a few cases (e.g. shared counter). > + * but may be valid in a few cases. > */ > struct rte_flow_attr { > uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,10 @@ struct rte_flow_query_age > { > * Counters can be retrieved and reset through ``rte_flow_query()``, see > * ``struct rte_flow_query_count``. > * > - * @deprecated Shared attribute is deprecated, use generic > - * RTE_FLOW_ACTION_TYPE_INDIRECT action. > - * > - * The shared flag indicates whether the counter is unique to the flow rule the > - * action is specified with, or whether it is a shared counter. > - * > - * For a count action with the shared flag set, then then a global device > - * namespace is assumed for the counter id, so that any matched flow rules using > - * a count action with the same counter id on the same port will contribute to > - * that counter. > - * > * For ports within the same switch domain then the counter id namespace extends > * to all ports within that switch domain. I don't think we need this anymore. > */ > struct rte_flow_action_count { > - /** @deprecated Share counter ID with other flow rules. */ > - uint32_t shared:1; > - uint32_t reserved:31; /**< Reserved, must be zero. */ > uint32_t id; /**< Counter ID. */ Why do we need to keep the id field? > }; > > -- > 2.30.2 Best, Ori
From: Andrew Rybchenko
> Indirect actions should be used to do shared counters.
>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com> for mlx5.
Hi Ori, On 10/11/21 1:02 PM, Ori Kam wrote: > Hi Andrew, > > Sorry but I think I missed something. > >> -----Original Message----- >> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko >> Sent: Friday, October 8, 2021 1:26 PM >> >> Indirect actions should be used to do shared counters. >> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> >> Acked-by: Thomas Monjalon <thomas@monjalon.net> >> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com> >> Acked-by: Somnath Kotur <somnath.kotur@broadcom.com> >> Acked-by: Ori Kam <orika@nvidia.com> >> --- >> v2: >> - remove reserved field from count structure (review from Stephen) >> - apply mlx5 review notes from Matan >> >> app/test-pmd/cmdline_flow.c | 10 -- >> doc/guides/prog_guide/rte_flow.rst | 19 +--- >> doc/guides/rel_notes/deprecation.rst | 4 - >> doc/guides/rel_notes/release_21_11.rst | 4 + >> drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - >> drivers/net/cnxk/cnxk_rte_flow.c | 8 -- >> drivers/net/hns3/hns3_flow.c | 3 +- >> drivers/net/ice/ice_fdir_filter.c | 4 +- >> drivers/net/mlx5/mlx5.c | 11 -- >> drivers/net/mlx5/mlx5.h | 9 -- >> drivers/net/mlx5/mlx5_flow_dv.c | 118 ++------------------- >> drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- >> drivers/net/octeontx2/otx2_flow_parse.c | 10 -- >> drivers/net/sfc/sfc_mae.c | 9 +- >> drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- >> lib/ethdev/rte_flow.h | 16 +-- >> 16 files changed, 22 insertions(+), 237 deletions(-) >> > > [Snip] > >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 7b1ed7f110..9819c25d2f 100644 >> --- a/lib/ethdev/rte_flow.h >> +++ b/lib/ethdev/rte_flow.h >> @@ -75,7 +75,7 @@ extern "C" { >> * At least one direction must be specified. >> * >> * Specifying both directions at once for a given rule is not recommended >> - * but may be valid in a few cases (e.g. shared counter). >> + * but may be valid in a few cases. >> */ >> struct rte_flow_attr { >> uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,10 @@ struct rte_flow_query_age >> { >> * Counters can be retrieved and reset through ``rte_flow_query()``, see >> * ``struct rte_flow_query_count``. >> * >> - * @deprecated Shared attribute is deprecated, use generic >> - * RTE_FLOW_ACTION_TYPE_INDIRECT action. >> - * >> - * The shared flag indicates whether the counter is unique to the flow rule the >> - * action is specified with, or whether it is a shared counter. >> - * >> - * For a count action with the shared flag set, then then a global device >> - * namespace is assumed for the counter id, so that any matched flow rules using >> - * a count action with the same counter id on the same port will contribute to >> - * that counter. >> - * >> * For ports within the same switch domain then the counter id namespace extends >> * to all ports within that switch domain. > > I don't think we need this anymore. I agree. I'll remove it in v3 if required, but I hope it could be removed on apply as well. > >> */ >> struct rte_flow_action_count { >> - /** @deprecated Share counter ID with other flow rules. */ >> - uint32_t shared:1; >> - uint32_t reserved:31; /**< Reserved, must be zero. */ >> uint32_t id; /**< Counter ID. */ > > Why do we need to keep the id field? It is a very good question. I thought about it and preserved it for the corner case of two COUNT actions in the same rule. If so, id is required to distinguish on query. I don't know if we really need it to have two basically duplicate counters in the same rule. However, since order of actions matter, COUNT, VXLAN_ENCAP, COUNT should produce different byte counters. I suggest to continue discussion and gather more thought on it, but do not block the patch, since strictly speaking it is a bit separate topic as noted above. Thanks, Andrew.
Hi Andrew, > -----Original Message----- > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute > > Hi Ori, > > On 10/11/21 1:02 PM, Ori Kam wrote: > > Hi Andrew, > > > > Sorry but I think I missed something. > > > >> -----Original Message----- > >> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko > >> Sent: Friday, October 8, 2021 1:26 PM > >> > >> Indirect actions should be used to do shared counters. > >> > >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > >> Acked-by: Thomas Monjalon <thomas@monjalon.net> > >> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com> > >> Acked-by: Somnath Kotur <somnath.kotur@broadcom.com> > >> Acked-by: Ori Kam <orika@nvidia.com> > >> --- > >> v2: > >> - remove reserved field from count structure (review from Stephen) > >> - apply mlx5 review notes from Matan > >> > >> app/test-pmd/cmdline_flow.c | 10 -- > >> doc/guides/prog_guide/rte_flow.rst | 19 +--- > >> doc/guides/rel_notes/deprecation.rst | 4 - > >> doc/guides/rel_notes/release_21_11.rst | 4 + > >> drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 5 - > >> drivers/net/cnxk/cnxk_rte_flow.c | 8 -- > >> drivers/net/hns3/hns3_flow.c | 3 +- > >> drivers/net/ice/ice_fdir_filter.c | 4 +- > >> drivers/net/mlx5/mlx5.c | 11 -- > >> drivers/net/mlx5/mlx5.h | 9 -- > >> drivers/net/mlx5/mlx5_flow_dv.c | 118 ++------------------- > >> drivers/net/mlx5/mlx5_flow_verbs.c | 22 +--- > >> drivers/net/octeontx2/otx2_flow_parse.c | 10 -- > >> drivers/net/sfc/sfc_mae.c | 9 +- > >> drivers/net/softnic/rte_eth_softnic_flow.c | 7 -- > >> lib/ethdev/rte_flow.h | 16 +-- > >> 16 files changed, 22 insertions(+), 237 deletions(-) > >> > > > > [Snip] > > > >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index > >> 7b1ed7f110..9819c25d2f 100644 > >> --- a/lib/ethdev/rte_flow.h > >> +++ b/lib/ethdev/rte_flow.h > >> @@ -75,7 +75,7 @@ extern "C" { > >> * At least one direction must be specified. > >> * > >> * Specifying both directions at once for a given rule is not > >> recommended > >> - * but may be valid in a few cases (e.g. shared counter). > >> + * but may be valid in a few cases. > >> */ > >> struct rte_flow_attr { > >> uint32_t group; /**< Priority group. */ @@ -2498,24 +2498,10 @@ > >> struct rte_flow_query_age { > >> * Counters can be retrieved and reset through ``rte_flow_query()``, see > >> * ``struct rte_flow_query_count``. > >> * > >> - * @deprecated Shared attribute is deprecated, use generic > >> - * RTE_FLOW_ACTION_TYPE_INDIRECT action. > >> - * > >> - * The shared flag indicates whether the counter is unique to the > >> flow rule the > >> - * action is specified with, or whether it is a shared counter. > >> - * > >> - * For a count action with the shared flag set, then then a global > >> device > >> - * namespace is assumed for the counter id, so that any matched flow > >> rules using > >> - * a count action with the same counter id on the same port will > >> contribute to > >> - * that counter. > >> - * > >> * For ports within the same switch domain then the counter id namespace extends > >> * to all ports within that switch domain. > > > > I don't think we need this anymore. > > I agree. I'll remove it in v3 if required, but I hope it could be removed on apply as well. > > > > >> */ > >> struct rte_flow_action_count { > >> - /** @deprecated Share counter ID with other flow rules. */ > >> - uint32_t shared:1; > >> - uint32_t reserved:31; /**< Reserved, must be zero. */ > >> uint32_t id; /**< Counter ID. */ > > > > Why do we need to keep the id field? > > It is a very good question. I thought about it and preserved it for the corner case of two COUNT actions in > the same rule. > If so, id is required to distinguish on query. > I don't know if we really need it to have two basically duplicate counters in the same rule. However, since > order of actions matter, COUNT, VXLAN_ENCAP, COUNT should produce different byte counters. > Good thinking, I will not block this patch. Just please fix the comment, if it can be done in apply that will be good for me > I suggest to continue discussion and gather more thought on it, but do not block the patch, since strictly > speaking it is a bit separate topic as noted above. > Yes lets take it in one of the RTE_FLOW meetings or over mail. > Thanks, > Andrew. Best, Ori
On 10/11/2021 11:02 AM, Matan Azrad wrote:
>
>
> From: Andrew Rybchenko
>> Indirect actions should be used to do shared counters.
>>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Acked-by: Somnath Kotur <somnath.kotur@broadcom.com>
>> Acked-by: Ori Kam <orika@nvidia.com>
>
> Acked-by: Matan Azrad <matan@nvidia.com> for mlx5.
>
Applied to dpdk-next-net/main, thanks.