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
