[dpdk-dev] [PATCH 4/4] net/mlx5: remove MR refcnt

Nélio Laranjeiro nelio.laranjeiro at 6wind.com
Tue Jan 16 09:09:59 CET 2018


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 <xuemingl at 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


More information about the dev mailing list