Re: [dpdk-dev] [PATCH] ethdev: remove deprecated shared counter attribute

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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]