[dpdk-dev] [PATCH v2] net/mlx5: fix Direct Verbs flow matcher caching

[dpdk-dev] [PATCH v2] net/mlx5: fix Direct Verbs flow matcher caching

[Ori Kam]

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@mellanox.com>
Acked-by: Yongseok Koh <yskoh@mellanox.com>

---

V2:
 * Some cosmetics based on Yongseok comments.

---
 drivers/net/mlx5/mlx5.h         |  2 +-
 drivers/net/mlx5/mlx5_flow.h    |  5 ++-
 drivers/net/mlx5/mlx5_flow_dv.c | 77 +++++++++++++++++++++--------------------
 drivers/net/mlx5/mlx5_rxtx.h    |  7 ----
 4 files changed, 45 insertions(+), 46 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;
 	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..a91f991 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 */
 	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);
 	}
 }
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index 02034a4..d225b9c 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -171,13 +171,6 @@ struct mlx5_hrxq {
 	uint8_t rss_key[]; /* Hash key. */
 };
 
-/* List of cached objects. */
-struct mlx5_cache {
-	LIST_ENTRY(mlx5_cache) next; /* Pointer to the next element. */
-	rte_atomic32_t refcnt; /* Reference counter. */
-	void *resource; /* Cached resource */
-};
-
 /* TX queue descriptor. */
 __extension__
 struct mlx5_txq_data {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix Direct Verbs flow matcher caching

[Thomas Monjalon]

04/10/2018 07:53, Ori Kam:
> 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@mellanox.com>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>

applied to dpdk-next-net-mlx

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix Direct Verbs flow matcher caching

[Ferruh Yigit]

On 10/4/2018 9:39 PM, Thomas Monjalon wrote:
> 04/10/2018 07:53, Ori Kam:
>> 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@mellanox.com>
>> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> 
> applied to dpdk-next-net-mlx

Squashed into relevant commit in next-net, thanks.

But since this patch touches more than what it claims fixing [1], it took some
effort, final output of conflict resolving looks OK but please confirm top of
next-net.


[1]
net/mlx5: add Direct Verbs final functions
net/mlx5: add Direct Verbs translate items
net/mlx5: add Direct Verbs prepare function