Hi Xueming, On Mon, Jan 15, 2018 at 02:54:20PM +0800, Xueming Li wrote: > MRs are registered to priv at device start or on the fly, destroyied > when device stop. > No mR registration to perticular TXQ, MR references cache in TXQ just > part of device MR list, no reason to keep MR refcnt. > > Signed-off-by: Xueming Li <xuemi...@mellanox.com> > --- > drivers/net/mlx5/mlx5.h | 1 - > drivers/net/mlx5/mlx5_mr.c | 51 > +++++--------------------------------------- > drivers/net/mlx5/mlx5_rxq.c | 1 - > drivers/net/mlx5/mlx5_rxtx.h | 2 -- > drivers/net/mlx5/mlx5_txq.c | 18 ---------------- > 5 files changed, 5 insertions(+), 68 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h > index a6cd1474c..df6a70d96 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -318,7 +318,6 @@ int priv_socket_connect(struct priv *priv); > > struct mlx5_mr *priv_mr_new(struct priv *priv, struct rte_mempool *mp, > uintptr_t addr); > -struct mlx5_mr *priv_mr_get(struct priv *, struct rte_mempool *); > int priv_mr_release(struct priv *, struct mlx5_mr *); > int priv_mr_verify(struct priv *); > struct mlx5_mr *priv_mr_lookup(struct priv *priv, uintptr_t addr); > diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c > index 8d8fabea1..bd1be5606 100644 > --- a/drivers/net/mlx5/mlx5_mr.c > +++ b/drivers/net/mlx5/mlx5_mr.c > @@ -88,15 +88,11 @@ mlx5_mempool_register_cb(struct rte_mempool *mp, void > *opaque, > cb->error = -1; > return; > } > - mr->mp = mp; > DEBUG("mempool %p:%u area start=%p size=%zu lkey=0x%08x", > (void *)mp, mem_idx, memhdr->addr, memhdr->len, mr->mr->lkey); > mr->lkey = rte_cpu_to_be_32(mr->mr->lkey); > mr->start = (uintptr_t)memhdr->addr; > mr->end = (uintptr_t)mr->mr->addr + mr->mr->length; > - rte_atomic32_inc(&mr->refcnt); > - DEBUG("%p: new Memory Region %p refcnt: %d", (void *)priv, > - (void *)mr, rte_atomic32_read(&mr->refcnt));
Please keep the debug message. > LIST_INSERT_HEAD(&priv->mr, mr, next); > cb->mr = mr; > } > @@ -121,11 +117,8 @@ mlx5_mp2mr(struct priv *priv, struct rte_mempool *mp, > uintptr_t addr) > > priv_lock(priv); > mr = priv_mr_lookup(priv, addr); > - if (!mr) { > + if (!mr) > mr = priv_mr_new(priv, mp, addr); > - if (mr) > - rte_atomic32_inc(&mr->refcnt); > - } > priv_unlock(priv); > return mr; > } > @@ -217,35 +210,6 @@ priv_mr_new(struct priv *priv, struct rte_mempool *mp, > uintptr_t addr) > } > > /** > - * Search the memory region object in the memory region list. > - * > - * @param priv > - * Pointer to private structure. > - * @param mp > - * Pointer to the memory pool to register. > - * @return > - * The memory region on success. > - */ > -struct mlx5_mr* > -priv_mr_get(struct priv *priv, struct rte_mempool *mp) > -{ > - struct mlx5_mr *mr; > - > - assert(mp); > - if (LIST_EMPTY(&priv->mr)) > - return NULL; > - LIST_FOREACH(mr, &priv->mr, next) { > - if (mr->mp == mp) { > - rte_atomic32_inc(&mr->refcnt); > - DEBUG("Memory Region %p refcnt: %d", > - (void *)mr, rte_atomic32_read(&mr->refcnt)); > - return mr; > - } > - } > - return NULL; > -} > - > -/** > * Release the memory region object. > * > * @param mr > @@ -259,15 +223,10 @@ priv_mr_release(struct priv *priv, struct mlx5_mr *mr) > { > (void)priv; > assert(mr); > - DEBUG("Memory Region %p refcnt: %d", > - (void *)mr, rte_atomic32_read(&mr->refcnt)); > - if (rte_atomic32_dec_and_test(&mr->refcnt)) { > - claim_zero(ibv_dereg_mr(mr->mr)); > - LIST_REMOVE(mr, next); > - rte_free(mr); > - return 0; > - } > - return EBUSY; > + claim_zero(ibv_dereg_mr(mr->mr)); > + LIST_REMOVE(mr, next); > + rte_free(mr); > + return 0; > } > > /** > diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c > index a581a2d61..461b4ff91 100644 > --- a/drivers/net/mlx5/mlx5_rxq.c > +++ b/drivers/net/mlx5/mlx5_rxq.c > @@ -852,7 +852,6 @@ mlx5_priv_rxq_ibv_get(struct priv *priv, uint16_t idx) > return NULL; > rxq_ctrl = container_of(rxq_data, struct mlx5_rxq_ctrl, rxq); > if (rxq_ctrl->ibv) { > - priv_mr_get(priv, rxq_data->mp); > rte_atomic32_inc(&rxq_ctrl->ibv->refcnt); > DEBUG("%p: Verbs Rx queue %p: refcnt %d", (void *)priv, > (void *)rxq_ctrl->ibv, > diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h > index 6589a662e..0b8332cc0 100644 > --- a/drivers/net/mlx5/mlx5_rxtx.h > +++ b/drivers/net/mlx5/mlx5_rxtx.h > @@ -85,12 +85,10 @@ struct priv; > /* Memory region queue object. */ > struct mlx5_mr { > LIST_ENTRY(mlx5_mr) next; /**< Pointer to the next element. */ > - rte_atomic32_t refcnt; /*<< Reference counter. */ > uint32_t lkey; /*<< rte_cpu_to_be_32(mr->lkey) */ > uintptr_t start; /* Start address of MR */ > uintptr_t end; /* End address of MR */ > struct ibv_mr *mr; /*<< Memory Region. */ > - struct rte_mempool *mp; /*<< Memory Pool. */ > }; > > /* Compressed CQE context. */ > diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c > index 26db15a4f..3c337a4ac 100644 > --- a/drivers/net/mlx5/mlx5_txq.c > +++ b/drivers/net/mlx5/mlx5_txq.c > @@ -801,18 +801,7 @@ mlx5_priv_txq_get(struct priv *priv, uint16_t idx) > if ((*priv->txqs)[idx]) { > ctrl = container_of((*priv->txqs)[idx], struct mlx5_txq_ctrl, > txq); > - unsigned int i; > - > mlx5_priv_txq_ibv_get(priv, idx); > - for (i = 0; i != MLX5_PMD_TX_MP_CACHE; ++i) { > - struct mlx5_mr *mr = NULL; > - > - (void)mr; > - if (ctrl->txq.mp2mr[i]) { > - mr = priv_mr_get(priv, ctrl->txq.mp2mr[i]->mp); > - assert(mr); > - } > - } > rte_atomic32_inc(&ctrl->refcnt); > DEBUG("%p: Tx queue %p: refcnt %d", (void *)priv, > (void *)ctrl, rte_atomic32_read(&ctrl->refcnt)); > @@ -834,7 +823,6 @@ mlx5_priv_txq_get(struct priv *priv, uint16_t idx) > int > mlx5_priv_txq_release(struct priv *priv, uint16_t idx) > { > - unsigned int i; > struct mlx5_txq_ctrl *txq; > > if (!(*priv->txqs)[idx]) > @@ -849,12 +837,6 @@ mlx5_priv_txq_release(struct priv *priv, uint16_t idx) > if (!ret) > txq->ibv = NULL; > } > - for (i = 0; i != MLX5_PMD_TX_MP_CACHE; ++i) { > - if (txq->txq.mp2mr[i]) { > - priv_mr_release(priv, txq->txq.mp2mr[i]); > - txq->txq.mp2mr[i] = NULL; > - } > - } Nullifying the case when the queue is release should remain. It is better to detect an access to a null pointer than to a freed memory space. > if (rte_atomic32_dec_and_test(&txq->refcnt)) { > txq_free_elts(txq); > LIST_REMOVE(txq, next); > -- > 2.13.3 > Thanks, -- Nélio Laranjeiro 6WIND