Re: [PATCH v4 2/8] net/mlx4: associate MR to MP in a short function

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Matan/Ophir,

On Tue, Oct 31, 2017 at 06:21:27PM +0000, Matan Azrad wrote:
> From: Ophir Munk <ophirmu@mellanox.com>
> 
> Associate memory region to mempool (on data path) in a short function.
> Handle the less common case of adding a new memory region to mempool
> in a separate function.
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>

Thanks, a few minor comments below to address compilation issues, besides
that:

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

There's an issue about a missing inclusion of assert.h. This one should be
addressed before applying this series by the patch I submitted separately:

 "net/mlx4: fix missing include"

Can you include it in front of v5 to make things easier for Ferruh?

> ---
>  drivers/net/mlx4/mlx4.h      |  2 ++
>  drivers/net/mlx4/mlx4_mr.c   | 47 ++++++++++++++++++++++++++++++++++++
>  drivers/net/mlx4/mlx4_rxtx.c | 57 --------------------------------------------
>  drivers/net/mlx4/mlx4_rxtx.h | 31 +++++++++++++++++++++++-
>  4 files changed, 79 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index e0a9853..70cf453 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -160,5 +160,7 @@ int mlx4_flow_ctrl_set(struct rte_eth_dev *dev,
>  /* mlx4_mr.c */
>  
>  struct ibv_mr *mlx4_mp2mr(struct ibv_pd *pd, struct rte_mempool *mp);
> +uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp,
> +			 uint32_t i);
>  
>  #endif /* RTE_PMD_MLX4_H_ */
> diff --git a/drivers/net/mlx4/mlx4_mr.c b/drivers/net/mlx4/mlx4_mr.c
> index 9700884..75ee32f 100644
> --- a/drivers/net/mlx4/mlx4_mr.c
> +++ b/drivers/net/mlx4/mlx4_mr.c
> @@ -55,6 +55,7 @@

You need to include inttypes.h for PRIu32 definition.

Even though it doesn't cause a compilation failure, rte_branch_prediction.h
must also be included here for unlikely().

>  #include <rte_memory.h>
>  #include <rte_mempool.h>
>  
> +#include "mlx4_rxtx.h"
>  #include "mlx4_utils.h"
>  
>  struct mlx4_check_mempool_data {
> @@ -181,3 +182,49 @@ struct ibv_mr *
>  		rte_errno = errno ? errno : EINVAL;
>  	return mr;
>  }
> +
> +/**
> + * Add memory region (MR) <-> memory pool (MP) association to txq->mp2mr[].
> + * If mp2mr[] is full, remove an entry first.
> + *
> + * @param txq
> + *   Pointer to Tx queue structure.
> + * @param[in] mp
> + *   Memory pool for which a memory region lkey must be added.
> + * @param[in] i
> + *   Index in memory pool (MP) where to add memory region (MR).
> + *
> + * @return
> + *   Added mr->lkey on success, (uint32_t)-1 on failure.
> + */
> +uint32_t
> +mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, uint32_t i)
> +{
> +	struct ibv_mr *mr;
> +
> +	/* Add a new entry, register MR first. */
> +	DEBUG("%p: discovered new memory pool \"%s\" (%p)",
> +	      (void *)txq, mp->name, (void *)mp);
> +	mr = mlx4_mp2mr(txq->priv->pd, mp);
> +	if (unlikely(mr == NULL)) {
> +		DEBUG("%p: unable to configure MR, ibv_reg_mr() failed.",
> +		      (void *)txq);
> +		return (uint32_t)-1;
> +	}
> +	if (unlikely(i == RTE_DIM(txq->mp2mr))) {
> +		/* Table is full, remove oldest entry. */
> +		DEBUG("%p: MR <-> MP table full, dropping oldest entry.",
> +		      (void *)txq);
> +		--i;
> +		claim_zero(ibv_dereg_mr(txq->mp2mr[0].mr));
> +		memmove(&txq->mp2mr[0], &txq->mp2mr[1],
> +			(sizeof(txq->mp2mr) - sizeof(txq->mp2mr[0])));
> +	}
> +	/* Store the new entry. */
> +	txq->mp2mr[i].mp = mp;
> +	txq->mp2mr[i].mr = mr;
> +	txq->mp2mr[i].lkey = mr->lkey;
> +	DEBUG("%p: new MR lkey for MP \"%s\" (%p): 0x%08" PRIU32,
> +	      (void *)txq, mp->name, (void *)mp, txq->mp2mr[i].lkey);

PRIU32 -> PRIu32

-- 
Adrien Mazarguil
6WIND