DPDK patches and discussions
 help / color / mirror / Atom feed
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: dev@dpdk.org, Ophir Munk <ophirmu@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v3 2/7] net/mlx4: associate MR to MP in a short function
Date: Mon, 30 Oct 2017 15:23:23 +0100	[thread overview]
Message-ID: <20171030142323.GY26782@6wind.com> (raw)
In-Reply-To: <1509358049-18854-3-git-send-email-matan@mellanox.com>

On Mon, Oct 30, 2017 at 10:07:24AM +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 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.

> ---
>  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.

>  
>  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 "."

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

Same here.

> + *
> + * @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.

> +/**
> + * 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.

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.

(Also missing empty line here.)

>  #endif /* MLX4_RXTX_H_ */
> -- 
> 1.8.3.1
> 

-- 
Adrien Mazarguil
6WIND

  reply	other threads:[~2017-10-30 14:23 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1508752838-30408-1-git-send-email-ophirmu@mellanox.com>
2017-10-23 14:21 ` [dpdk-dev] [PATCH v2 0/7] net/mlx4: follow-up on new TX datapath introduced in RC1 Ophir Munk
2017-10-23 14:21   ` [dpdk-dev] [PATCH v2 1/7] net/mlx4: remove error flows from Tx fast path Ophir Munk
2017-10-25 16:49     ` Adrien Mazarguil
2017-10-23 14:21   ` [dpdk-dev] [PATCH v2 2/7] net/mlx4: inline more Tx functions Ophir Munk
2017-10-25 16:49     ` Adrien Mazarguil
2017-10-25 21:42       ` Ophir Munk
2017-10-26  7:48         ` Adrien Mazarguil
2017-10-26 14:27           ` Ophir Munk
2017-10-29 19:30             ` Ophir Munk
2017-10-23 14:21   ` [dpdk-dev] [PATCH v2 3/7] net/mlx4: save lkey in big-endian format Ophir Munk
2017-10-23 15:24     ` Nélio Laranjeiro
2017-10-23 14:21   ` [dpdk-dev] [PATCH v2 4/7] net/mlx4: merge Tx path functions Ophir Munk
2017-10-24 13:51     ` Nélio Laranjeiro
2017-10-24 20:36       ` Ophir Munk
2017-10-25  7:50         ` Nélio Laranjeiro
2017-10-26 10:31           ` Matan Azrad
2017-10-26 12:12             ` Nélio Laranjeiro
2017-10-26 12:30               ` Matan Azrad
2017-10-26 13:44                 ` Nélio Laranjeiro
2017-10-26 16:21                   ` Matan Azrad
2017-10-23 14:21   ` [dpdk-dev] [PATCH v2 5/7] net/mlx4: remove unnecessary variables in Tx burst Ophir Munk
2017-10-25 16:49     ` Adrien Mazarguil
2017-10-23 14:21   ` [dpdk-dev] [PATCH v2 6/7] net/mlx4: improve performance of one Tx segment Ophir Munk
2017-10-25 16:50     ` Adrien Mazarguil
2017-10-23 14:22   ` [dpdk-dev] [PATCH v2 7/7] net/mlx4: separate Tx for multi-segments Ophir Munk
2017-10-25 16:50     ` Adrien Mazarguil
2017-10-30  8:15       ` Ophir Munk
2017-10-30 10:07   ` [dpdk-dev] [PATCH v3 0/7] Tx path improvements Matan Azrad
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 1/7] net/mlx4: remove error flows from Tx fast path Matan Azrad
2017-10-30 14:23       ` Adrien Mazarguil
2017-10-30 18:11         ` Matan Azrad
2017-10-31 10:16           ` Adrien Mazarguil
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 2/7] net/mlx4: associate MR to MP in a short function Matan Azrad
2017-10-30 14:23       ` Adrien Mazarguil [this message]
2017-10-31 13:25         ` Ophir Munk
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 3/7] net/mlx4: merge Tx path functions Matan Azrad
2017-10-30 14:23       ` Adrien Mazarguil
2017-10-30 18:12         ` Matan Azrad
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 4/7] net/mlx4: remove completion counter in Tx burst Matan Azrad
2017-10-30 14:23       ` Adrien Mazarguil
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 5/7] net/mlx4: separate Tx segment cases Matan Azrad
2017-10-30 14:23       ` Adrien Mazarguil
2017-10-30 18:23         ` Matan Azrad
2017-10-31 10:17           ` Adrien Mazarguil
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 6/7] net/mlx4: mitigate Tx path memory barriers Matan Azrad
2017-10-30 14:23       ` Adrien Mazarguil
2017-10-30 19:47         ` Matan Azrad
2017-10-31 10:17           ` Adrien Mazarguil
2017-10-31 11:35             ` Matan Azrad
2017-10-31 13:21               ` Adrien Mazarguil
2017-10-30 10:07     ` [dpdk-dev] [PATCH v3 7/7] net/mlx4: remove empty Tx segment support Matan Azrad
2017-10-30 14:24       ` Adrien Mazarguil
2017-10-31 18:21     ` [dpdk-dev] [PATCH v4 0/8] net/mlx4: Tx path improvements Matan Azrad
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 1/8] net/mlx4: remove error flows from Tx fast path Matan Azrad
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 2/8] net/mlx4: associate MR to MP in a short function Matan Azrad
2017-11-02 13:42         ` Adrien Mazarguil
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 3/8] net/mlx4: fix ring wraparound compiler hint Matan Azrad
2017-11-02 13:42         ` Adrien Mazarguil
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 4/8] net/mlx4: merge Tx path functions Matan Azrad
2017-11-02 13:42         ` Adrien Mazarguil
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 5/8] net/mlx4: remove duplicate handling in Tx burst Matan Azrad
2017-11-02 13:42         ` Adrien Mazarguil
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 6/8] net/mlx4: separate Tx segment cases Matan Azrad
2017-11-02 13:43         ` Adrien Mazarguil
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 7/8] net/mlx4: fix HW memory optimizations careless Matan Azrad
2017-11-02 13:43         ` Adrien Mazarguil
2017-10-31 18:21       ` [dpdk-dev] [PATCH v4 8/8] net/mlx4: mitigate Tx path memory barriers Matan Azrad
2017-11-02 13:43         ` Adrien Mazarguil
2017-11-02 13:41       ` [dpdk-dev] [PATCH] net/mlx4: fix missing include Adrien Mazarguil
2017-11-02 20:35         ` Ferruh Yigit
2017-11-02 16:42     ` [dpdk-dev] [PATCH v5 0/8] net/mlx4: Tx path improvements Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 1/8] net/mlx4: remove error flows from Tx fast path Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 2/8] net/mlx4: associate MR to MP in a short function Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 3/8] net/mlx4: fix ring wraparound compiler hint Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 4/8] net/mlx4: merge Tx path functions Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 5/8] net/mlx4: remove duplicate handling in Tx burst Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 6/8] net/mlx4: separate Tx segment cases Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 7/8] net/mlx4: fix HW memory optimizations careless Matan Azrad
2017-11-02 16:42       ` [dpdk-dev] [PATCH v5 8/8] net/mlx4: mitigate Tx path memory barriers Matan Azrad
2017-11-02 17:07       ` [dpdk-dev] [PATCH v5 0/8] net/mlx4: Tx path improvements Adrien Mazarguil
2017-11-02 20:35         ` Ferruh Yigit
2017-11-02 20:41       ` Ferruh Yigit
2017-11-03  9:48         ` Adrien Mazarguil
2017-11-03 19:25       ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171030142323.GY26782@6wind.com \
    --to=adrien.mazarguil@6wind.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=ophirmu@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).