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 <or...@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
>
