[dpdk-dev] [PATCH v3 2/7] net/mlx4: associate MR to MP in a short function

Ophir Munk ophirmu at mellanox.com
Tue Oct 31 14:25:12 CET 2017


Hi,
Please see inline

On Monday, October 30, 2017 4:23 PM, Adrien Mazarguil wrote:
> 
> On Mon, Oct 30, 2017 at 10:07:24AM +0000, Matan Azrad wrote:
> > From: Ophir Munk <ophirmu at 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 at mellanox.com>
> 
> Thanks for moving that function out of the way, a few more comments
> below.
> 
> By the way, commit log lines can be up to 75 characters wide, only the title
> line is limited to 50.
> 

Commit message updated to be longer

> > ---
> >  drivers/net/mlx4/mlx4.h      |  2 ++
> >  drivers/net/mlx4/mlx4_mr.c   | 46
> +++++++++++++++++++++++++++++++++++
> >  drivers/net/mlx4/mlx4_rxtx.c | 57
> > --------------------------------------------
> >  drivers/net/mlx4/mlx4_rxtx.h | 33 ++++++++++++++++++++++++-
> >  4 files changed, 80 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..ebf6547 100644
> > --- a/drivers/net/mlx4/mlx4_mr.c
> > +++ b/drivers/net/mlx4/mlx4_mr.c
> > @@ -56,6 +56,7 @@
> >  #include <rte_mempool.h>
> >
> >  #include "mlx4_utils.h"
> > +#include "mlx4_rxtx.h"
> 
> Alphabetical order is preferred, or so I thought. Nothing seems to enforce
> this in the coding rules, it's only a suggestion.

Suggestion accepted

> 
> >
> >  struct mlx4_check_mempool_data {
> >  	int ret;
> > @@ -181,3 +182,48 @@ 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
> 
> Missing "."

Added "."

> 
> > + * @param[in] i
> > + *   Index in memory pool (MP) where to add memory region (MR)
> 
> Same here.

Done

> 
> > + *
> > + * @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)
> 
> Coding style again; return type must be on a separate line for definitions (not
> declarations).
> 
> > +{
> > +	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);
> > +	return txq->mp2mr[i].lkey;
> > +}
> > diff --git a/drivers/net/mlx4/mlx4_rxtx.c
> > b/drivers/net/mlx4/mlx4_rxtx.c index 4f899ff..f89df46 100644
> > --- a/drivers/net/mlx4/mlx4_rxtx.c
> > +++ b/drivers/net/mlx4/mlx4_rxtx.c
> > @@ -239,63 +239,6 @@ struct pv {
> >  }
> >
> >  /**
> > - * Get memory region (MR) <-> memory pool (MP) association from txq-
> >mp2mr[].
> > - * Add MP to txq->mp2mr[] if it's not registered yet. 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 returned.
> > - *
> > - * @return
> > - *   mr->lkey on success, (uint32_t)-1 on failure.
> > - */
> > -uint32_t
> > -mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp) -{
> > -	unsigned int i;
> > -	struct ibv_mr *mr;
> > -
> > -	for (i = 0; (i != RTE_DIM(txq->mp2mr)); ++i) {
> > -		if (unlikely(txq->mp2mr[i].mp == NULL)) {
> > -			/* Unknown MP, add a new MR for it. */
> > -			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);
> > -			return txq->mp2mr[i].lkey;
> > -		}
> > -	}
> > -	/* 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);
> > -	return txq->mp2mr[i].lkey;
> > -}
> > -
> > -/**
> >   * Posts a single work request to a send queue.
> >   *
> >   * @param txq
> > diff --git a/drivers/net/mlx4/mlx4_rxtx.h
> > b/drivers/net/mlx4/mlx4_rxtx.h index 7d67748..b32fcc9 100644
> > --- a/drivers/net/mlx4/mlx4_rxtx.h
> > +++ b/drivers/net/mlx4/mlx4_rxtx.h
> > @@ -36,6 +36,7 @@
> >
> >  #include <stdint.h>
> >  #include <sys/queue.h>
> > +#include <assert.h>
> >
> >  /* Verbs headers do not support -pedantic. */  #ifdef PEDANTIC @@
> > -165,7 +166,6 @@ int mlx4_rx_queue_setup(struct rte_eth_dev *dev,
> > uint16_t idx,
> >
> >  /* mlx4_rxtx.c */
> >
> > -uint32_t mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp);
> > uint16_t mlx4_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts,
> >  		       uint16_t pkts_n);
> >  uint16_t mlx4_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, @@
> > -182,4 +182,35 @@ int mlx4_tx_queue_setup(struct rte_eth_dev *dev,
> uint16_t idx,
> >  			const struct rte_eth_txconf *conf);  void
> > mlx4_tx_queue_release(void *dpdk_txq);
> >
> > +
> 
> Extra empty line, however see below.
> 

Extra line removed

> > +/**
> > + * Get memory region (MR) <-> memory pool (MP) association from txq-
> >mp2mr[].
> > + * Call mlx4_txq_add_mr() if MP is not registered yet.
> > + *
> > + * @param txq
> > + *   Pointer to Tx queue structure.
> > + * @param[in] mp
> > + *   Memory pool for which a memory region lkey must be returned.
> > + *
> > + * @return
> > + *   mr->lkey on success, (uint32_t)-1 on failure.
> > + */
> > +static inline uint32_t
> > +mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; (i != RTE_DIM(txq->mp2mr)); ++i) {
> > +		if (unlikely(txq->mp2mr[i].mp == NULL)) {
> > +			/* Unknown MP, add a new MR for it. */
> > +			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);
> > +			return txq->mp2mr[i].lkey;
> > +		}
> > +	}
> > +	return mlx4_txq_add_mr(txq, mp, i);
> > +}
> 
> I'm OK with keeping this function as a static inline defined in a header file,
> however I think mlx4_rxtx.h is not the right place for it given the remaining
> MR-handling functions are declared by mlx4.h. You should move it to that file
> as well (likely below mlx4_txq_add_mr()).
> 
> Note: RTE_DIM() and unlikely() require rte_common.h, NULL requires
> stddef.h.
>

Please note that moving mlx4_txq_mp2mr() to mlx4.h requires to "#include mlx4_rxtx.h" in mlx4.h 
(because of accessing struct txq fields).
At the same time mlx4_rxtx.h requires to "#include mlx4.h" because it uses definitions such as 
MLX4_RSS_HASH_KEY_SIZE.
End result is that mlx4.h and mlx4_rxtx.h include each other. 
It does not  compile without rewriting code (domino effect) which I don't think is a desirable result 
of this review (unless you specify otherwise).
Therefore for v4 I have left mlx4_txq_mp2mr() in mlx4_rxtx.h
 
> While there, I think you should drop both assert() statements and the reliance
> on assert.h because such statements shouldn't be part of a static inline
> function not supposed to fail. Let's assume mp2mr[] is correctly updated
> upstream.
> 

assert() - removed

> (Also missing empty line here.)
> 

Empty line added

> >  #endif /* MLX4_RXTX_H_ */
> > --
> > 1.8.3.1
> >
> 
> --
> Adrien Mazarguil
> 6WIND


More information about the dev mailing list