* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-09-28 15:23 [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute Andrew Rybchenko
@ 2021-09-28 15:58 ` Stephen Hemminger
2021-09-28 16:25 ` Andrew Rybchenko
2021-10-05 19:11 ` Thomas Monjalon
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2021-09-28 15:58 UTC (permalink / raw)
To: Andrew Rybchenko
Cc: Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde, Somnath Kotur,
Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Viacheslav Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, Thomas Monjalon, Ferruh Yigit, dev
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-09-28 15:58 ` Stephen Hemminger
@ 2021-09-28 16:25 ` Andrew Rybchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Rybchenko @ 2021-09-28 16:25 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde, Somnath Kotur,
Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Viacheslav Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, Thomas Monjalon, Ferruh Yigit, dev
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-09-28 15:23 [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute Andrew Rybchenko
2021-09-28 15:58 ` Stephen Hemminger
@ 2021-10-05 19:11 ` Thomas Monjalon
2021-10-05 19:48 ` Ajit Khaparde
2021-10-06 9:45 ` Matan Azrad
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Thomas Monjalon @ 2021-10-05 19:11 UTC (permalink / raw)
To: Andrew Rybchenko
Cc: Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde, Somnath Kotur,
Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Viacheslav Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, Ferruh Yigit, dev
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 :)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-10-05 19:11 ` Thomas Monjalon
@ 2021-10-05 19:48 ` Ajit Khaparde
0 siblings, 0 replies; 15+ messages in thread
From: Ajit Khaparde @ 2021-10-05 19:48 UTC (permalink / raw)
To: Thomas Monjalon
Cc: Andrew Rybchenko, Ori Kam, Xiaoyun Li, Ray Kinsella,
Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Viacheslav Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, Ferruh Yigit, dpdk-dev
[-- 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 :)
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-09-28 15:23 [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute Andrew Rybchenko
2021-09-28 15:58 ` Stephen Hemminger
2021-10-05 19:11 ` Thomas Monjalon
@ 2021-10-06 9:45 ` Matan Azrad
2021-10-08 10:23 ` Andrew Rybchenko
2021-10-06 9:52 ` Somnath Kotur
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Matan Azrad @ 2021-10-06 9:45 UTC (permalink / raw)
To: Andrew Rybchenko, Ori Kam, Xiaoyun Li, Ray Kinsella,
Ajit Khaparde, Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Slava Ovsiienko,
Jerin Jacob, Jasvinder Singh, Cristian Dumitrescu,
NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-10-06 9:45 ` Matan Azrad
@ 2021-10-08 10:23 ` Andrew Rybchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Rybchenko @ 2021-10-08 10:23 UTC (permalink / raw)
To: Matan Azrad, Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde,
Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Slava Ovsiienko,
Jerin Jacob, Jasvinder Singh, Cristian Dumitrescu,
NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev
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]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-09-28 15:23 [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute Andrew Rybchenko
` (2 preceding siblings ...)
2021-10-06 9:45 ` Matan Azrad
@ 2021-10-06 9:52 ` Somnath Kotur
2021-10-06 12:04 ` Ori Kam
2021-10-08 10:26 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
5 siblings, 0 replies; 15+ messages in thread
From: Somnath Kotur @ 2021-10-06 9:52 UTC (permalink / raw)
To: Andrew Rybchenko
Cc: Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde,
Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Viacheslav Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, Thomas Monjalon, Ferruh Yigit, dev
[-- 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>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute
2021-09-28 15:23 [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute Andrew Rybchenko
` (3 preceding siblings ...)
2021-10-06 9:52 ` Somnath Kotur
@ 2021-10-06 12:04 ` Ori Kam
2021-10-08 10:26 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
5 siblings, 0 replies; 15+ messages in thread
From: Ori Kam @ 2021-10-06 12:04 UTC (permalink / raw)
To: Andrew Rybchenko, Xiaoyun Li, Ray Kinsella, Ajit Khaparde,
Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Slava Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
2021-09-28 15:23 [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute Andrew Rybchenko
` (4 preceding siblings ...)
2021-10-06 12:04 ` Ori Kam
@ 2021-10-08 10:26 ` Andrew Rybchenko
2021-10-11 10:02 ` Ori Kam
2021-10-11 10:02 ` Matan Azrad
5 siblings, 2 replies; 15+ messages in thread
From: Andrew Rybchenko @ 2021-10-08 10:26 UTC (permalink / raw)
To: Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde, Somnath Kotur,
Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Viacheslav Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, Thomas Monjalon, Ferruh Yigit
Cc: dev, Stephen Hemminger
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
2021-10-08 10:26 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
@ 2021-10-11 10:02 ` Ori Kam
2021-10-11 10:41 ` Andrew Rybchenko
2021-10-11 10:02 ` Matan Azrad
1 sibling, 1 reply; 15+ messages in thread
From: Ori Kam @ 2021-10-11 10:02 UTC (permalink / raw)
To: Andrew Rybchenko, Xiaoyun Li, Ray Kinsella, Ajit Khaparde,
Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Slava Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev, Stephen Hemminger
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
2021-10-11 10:02 ` Ori Kam
@ 2021-10-11 10:41 ` Andrew Rybchenko
2021-10-11 11:25 ` Ori Kam
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Rybchenko @ 2021-10-11 10:41 UTC (permalink / raw)
To: Ori Kam, Xiaoyun Li, Ray Kinsella, Ajit Khaparde, Somnath Kotur,
Nithin Dabilpuram, Kiran Kumar K, Sunil Kumar Kori, Satha Rao,
Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Slava Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev, Stephen Hemminger
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
2021-10-11 10:41 ` Andrew Rybchenko
@ 2021-10-11 11:25 ` Ori Kam
0 siblings, 0 replies; 15+ messages in thread
From: Ori Kam @ 2021-10-11 11:25 UTC (permalink / raw)
To: Andrew Rybchenko, Xiaoyun Li, Ray Kinsella, Ajit Khaparde,
Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Matan Azrad,
Slava Ovsiienko, Jerin Jacob, Jasvinder Singh,
Cristian Dumitrescu, NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev, Stephen Hemminger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
2021-10-08 10:26 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
2021-10-11 10:02 ` Ori Kam
@ 2021-10-11 10:02 ` Matan Azrad
2021-10-12 17:24 ` Ferruh Yigit
1 sibling, 1 reply; 15+ messages in thread
From: Matan Azrad @ 2021-10-11 10:02 UTC (permalink / raw)
To: Andrew Rybchenko, Ori Kam, Xiaoyun Li, Ray Kinsella,
Ajit Khaparde, Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Slava Ovsiienko,
Jerin Jacob, Jasvinder Singh, Cristian Dumitrescu,
NBU-Contact-Thomas Monjalon, Ferruh Yigit
Cc: dev, Stephen Hemminger
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dpdk-dev] [PATCH v2] ethdev: remove deprecated shared counter attribute
2021-10-11 10:02 ` Matan Azrad
@ 2021-10-12 17:24 ` Ferruh Yigit
0 siblings, 0 replies; 15+ messages in thread
From: Ferruh Yigit @ 2021-10-12 17:24 UTC (permalink / raw)
To: Matan Azrad, Andrew Rybchenko, Ori Kam, Xiaoyun Li, Ray Kinsella,
Ajit Khaparde, Somnath Kotur, Nithin Dabilpuram, Kiran Kumar K,
Sunil Kumar Kori, Satha Rao, Min Hu (Connor),
Yisen Zhuang, Lijun Ou, Qiming Yang, Qi Zhang, Slava Ovsiienko,
Jerin Jacob, Jasvinder Singh, Cristian Dumitrescu,
NBU-Contact-Thomas Monjalon
Cc: dev, Stephen Hemminger
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.
^ permalink raw reply [flat|nested] 15+ messages in thread