Re: [dpdk-dev] [PATCH v3 3/6] net/mlx5: flow counters simplifying runtime support check

[Shahaf Shuler <shahafs@mellanox.com>]

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 <viacheslavo@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