[dpdk-dev] [PATCH] net/mlx5: fix matcher caching

Yongseok Koh yskoh at mellanox.com
Thu Oct 4 00:46:58 CEST 2018


On Wed, Oct 03, 2018 at 01:23:40PM -0700, Ori Kam wrote:
> The Direct Verbs are using matcher object to filter flows, This object
> can be reused for all flows that are using the same flow items and
> masks.
> 
> This was implemented with an issue, that the list pointer pointed
> to incorrect list type, this resulted in compilation error when using
> GCC greater then 4.8.5
> 
> This commit fixes this issue by setting that the matcher object
> will include the LIST required members directly and not as a subobject.
> 
> Fixes: 8d21c3d7b237 ("net/mlx5: add Direct Verbs translate items")
> 
> Signed-off-by: Ori Kam <orika at mellanox.com>
> ---

I'm not a good title writer either but my two cents:

	net/mlx5: fix Direct Verbs flow matcher caching

Minor comments below. If those are all fixed in v2, you can put my Acked-by tag.

Thanks,
Yongseok

>  drivers/net/mlx5/mlx5.h         |  2 +-
>  drivers/net/mlx5/mlx5_flow.h    |  5 ++-
>  drivers/net/mlx5/mlx5_flow_dv.c | 77 +++++++++++++++++++++--------------------
>  3 files changed, 45 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 8de0d74..4122e54 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -208,7 +208,7 @@ struct priv {
>  	LIST_HEAD(txqibv, mlx5_txq_ibv) txqsibv; /* Verbs Tx queues. */
>  	/* Verbs Indirection tables. */
>  	LIST_HEAD(ind_tables, mlx5_ind_table_ibv) ind_tbls;
> -	LIST_HEAD(matcher, mlx5_cache) matchers;
> +	LIST_HEAD(matchers, mlx5_flow_dv_matcher) matchers;

Then, please remove mlx5_cache declaration in mlx5_rxtx.h.

>  	uint32_t link_speed_capa; /* Link speed capabilities. */
>  	struct mlx5_xstats_ctrl xstats_ctrl; /* Extended stats control. */
>  	int primary_socket; /* Unix socket for primary process. */
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 10d700a..6fc5bb8 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -153,7 +153,10 @@ struct mlx5_flow_dv_match_params {
>  
>  /* Matcher structure. */
>  struct mlx5_flow_dv_matcher {
> -	struct mlx5_cache cache; /**< Cache to struct mlx5dv_flow_matcher. */
> +	LIST_ENTRY(mlx5_flow_dv_matcher) next;
> +	/* Pointer to the next element. */
> +	rte_atomic32_t refcnt; /* Reference counter. */
> +	void *matcher_object; /* Pointer to DV matcher */

Use the same doxygen comment style as others, starting from "/**<"

>  	uint16_t crc; /**< CRC of key. */
>  	uint16_t priority; /**< Priority of matcher. */
>  	uint8_t egress; /**< Egress matcher. */
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index cf663cd..7d32532 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1060,54 +1060,56 @@
>  			 struct rte_flow_error *error)
>  {
>  	struct priv *priv = dev->data->dev_private;
> -	struct mlx5_flow_dv_matcher *cache;
> +	struct mlx5_flow_dv_matcher *cache_matcher;
>  	struct mlx5dv_flow_matcher_attr dv_attr = {
>  		.type = IBV_FLOW_ATTR_NORMAL,
>  		.match_mask = (void *)&matcher->mask,
>  	};
>  
>  	/* Lookup from cache. */
> -	LIST_FOREACH(cache, &priv->matchers, cache.next) {
> -		if (matcher->crc == cache->crc &&
> -		    matcher->priority == cache->priority &&
> -		    matcher->egress == cache->egress &&
> +	LIST_FOREACH(cache_matcher, &priv->matchers, next) {
> +		if (matcher->crc == cache_matcher->crc &&
> +		    matcher->priority == cache_matcher->priority &&
> +		    matcher->egress == cache_matcher->egress &&
>  		    !memcmp((const void *)matcher->mask.buf,
> -			    (const void *)cache->mask.buf, cache->mask.size)) {
> +			    (const void *)cache_matcher->mask.buf,
> +			    cache_matcher->mask.size)) {
>  			DRV_LOG(DEBUG,
>  				"priority %hd use %s matcher %p: refcnt %d++",
> -				cache->priority, cache->egress ? "tx" : "rx",
> -				(void *)cache,
> -				rte_atomic32_read(&cache->cache.refcnt));
> -			rte_atomic32_inc(&cache->cache.refcnt);
> -			dev_flow->dv.matcher = cache;
> +				cache_matcher->priority,
> +				cache_matcher->egress ? "tx" : "rx",
> +				(void *)cache_matcher,
> +				rte_atomic32_read(&cache_matcher->refcnt));
> +			rte_atomic32_inc(&cache_matcher->refcnt);
> +			dev_flow->dv.matcher = cache_matcher;
>  			return 0;
>  		}
>  	}
>  	/* Register new matcher. */
> -	cache = rte_calloc(__func__, 1, sizeof(*cache), 0);
> -	if (!cache)
> +	cache_matcher = rte_calloc(__func__, 1, sizeof(*cache_matcher), 0);
> +	if (!cache_matcher)
>  		return rte_flow_error_set(error, ENOMEM,
>  					  RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
>  					  "cannot allocate matcher memory");
> -	*cache = *matcher;
> +	*cache_matcher = *matcher;
>  	dv_attr.match_criteria_enable =
> -		flow_dv_matcher_enable(cache->mask.buf);
> +		flow_dv_matcher_enable(cache_matcher->mask.buf);
>  	dv_attr.priority = matcher->priority;
>  	if (matcher->egress)
>  		dv_attr.flags |= IBV_FLOW_ATTR_FLAGS_EGRESS;
> -	cache->cache.resource =
> +	cache_matcher->matcher_object =
>  		mlx5_glue->dv_create_flow_matcher(priv->ctx, &dv_attr);
> -	if (!cache->cache.resource)
> +	if (!cache_matcher->matcher_object)
>  		return rte_flow_error_set(error, ENOMEM,
>  					  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  					  NULL, "cannot create matcher");
> -	rte_atomic32_inc(&cache->cache.refcnt);
> -	LIST_INSERT_HEAD(&priv->matchers, &cache->cache, next);
> -	dev_flow->dv.matcher = cache;
> +	rte_atomic32_inc(&cache_matcher->refcnt);
> +	LIST_INSERT_HEAD(&priv->matchers, cache_matcher, next);
> +	dev_flow->dv.matcher = cache_matcher;
>  	DRV_LOG(DEBUG, "priority %hd new %s matcher %p: refcnt %d",
> -		cache->priority,
> -		cache->egress ? "tx" : "rx", (void *)cache,
> -		rte_atomic32_read(&cache->cache.refcnt));
> +		cache_matcher->priority,
> +		cache_matcher->egress ? "tx" : "rx", (void *)cache_matcher,
> +		rte_atomic32_read(&cache_matcher->refcnt));
>  	return 0;
>  }
>  
> @@ -1232,7 +1234,7 @@
>  			n++;
>  		}
>  		dv->flow =
> -			mlx5_glue->dv_create_flow(dv->matcher->cache.resource,
> +			mlx5_glue->dv_create_flow(dv->matcher->matcher_object,
>  						  (void *)&dv->value, n,
>  						  dv->actions);
>  		if (!dv->flow) {
> @@ -1265,28 +1267,29 @@
>   *
>   * @param dev
>   *   Pointer to Ethernet device.
> - * @param matcher
> - *   Pointer to flow matcher.
> + * @param flow
> + *   Pointer to mlx5_flow.
>   *
>   * @return
>   *   1 while a reference on it exists, 0 when freed.
>   */
>  static int
>  flow_dv_matcher_release(struct rte_eth_dev *dev,
> -			struct mlx5_flow_dv_matcher *matcher)
> +			struct mlx5_flow *flow)
>  {
> -	struct mlx5_cache *cache = &matcher->cache;
> +	struct mlx5_flow_dv_matcher *matcher = flow->dv.matcher;
>  
> -	assert(cache->resource);
> +	assert(matcher->matcher_object);
>  	DRV_LOG(DEBUG, "port %u matcher %p: refcnt %d--",
> -		dev->data->port_id, (void *)cache,
> -		rte_atomic32_read(&cache->refcnt));
> -	if (rte_atomic32_dec_and_test(&cache->refcnt)) {
> -		claim_zero(mlx5_glue->dv_destroy_flow_matcher(cache->resource));
> -		LIST_REMOVE(cache, next);
> -		rte_free(cache);
> +		dev->data->port_id, (void *)matcher,
> +		rte_atomic32_read(&matcher->refcnt));
> +	if (rte_atomic32_dec_and_test(&matcher->refcnt)) {
> +		claim_zero(mlx5_glue->dv_destroy_flow_matcher
> +			   (matcher->matcher_object));
> +		LIST_REMOVE(matcher, next);
> +		rte_free(matcher);
>  		DRV_LOG(DEBUG, "port %u matcher %p: removed",
> -			dev->data->port_id, (void *)cache);
> +			dev->data->port_id, (void *)matcher);
>  		return 0;
>  	}
>  	return 1;
> @@ -1346,7 +1349,7 @@
>  		dev_flow = LIST_FIRST(&flow->dev_flows);
>  		LIST_REMOVE(dev_flow, next);
>  		if (dev_flow->dv.matcher)
> -			flow_dv_matcher_release(dev, dev_flow->dv.matcher);
> +			flow_dv_matcher_release(dev, dev_flow);
>  		rte_free(dev_flow);
>  	}
>  }
> -- 
> 1.8.3.1
> 


More information about the dev mailing list