Re: [dpdk-dev] [PATCH v2 3/7] net/mlx4: save lkey in big-endian format

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Ophir,

Some small comments, please see bellow

On Mon, Oct 23, 2017 at 02:21:56PM +0000, Ophir Munk wrote:
> mlx4 NIC is using lkey in big endian format. Save lkey in this format
> in order to avoid conversions during Tx fast path
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
>  drivers/net/mlx4/mlx4_rxtx.c | 16 +++++++---------
>  drivers/net/mlx4/mlx4_rxtx.h | 14 ++++++++------
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c
> index ae37f9b..4a77be8 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.c
> +++ b/drivers/net/mlx4/mlx4_rxtx.c
> @@ -233,9 +233,9 @@ mlx4_txq_complete(struct txq *txq)
>   *   Index in memory pool (MP) where to add memory region (MR)
>   *
>   * @return
> - *   Added mr->lkey on success, (uint32_t)-1 on failure.
> + *   Added mr->lkey (rte_be32_t) on success, (uint32_t)-1 on failure.

This comment is strange, it always returns an rte_be32_t no matter what.

>   */
> -uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, uint32_t i)
> +rte_be32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, uint32_t i)
>  {
>  	struct ibv_mr *mr;
>  
> @@ -260,9 +260,9 @@ uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, uint32_t i)
>  	/* Store the new entry. */
>  	txq->mp2mr[i].mp = mp;
>  	txq->mp2mr[i].mr = mr;
> -	txq->mp2mr[i].lkey = mr->lkey;
> +	txq->mp2mr[i].lkey = rte_cpu_to_be_32(mr->lkey);
>  	DEBUG("%p: new MR lkey for MP \"%s\" (%p): 0x%08" PRIu32,
> -	      (void *)txq, mp->name, (void *)mp, txq->mp2mr[i].lkey);
> +	      (void *)txq, mp->name, (void *)mp, mr->lkey);
>  	return txq->mp2mr[i].lkey;
>  }
>  
> @@ -289,7 +289,6 @@ mlx4_post_send(struct txq *txq, struct rte_mbuf *pkt)
>  		uint16_t flags16[2];
>  	} srcrb;
>  	uint32_t head_idx = sq->head & sq->txbb_cnt_mask;
> -	uint32_t lkey;
>  	uintptr_t addr;
>  	uint32_t owner_opcode = MLX4_OPCODE_SEND;
>  	uint32_t byte_count;
> @@ -323,10 +322,10 @@ mlx4_post_send(struct txq *txq, struct rte_mbuf *pkt)
>  		if (unlikely(dseg >= (struct mlx4_wqe_data_seg *)sq->eob))
>  			dseg = (struct mlx4_wqe_data_seg *)sq->buf;
>  		dseg->addr = rte_cpu_to_be_64(addr);
> -		/* Memory region key for this memory pool. */
> -		lkey = mlx4_txq_mp2mr(txq, mlx4_txq_mb2mp(buf));
> +		/* Memory region key (big endian) for this memory pool. */
> +		dseg->lkey = mlx4_txq_mp2mr(txq, mlx4_txq_mb2mp(buf));

Is not the comment redundant with the return type of the function?

>  #ifndef NDEBUG
> -		if (unlikely(lkey == (uint32_t)-1)) {
> +		if (unlikely(dseg->lkey == rte_cpu_to_be_32((uint32_t)-1))) {

((uint32_t)-1) == ((rte_be32_t)-1) like (uin32t_t)0 == (rte_be32_t)0.
Why do you need this extra conversion?

>  			/* MR does not exist. */
>  			DEBUG("%p: unable to get MP <-> MR association",
>  			      (void *)txq);
> @@ -341,7 +340,6 @@ mlx4_post_send(struct txq *txq, struct rte_mbuf *pkt)
>  			return -EFAULT;
>  		}
>  #endif /* NDEBUG */
> -		dseg->lkey = rte_cpu_to_be_32(lkey);
>  		if (likely(buf->data_len)) {
>  			byte_count = rte_cpu_to_be_32(buf->data_len);
>  		} else {
> diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h
> index 719ef45..b1e8ac4 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.h
> +++ b/drivers/net/mlx4/mlx4_rxtx.h
> @@ -135,7 +135,7 @@ struct txq {
>  	struct {
>  		const struct rte_mempool *mp; /**< Cached memory pool. */
>  		struct ibv_mr *mr; /**< Memory region (for mp). */
> -		uint32_t lkey; /**< mr->lkey copy. */
> +		rte_be32_t lkey; /**< mr->lkey copy. (big endian) */

The "big endian" in the comment is redundant.

>  	} mp2mr[MLX4_PMD_TX_MP_CACHE]; /**< MP to MR translation table. */
>  	struct priv *priv; /**< Back pointer to private data. */
>  	unsigned int socket; /**< CPU socket ID for allocations. */
> @@ -169,7 +169,7 @@ uint16_t mlx4_tx_burst_removed(void *dpdk_txq, struct rte_mbuf **pkts,
>  			       uint16_t pkts_n);
>  uint16_t mlx4_rx_burst_removed(void *dpdk_rxq, struct rte_mbuf **pkts,
>  			       uint16_t pkts_n);
> -uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp,
> +rte_be32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp,
>  				unsigned int i);
>  
>  /* mlx4_txq.c */
> @@ -207,9 +207,9 @@ mlx4_txq_mb2mp(struct rte_mbuf *buf)
>   *   Memory pool for which a memory region lkey must be returned.
>   *
>   * @return
> - *   mr->lkey on success, (uint32_t)-1 on failure.
> + *   mr->lkey (rte_be32_t) on success, (uint32_t)-1 on failure.

The comment is also strange, the return type does not change in the mean time.

>   */
> -static __rte_always_inline uint32_t
> +static __rte_always_inline rte_be32_t
>  mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp)
>  {
>  	unsigned int i;
> @@ -220,8 +220,10 @@ mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp)
>  			break;
>  		}
>  		if (txq->mp2mr[i].mp == mp) {
> -			assert(txq->mp2mr[i].lkey != (uint32_t)-1);
> -			assert(txq->mp2mr[i].mr->lkey == txq->mp2mr[i].lkey);
> +			assert(txq->mp2mr[i].lkey !=
> +				rte_cpu_to_be_32((uint32_t)-1));

Thanks,

-- 
Nélio Laranjeiro
6WIND