From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Ophir Munk <ophirmu@mellanox.com>
Cc: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>,
Olga Shern <olgas@mellanox.com>, Matan Azrad <matan@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/7] net/mlx4: inline more Tx functions
Date: Wed, 25 Oct 2017 18:49:38 +0200 [thread overview]
Message-ID: <20171025164938.GH26782@6wind.com> (raw)
In-Reply-To: <1508768520-4810-3-git-send-email-ophirmu@mellanox.com>
Hi Ophir,
On Mon, Oct 23, 2017 at 02:21:55PM +0000, Ophir Munk wrote:
> Change functions to inline on Tx fast path to improve performance
>
> Inside the inline function call other functions to handle "unlikely"
> cases such that the inline function code footprint is small.
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
Reading this, it's like adding __rte_always_inline improves performance at
all, which I doubt unless you can show proof through performance results.
When in doubt, leave it to the compiler, the static keyword is usually
enough of a hint. Too much forced inlining may actually be harmful.
What this patch really does is splitting the heavy lookup/registration
function in two halves with one small static inline function for the lookup
part that calls the separate registration part in the unlikely event MR is
not already registered.
Thankfully the compiler doesn't inline the large registration function back,
which results in the perceived performance improvement for the time being,
however there is no guarantee it won't happen in the future (you didn't use
the noinline keyword on the registration function for that).
Therefore I have a bunch of comments and suggestions, see below.
> ---
> drivers/net/mlx4/mlx4_rxtx.c | 43 ++++++------------------------------
> drivers/net/mlx4/mlx4_rxtx.h | 52 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 58 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c
> index 011ea79..ae37f9b 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.c
> +++ b/drivers/net/mlx4/mlx4_rxtx.c
> @@ -220,54 +220,25 @@ mlx4_txq_complete(struct txq *txq)
> return 0;
> }
>
> -/**
> - * Get memory pool (MP) from mbuf. If mbuf is indirect, the pool from which
> - * the cloned mbuf is allocated is returned instead.
> - *
> - * @param buf
> - * Pointer to mbuf.
> - *
> - * @return
> - * Memory pool where data is located for given mbuf.
> - */
> -static struct rte_mempool *
> -mlx4_txq_mb2mp(struct rte_mbuf *buf)
> -{
> - if (unlikely(RTE_MBUF_INDIRECT(buf)))
> - return rte_mbuf_from_indirect(buf)->pool;
> - return buf->pool;
> -}
>
> /**
> - * 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.
> + * 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 returned.
> + * 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
> - * mr->lkey on success, (uint32_t)-1 on failure.
> + * Added mr->lkey on success, (uint32_t)-1 on failure.
> */
> -uint32_t
> -mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp)
> +uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, uint32_t i)
> {
> - 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);
> diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h
> index e10bbca..719ef45 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.h
> +++ b/drivers/net/mlx4/mlx4_rxtx.h
> @@ -53,6 +53,7 @@
>
> #include "mlx4.h"
> #include "mlx4_prm.h"
> +#include "mlx4_utils.h"
Why?
>
> /** Rx queue counters. */
> struct mlx4_rxq_stats {
> @@ -160,7 +161,6 @@ void mlx4_rx_queue_release(void *dpdk_rxq);
>
> /* 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,
> @@ -169,6 +169,8 @@ 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,
> + unsigned int i);
>
> /* mlx4_txq.c */
>
> @@ -177,4 +179,52 @@ 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);
>
> +/**
> + * Get memory pool (MP) from mbuf. If mbuf is indirect, the pool from which
> + * the cloned mbuf is allocated is returned instead.
> + *
> + * @param buf
> + * Pointer to mbuf.
> + *
> + * @return
> + * Memory pool where data is located for given mbuf.
> + */
> +static __rte_always_inline struct rte_mempool *
> +mlx4_txq_mb2mp(struct rte_mbuf *buf)
> +{
> + if (unlikely(RTE_MBUF_INDIRECT(buf)))
> + return rte_mbuf_from_indirect(buf)->pool;
> + return buf->pool;
> +}
> +
> +/**
> + * 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 __rte_always_inline uint32_t
Note __rte_always_inline is defined in rte_common.h and should be explicitly
included (however don't do that, see below).
> +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);
assert() requires assert.h (but don't include it, see subsequent
suggestion).
> + return txq->mp2mr[i].lkey;
> + }
> + }
> + return mlx4_txq_add_mr(txq, mp, i);
> +}
> #endif /* MLX4_RXTX_H_ */
So as described above, these functions do not need the __rte_always_inline,
please remove it. They also do not need to be located in a header file; the
reason it's the case for their mlx5 counterparts is that they have to be
shared between vectorized/non-vectorized code. No such requirement here, you
should move them back to their original spot.
My suggestion for this performance improvement is to move mlx4_txq_add_mr()
to a different file, mlx4_mr.c looks like a good candidate. This fact will
ensure it's never inlined and far away from the data path.
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2017-10-25 16:49 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 [this message]
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
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=20171025164938.GH26782@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=olgas@mellanox.com \
--cc=ophirmu@mellanox.com \
--cc=thomas@monjalon.net \
/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).