net/mlx5: fix matcher caching
Checks
Commit Message
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>
---
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(-)
Comments
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@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
>
@@ -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. */
@@ -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. */
@@ -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);
}
}