From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH] net/mlx5: fix matcher caching Date: Wed, 3 Oct 2018 22:46:58 +0000 Message-ID: <20181003224644.GG26206@mtidpdk.mti.labs.mlnx> References: <1538598198-148824-1-git-send-email-orika@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Shahaf Shuler , "dev@dpdk.org" To: Ori Kam Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0044.outbound.protection.outlook.com [104.47.2.44]) by dpdk.org (Postfix) with ESMTP id 0C3CF1B121 for ; Thu, 4 Oct 2018 00:47:00 +0200 (CEST) In-Reply-To: <1538598198-148824-1-git-send-email-orika@mellanox.com> Content-Language: en-US Content-ID: <3A5AE61A1B13DB4C8C56D7F6DCD87739@eurprd05.prod.outlook.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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. >=20 > 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 >=20 > This commit fixes this issue by setting that the matcher object > will include the LIST required members directly and not as a subobject. >=20 > Fixes: 8d21c3d7b237 ("net/mlx5: add Direct Verbs translate items") >=20 > Signed-off-by: Ori Kam > --- 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(-) >=20 > 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 { > =20 > /* 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 =3D dev->data->dev_private; > - struct mlx5_flow_dv_matcher *cache; > + struct mlx5_flow_dv_matcher *cache_matcher; > struct mlx5dv_flow_matcher_attr dv_attr =3D { > .type =3D IBV_FLOW_ATTR_NORMAL, > .match_mask =3D (void *)&matcher->mask, > }; > =20 > /* Lookup from cache. */ > - LIST_FOREACH(cache, &priv->matchers, cache.next) { > - if (matcher->crc =3D=3D cache->crc && > - matcher->priority =3D=3D cache->priority && > - matcher->egress =3D=3D cache->egress && > + LIST_FOREACH(cache_matcher, &priv->matchers, next) { > + if (matcher->crc =3D=3D cache_matcher->crc && > + matcher->priority =3D=3D cache_matcher->priority && > + matcher->egress =3D=3D 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 =3D 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 =3D cache_matcher; > return 0; > } > } > /* Register new matcher. */ > - cache =3D rte_calloc(__func__, 1, sizeof(*cache), 0); > - if (!cache) > + cache_matcher =3D 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 =3D *matcher; > + *cache_matcher =3D *matcher; > dv_attr.match_criteria_enable =3D > - flow_dv_matcher_enable(cache->mask.buf); > + flow_dv_matcher_enable(cache_matcher->mask.buf); > dv_attr.priority =3D matcher->priority; > if (matcher->egress) > dv_attr.flags |=3D IBV_FLOW_ATTR_FLAGS_EGRESS; > - cache->cache.resource =3D > + cache_matcher->matcher_object =3D > 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 =3D cache; > + rte_atomic32_inc(&cache_matcher->refcnt); > + LIST_INSERT_HEAD(&priv->matchers, cache_matcher, next); > + dev_flow->dv.matcher =3D 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; > } > =20 > @@ -1232,7 +1234,7 @@ > n++; > } > dv->flow =3D > - 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 =3D &matcher->cache; > + struct mlx5_flow_dv_matcher *matcher =3D flow->dv.matcher; > =20 > - 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 =3D 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); > } > } > --=20 > 1.8.3.1 >=20