DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: xiangxia.m.yue@gmail.com
Cc: dev@dpdk.org, arybchenko@solarflare.com, gage.eads@intel.com,
	artem.andreev@oktetlabs.ru, jerinj@marvell.com,
	ndabilpuram@marvell.com, vattunuru@marvell.com,
	hemant.agrawal@nxp.com
Subject: Re: [dpdk-dev] [PATCH dpdk-dev v2] mempool: sort the rte_mempool_ops by name
Date: Thu, 5 Mar 2020 17:57:39 +0100	[thread overview]
Message-ID: <20200305165739.GH13822@platinum> (raw)
In-Reply-To: <1583396440-25485-1-git-send-email-xiangxia.m.yue@gmail.com>

Hi,

On Thu, Mar 05, 2020 at 04:20:40PM +0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> The order of mempool initiation affects mempool index in the
> rte_mempool_ops_table. For example, when building APPs with:
> 
> $ gcc -lrte_mempool_bucket -lrte_mempool_ring ...
> 
> The "bucket" mempool will be registered firstly, and its index
> in table is 0 while the index of "ring" mempool is 1. DPDK
> uses the mk/rte.app.mk to build APPs, and others, for example,
> Open vSwitch, use the libdpdk.a or libdpdk.so to build it.
> The mempool lib linked in dpdk and Open vSwitch is different.
> 
> The mempool can be used between primary and secondary process,
> such as dpdk-pdump and pdump-pmd/Open vSwitch(pdump enabled).
> There will be a crash because dpdk-pdump creates the "ring_mp_mc"
> ring which index in table is 0, but the index of "bucket" ring
> is 0 in Open vSwitch. If Open vSwitch use the index 0 to get
> mempool ops and malloc memory from mempool. The crash will occur:
> 
>     bucket_dequeue (access null and crash)
>     rte_mempool_get_ops (should get "ring_mp_mc",
>                          but get "bucket" mempool)
>     rte_mempool_ops_dequeue_bulk
>     ...
>     rte_pktmbuf_alloc
>     rte_pktmbuf_copy
>     pdump_copy
>     pdump_rx
>     rte_eth_rx_burst
> 
> To avoid the crash, there are some solution:
> * constructor priority: Different mempool uses different
>   priority in RTE_INIT, but it's not easy to maintain.
> 
> * change mk/rte.app.mk: Change the order in mk/rte.app.mk to
>   be same as libdpdk.a/libdpdk.so, but when adding a new mempool
>   driver in future, we must make sure the order.
> 
> * register mempool orderly: Sort the mempool when registering,
>   so the lib linked will not affect the index in mempool table.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Sorting the pool drivers certainly make things better than they
are today. However, there is still an issue as soon as the mempool
list is not the same on both sides.

I don't see any better solution anyway. Storing the ops pointers in the
shared memory won't work, since function addresses won't be the same in
the 2 processes.

Just one minor comment below.

> ---
> v2:
> 1. use the qsort to sort the mempool_ops.
> 2. tested: https://travis-ci.com/ovn-open-virtual-networks/dpdk-next-net/builds/151894026
> ---
>  lib/librte_mempool/rte_mempool_ops.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> index 22c5251..e9113cf 100644
> --- a/lib/librte_mempool/rte_mempool_ops.c
> +++ b/lib/librte_mempool/rte_mempool_ops.c
> @@ -17,6 +17,15 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
>  	.num_ops = 0
>  };
>  
> +static int
> +compare_mempool_ops(const void *a, const void *b)
> +{
> +	const struct rte_mempool_ops *m_a = a;
> +	const struct rte_mempool_ops *m_b = b;
> +
> +	return strcmp(m_a->name, m_b->name);
> +}
> +
>  /* add a new ops struct in rte_mempool_ops_table, return its index. */
>  int
>  rte_mempool_register_ops(const struct rte_mempool_ops *h)
> @@ -63,6 +72,11 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
>  	ops->get_info = h->get_info;
>  	ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
>  
> +	/* sort the rte_mempool_ops by name. the order of the mempool
> +	 * lib initiation will not affect rte_mempool_ops index. */

initiation -> initialization

there is also a checkpatch comment:

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#153: FILE: lib/librte_mempool/rte_mempool_ops.c:76:
+	 * lib initiation will not affect rte_mempool_ops index. */


