From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: dev@dpdk.org, Ophir Munk <ophirmu@mellanox.com>
Subject: Re: [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
next prev parent 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 ` [PATCH v2 0/7] net/mlx4: follow-up on new TX datapath introduced in RC1 Ophir Munk
2017-10-23 14:21 ` [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 ` [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 ` [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 ` [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 ` [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 ` [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 ` [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 ` [PATCH v3 0/7] Tx path improvements Matan Azrad
2017-10-30 10:07 ` [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 ` [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 ` [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 ` [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 ` [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 ` [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 ` [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 ` [PATCH v4 0/8] net/mlx4: Tx path improvements Matan Azrad
2017-10-31 18:21 ` [PATCH v4 1/8] net/mlx4: remove error flows from Tx fast path Matan Azrad
2017-10-31 18:21 ` [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 ` [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 ` [PATCH v4 4/8] net/mlx4: merge Tx path functions Matan Azrad
2017-11-02 13:42 ` Adrien Mazarguil
2017-10-31 18:21 ` [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 ` [PATCH v4 6/8] net/mlx4: separate Tx segment cases Matan Azrad
2017-11-02 13:43 ` Adrien Mazarguil
2017-10-31 18:21 ` [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 ` [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 ` [PATCH] net/mlx4: fix missing include Adrien Mazarguil
2017-11-02 20:35 ` Ferruh Yigit
2017-11-02 16:42 ` [PATCH v5 0/8] net/mlx4: Tx path improvements Matan Azrad
2017-11-02 16:42 ` [PATCH v5 1/8] net/mlx4: remove error flows from Tx fast path Matan Azrad
2017-11-02 16:42 ` [PATCH v5 2/8] net/mlx4: associate MR to MP in a short function Matan Azrad
2017-11-02 16:42 ` [PATCH v5 3/8] net/mlx4: fix ring wraparound compiler hint Matan Azrad
2017-11-02 16:42 ` [PATCH v5 4/8] net/mlx4: merge Tx path functions Matan Azrad
2017-11-02 16:42 ` [PATCH v5 5/8] net/mlx4: remove duplicate handling in Tx burst Matan Azrad
2017-11-02 16:42 ` [PATCH v5 6/8] net/mlx4: separate Tx segment cases Matan Azrad
2017-11-02 16:42 ` [PATCH v5 7/8] net/mlx4: fix HW memory optimizations careless Matan Azrad
2017-11-02 16:42 ` [PATCH v5 8/8] net/mlx4: mitigate Tx path memory barriers Matan Azrad
2017-11-02 17:07 ` [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.