Friday, October 19, 2018 6:21 PM, Slava Ovsiienko:
> Subject: [PATCH v3 3/6] net/mlx5: flow counters simplifying runtime support
> check
How about "net/mlx5: simplify flow counters support check
>
> This part of patchset removes the redundant check of counters support in
> runtime. The flag flow_counter_en is eliminated from the code. The Verbs
> create counter function just returns an error if no counter support presented
> in kernel.
>
> Some log messages regarding the counter support type and presence are
> added.
>
> mlx5_flow_validate_action_count() is also updated due to flow_counter_en
> flag removal.
In continue to previous patch comments, this patch should only make the needed
preparation for the new counters.
The new counters macro along with implementation should be on a separate
commit.
>
> Signed-off-by: Viacheslav Ovsiienko <viachesl...@mellanox.com>
> ---
> drivers/net/mlx5/mlx5.c | 15 ++++++++++-----
> drivers/net/mlx5/mlx5.h | 1 -
> drivers/net/mlx5/mlx5_flow.c | 16 +++++++++-------
> 3 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> bb19085..8a33639 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1009,12 +1009,17 @@
> config.hw_csum = !!(attr.device_cap_flags_ex &
> IBV_DEVICE_RAW_IP_CSUM);
> DRV_LOG(DEBUG, "checksum offloading is %ssupported",
> (config.hw_csum ? "" : "not "));
> -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42
> - config.flow_counter_en = !!attr.max_counter_sets;
> +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42)
> mlx5_glue->describe_counter_set(ctx, 0, &cs_desc);
> - DRV_LOG(DEBUG, "counter type = %d, num of cs = %ld, attributes =
> %d",
> - cs_desc.counter_type, cs_desc.num_of_cs,
> - cs_desc.attributes);
> + DRV_LOG(DEBUG, "Flow counters are supported (4.2), "
> + "type = %d, num of cs = %ld, attr = %d",
> + cs_desc.counter_type,
> + cs_desc.num_of_cs,
> + cs_desc.attributes);
I would refine on saying the counters are supported (since you cannot be sure).
I would recommend to have a message log only if the macro is not defined to say
"flow counters are not supported". This is the only thing we know for sure.
> +#elif defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + DRV_LOG(DEBUG, "Flow counters are supported (4.5)"); #else
> + DRV_LOG(DEBUG, "Flow counters are not supported");
> #endif
> config.ind_table_max_size =
> attr.rss_caps.max_rwq_indirection_table_size;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> d14239c..74d87c0 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -114,7 +114,6 @@ struct mlx5_dev_config {
> unsigned int tunnel_en:1;
> /* Whether tunnel stateless offloads are supported. */
> unsigned int mpls_en:1; /* MPLS over GRE/UDP is enabled. */
> - unsigned int flow_counter_en:1; /* Whether flow counter is
> supported. */
> unsigned int cqe_comp:1; /* CQE compression is enabled. */
> unsigned int tso:1; /* Whether TSO is supported. */
> unsigned int tx_vec_en:1; /* Tx vector is enabled. */ diff --git
> a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c index
> fcabab0..c15722d 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -921,22 +921,24 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
> * 0 on success, a negative errno value otherwise and rte_ernno is set.
> */
> int
> -mlx5_flow_validate_action_count(struct rte_eth_dev *dev,
> +mlx5_flow_validate_action_count(struct rte_eth_dev *dev __rte_unused,
> const struct rte_flow_attr *attr,
> struct rte_flow_error *error)
> {
> - struct priv *priv = dev->data->dev_private;
> -
> - if (!priv->config.flow_counter_en)
> - return rte_flow_error_set(error, ENOTSUP,
> - RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> - "flow counters are not supported.");
> +#if !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) && \
> + !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)
> + (void)attr;
> + return rte_flow_error_set(error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ACTION, NULL,
> + "flow counters are not supported."); #else
This ifdef addition is not needed. Just let It fall on the counter creation
(like you commit log says).
> if (attr->egress)
> return rte_flow_error_set(error, ENOTSUP,
>
> RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, NULL,
> "count action not supported for "
> "egress");
> return 0;
> +#endif
> }
>
> /**
> --
> 1.8.3.1