> +	qsort(rte_mempool_ops_table.ops, rte_mempool_ops_table.num_ops,
> +	      sizeof(rte_mempool_ops_table.ops[0]), compare_mempool_ops);
> +
>  	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>  
>  	return ops_index;
> -- 
> 1.8.3.1
> 

Then,
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks!

  reply	other threads:[~2020-03-05 16:57 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-02  1:57 [dpdk-dev] [PATCH] " xiangxia.m.yue
2020-03-02 13:45 ` Jerin Jacob
2020-03-04 13:17   ` Tonghao Zhang
2020-03-04 13:33     ` Jerin Jacob
2020-03-04 14:46       ` Tonghao Zhang
2020-03-04 15:14         ` Jerin Jacob
2020-03-04 15:25           ` Tonghao Zhang
2020-03-05  8:20 ` [dpdk-dev] [PATCH dpdk-dev v2] " xiangxia.m.yue
2020-03-05 16:57   ` Olivier Matz [this message]
2020-03-06 13:36 ` [dpdk-dev] [PATCH dpdk-dev v3] " xiangxia.m.yue
2020-03-06 13:37   ` Jerin Jacob
2020-03-07 12:51     ` Andrew Rybchenko
2020-03-07 12:54       ` Andrew Rybchenko
2020-03-09  3:01         ` Tonghao Zhang
2020-03-09  8:27           ` Olivier Matz
2020-03-09  8:55             ` Tonghao Zhang
2020-03-09  9:05               ` Olivier Matz
2020-03-09 13:15               ` David Marchand
2020-03-16  7:43                 ` Tonghao Zhang
2020-03-16  7:55                   ` Olivier Matz
2020-03-24  9:35             ` Andrew Rybchenko
2020-03-24 12:41               ` Tonghao Zhang
2020-04-09 10:52 ` [dpdk-dev] [PATCH dpdk-dev 1/2] eal: introduce last-init queue for libraries initialization xiangxia.m.yue
2020-04-09 10:53   ` [dpdk-dev] [PATCH dpdk-dev 2/2] mempool: use shared memzone for rte_mempool_ops xiangxia.m.yue
2020-04-09 11:31   ` [dpdk-dev] [PATCH dpdk-dev 1/2] eal: introduce last-init queue for libraries initialization Jerin Jacob
2020-04-09 15:04     ` Tonghao Zhang
2020-04-09 15:02 ` [dpdk-dev] [PATCH dpdk-dev v2 1/2] eal: introduce rte-init " xiangxia.m.yue
2020-04-09 15:02   ` [dpdk-dev] [PATCH dpdk-dev v2 2/2] mempool: use shared memzone for rte_mempool_ops xiangxia.m.yue
2020-04-10  6:18   ` [dpdk-dev] [PATCH dpdk-dev v2 1/2] eal: introduce rte-init queue for libraries initialization Jerin Jacob
2020-04-10 13:11     ` Jerin Jacob
2020-04-12  3:20       ` Tonghao Zhang
2020-04-12  3:32         ` Tonghao Zhang
2020-04-13 11:32           ` Jerin Jacob
2020-04-13 14:21 ` [dpdk-dev] [PATCH dpdk-dev v3 " xiangxia.m.yue
2020-04-13 14:21   ` [dpdk-dev] [PATCH dpdk-dev v3 2/2] mempool: use shared memzone for rte_mempool_ops xiangxia.m.yue
2020-04-16 22:27     ` Thomas Monjalon
2020-04-27  8:03       ` Tonghao Zhang
2020-04-27 11:40         ` Thomas Monjalon
2020-04-27 12:51           ` Tonghao Zhang
2020-04-28 13:22             ` Tonghao Zhang
2020-05-04  7:42               ` Olivier Matz
2021-03-25 14:24                 ` David Marchand
2020-04-23 13:38     ` Andrew Rybchenko
2020-04-27  5:23       ` Tonghao Zhang

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=20200305165739.GH13822@platinum \
    --to=olivier.matz@6wind.com \
    --cc=artem.andreev@oktetlabs.ru \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=gage.eads@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerinj@marvell.com \
    --cc=ndabilpuram@marvell.com \
    --cc=vattunuru@marvell.com \
    --cc=xiangxia.m.yue@gmail.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).