diff mbox series

mempool: sort the rte_mempool_ops by name

Message ID 1583114253-15345-1-git-send-email-xiangxia.m.yue@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series mempool: sort the rte_mempool_ops by name |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/travis-robot success Travis build: passed

Commit Message

Tonghao Zhang March 2, 2020, 1:57 a.m. UTC 
  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>
---
 lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Jerin Jacob March 2, 2020, 1:45 p.m. UTC | #1 
On Mon, Mar 2, 2020 at 7:27 AM <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>
> ---
>  lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> index 22c5251..06dfe16 100644
> --- a/lib/librte_mempool/rte_mempool_ops.c
> +++ b/lib/librte_mempool/rte_mempool_ops.c
> @@ -22,7 +22,7 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
>  rte_mempool_register_ops(const struct rte_mempool_ops *h)
>  {
>         struct rte_mempool_ops *ops;
> -       int16_t ops_index;
> +       unsigned ops_index, i;
>
>         rte_spinlock_lock(&rte_mempool_ops_table.sl);
>
> @@ -50,7 +50,19 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
>                 return -EEXIST;
>         }
>
> -       ops_index = rte_mempool_ops_table.num_ops++;
> +       /* sort the rte_mempool_ops by name. the order of the mempool
> +        * lib initiation will not affect rte_mempool_ops index. */

+1 for the fix.
For the implementation, why not use qsort_r() for sorting?


> +       ops_index = rte_mempool_ops_table.num_ops;
> +       for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> +               if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
> +                       do {
> +                               rte_mempool_ops_table.ops[ops_index] =
> +                                       rte_mempool_ops_table.ops[ops_index -1];
> +                       } while (--ops_index > i);
> +                       break;
> +               }
> +       }
> +
>         ops = &rte_mempool_ops_table.ops[ops_index];
>         strlcpy(ops->name, h->name, sizeof(ops->name));
>         ops->alloc = h->alloc;
> @@ -63,6 +75,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
>         ops->get_info = h->get_info;
>         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
>
> +       rte_mempool_ops_table.num_ops++;
> +
>         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>
>         return ops_index;
> --
> 1.8.3.1
>
Tonghao Zhang March 4, 2020, 1:17 p.m. UTC | #2 
On Mon, Mar 2, 2020 at 9:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Mon, Mar 2, 2020 at 7:27 AM <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>
> > ---
> >  lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> > index 22c5251..06dfe16 100644
> > --- a/lib/librte_mempool/rte_mempool_ops.c
> > +++ b/lib/librte_mempool/rte_mempool_ops.c
> > @@ -22,7 +22,7 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> >  rte_mempool_register_ops(const struct rte_mempool_ops *h)
> >  {
> >         struct rte_mempool_ops *ops;
> > -       int16_t ops_index;
> > +       unsigned ops_index, i;
> >
> >         rte_spinlock_lock(&rte_mempool_ops_table.sl);
> >
> > @@ -50,7 +50,19 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> >                 return -EEXIST;
> >         }
> >
> > -       ops_index = rte_mempool_ops_table.num_ops++;
> > +       /* sort the rte_mempool_ops by name. the order of the mempool
> > +        * lib initiation will not affect rte_mempool_ops index. */
>
> +1 for the fix.
> For the implementation, why not use qsort_r() for sorting?
The implementation is easy, and the number of mempool driver is not too large.
But we can use the qsort_r to implement it.
>
> > +       ops_index = rte_mempool_ops_table.num_ops;
> > +       for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> > +               if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
> > +                       do {
> > +                               rte_mempool_ops_table.ops[ops_index] =
> > +                                       rte_mempool_ops_table.ops[ops_index -1];
> > +                       } while (--ops_index > i);
> > +                       break;
> > +               }
> > +       }
> > +
> >         ops = &rte_mempool_ops_table.ops[ops_index];
> >         strlcpy(ops->name, h->name, sizeof(ops->name));
> >         ops->alloc = h->alloc;
> > @@ -63,6 +75,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> >         ops->get_info = h->get_info;
> >         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
> >
> > +       rte_mempool_ops_table.num_ops++;
> > +
> >         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> >
> >         return ops_index;
> > --
> > 1.8.3.1
> >
Jerin Jacob March 4, 2020, 1:33 p.m. UTC | #3 
On Wed, Mar 4, 2020 at 6:48 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Mar 2, 2020 at 9:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Mon, Mar 2, 2020 at 7:27 AM <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>
> > > ---
> > >  lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> > > index 22c5251..06dfe16 100644
> > > --- a/lib/librte_mempool/rte_mempool_ops.c
> > > +++ b/lib/librte_mempool/rte_mempool_ops.c
> > > @@ -22,7 +22,7 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > >  rte_mempool_register_ops(const struct rte_mempool_ops *h)
> > >  {
> > >         struct rte_mempool_ops *ops;
> > > -       int16_t ops_index;
> > > +       unsigned ops_index, i;
> > >
> > >         rte_spinlock_lock(&rte_mempool_ops_table.sl);
> > >
> > > @@ -50,7 +50,19 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > >                 return -EEXIST;
> > >         }
> > >
> > > -       ops_index = rte_mempool_ops_table.num_ops++;
> > > +       /* sort the rte_mempool_ops by name. the order of the mempool
> > > +        * lib initiation will not affect rte_mempool_ops index. */
> >
> > +1 for the fix.
> > For the implementation, why not use qsort_r() for sorting?
> The implementation is easy, and the number of mempool driver is not too large.
> But we can use the qsort_r to implement it.

Since it is in a slow path, IMO, better to use standard sort functions
for better readability.


> >
> > > +       ops_index = rte_mempool_ops_table.num_ops;
> > > +       for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> > > +               if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
> > > +                       do {
> > > +                               rte_mempool_ops_table.ops[ops_index] =
> > > +                                       rte_mempool_ops_table.ops[ops_index -1];
> > > +                       } while (--ops_index > i);
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > >         ops = &rte_mempool_ops_table.ops[ops_index];
> > >         strlcpy(ops->name, h->name, sizeof(ops->name));
> > >         ops->alloc = h->alloc;
> > > @@ -63,6 +75,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > >         ops->get_info = h->get_info;
> > >         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
> > >
> > > +       rte_mempool_ops_table.num_ops++;
> > > +
> > >         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> > >
> > >         return ops_index;
> > > --
> > > 1.8.3.1
> > >
>
>
>
> --
> Thanks,
> Tonghao
Tonghao Zhang March 4, 2020, 2:46 p.m. UTC | #4 
On Wed, Mar 4, 2020 at 9:33 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Mar 4, 2020 at 6:48 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Mar 2, 2020 at 9:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 7:27 AM <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>
> > > > ---
> > > >  lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
> > > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> > > > index 22c5251..06dfe16 100644
> > > > --- a/lib/librte_mempool/rte_mempool_ops.c
> > > > +++ b/lib/librte_mempool/rte_mempool_ops.c
> > > > @@ -22,7 +22,7 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > >  rte_mempool_register_ops(const struct rte_mempool_ops *h)
> > > >  {
> > > >         struct rte_mempool_ops *ops;
> > > > -       int16_t ops_index;
> > > > +       unsigned ops_index, i;
> > > >
> > > >         rte_spinlock_lock(&rte_mempool_ops_table.sl);
> > > >
> > > > @@ -50,7 +50,19 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > >                 return -EEXIST;
> > > >         }
> > > >
> > > > -       ops_index = rte_mempool_ops_table.num_ops++;
> > > > +       /* sort the rte_mempool_ops by name. the order of the mempool
> > > > +        * lib initiation will not affect rte_mempool_ops index. */
> > >
> > > +1 for the fix.
> > > For the implementation, why not use qsort_r() for sorting?
> > The implementation is easy, and the number of mempool driver is not too large.
> > But we can use the qsort_r to implement it.
>
> Since it is in a slow path, IMO, better to use standard sort functions
> for better readability.
Agree, can you help me review the patch:

diff --git a/lib/librte_mempool/rte_mempool_ops.c
b/lib/librte_mempool/rte_mempool_ops.c
index 22c5251..1acee58 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,9 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
        ops->get_info = h->get_info;
        ops->dequeue_contig_blocks = h->dequeue_contig_blocks;

+       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;


>
> > >
> > > > +       ops_index = rte_mempool_ops_table.num_ops;
> > > > +       for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> > > > +               if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
> > > > +                       do {
> > > > +                               rte_mempool_ops_table.ops[ops_index] =
> > > > +                                       rte_mempool_ops_table.ops[ops_index -1];
> > > > +                       } while (--ops_index > i);
> > > > +                       break;
> > > > +               }
> > > > +       }
> > > > +
> > > >         ops = &rte_mempool_ops_table.ops[ops_index];
> > > >         strlcpy(ops->name, h->name, sizeof(ops->name));
> > > >         ops->alloc = h->alloc;
> > > > @@ -63,6 +75,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > >         ops->get_info = h->get_info;
> > > >         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
> > > >
> > > > +       rte_mempool_ops_table.num_ops++;
> > > > +
> > > >         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> > > >
> > > >         return ops_index;
> > > > --
> > > > 1.8.3.1
> > > >
> >
> >
> >
> > --
> > Thanks,
> > Tonghao



--
Thanks,
Tonghao
Jerin Jacob March 4, 2020, 3:14 p.m. UTC | #5 
On Wed, Mar 4, 2020 at 8:17 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Wed, Mar 4, 2020 at 9:33 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Wed, Mar 4, 2020 at 6:48 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 9:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 2, 2020 at 7:27 AM <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>
> > > > > ---
> > > > >  lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
> > > > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> > > > > index 22c5251..06dfe16 100644
> > > > > --- a/lib/librte_mempool/rte_mempool_ops.c
> > > > > +++ b/lib/librte_mempool/rte_mempool_ops.c
> > > > > @@ -22,7 +22,7 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > > >  rte_mempool_register_ops(const struct rte_mempool_ops *h)
> > > > >  {
> > > > >         struct rte_mempool_ops *ops;
> > > > > -       int16_t ops_index;
> > > > > +       unsigned ops_index, i;
> > > > >
> > > > >         rte_spinlock_lock(&rte_mempool_ops_table.sl);
> > > > >
> > > > > @@ -50,7 +50,19 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > > >                 return -EEXIST;
> > > > >         }
> > > > >
> > > > > -       ops_index = rte_mempool_ops_table.num_ops++;
> > > > > +       /* sort the rte_mempool_ops by name. the order of the mempool
> > > > > +        * lib initiation will not affect rte_mempool_ops index. */
> > > >
> > > > +1 for the fix.
> > > > For the implementation, why not use qsort_r() for sorting?
> > > The implementation is easy, and the number of mempool driver is not too large.
> > > But we can use the qsort_r to implement it.
> >
> > Since it is in a slow path, IMO, better to use standard sort functions
> > for better readability.
> Agree, can you help me review the patch:
>
> diff --git a/lib/librte_mempool/rte_mempool_ops.c
> b/lib/librte_mempool/rte_mempool_ops.c
> index 22c5251..1acee58 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,9 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
>         ops->get_info = h->get_info;
>         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
>
> +       qsort(rte_mempool_ops_table.ops, rte_mempool_ops_table.num_ops,
> +             sizeof(rte_mempool_ops_table.ops[0]), compare_mempool_ops);

Looks good.

Not tested. Please check qsort behavior when
rte_mempool_ops_table.num_ops == 0 case.



> +
>         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>
>         return ops_index;
>
>
> >
> > > >
> > > > > +       ops_index = rte_mempool_ops_table.num_ops;
> > > > > +       for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> > > > > +               if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
> > > > > +                       do {
> > > > > +                               rte_mempool_ops_table.ops[ops_index] =
> > > > > +                                       rte_mempool_ops_table.ops[ops_index -1];
> > > > > +                       } while (--ops_index > i);
> > > > > +                       break;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > >         ops = &rte_mempool_ops_table.ops[ops_index];
> > > > >         strlcpy(ops->name, h->name, sizeof(ops->name));
> > > > >         ops->alloc = h->alloc;
> > > > > @@ -63,6 +75,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > > >         ops->get_info = h->get_info;
> > > > >         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
> > > > >
> > > > > +       rte_mempool_ops_table.num_ops++;
> > > > > +
> > > > >         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> > > > >
> > > > >         return ops_index;
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > >
> > >
> > >
> > > --
> > > Thanks,
> > > Tonghao
>
>
>
> --
> Thanks,
> Tonghao
Tonghao Zhang March 4, 2020, 3:25 p.m. UTC | #6 
On Wed, Mar 4, 2020 at 11:14 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Mar 4, 2020 at 8:17 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Wed, Mar 4, 2020 at 9:33 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > On Wed, Mar 4, 2020 at 6:48 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 2, 2020 at 9:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 2, 2020 at 7:27 AM <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>
> > > > > > ---
> > > > > >  lib/librte_mempool/rte_mempool_ops.c | 18 ++++++++++++++++--
> > > > > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> > > > > > index 22c5251..06dfe16 100644
> > > > > > --- a/lib/librte_mempool/rte_mempool_ops.c
> > > > > > +++ b/lib/librte_mempool/rte_mempool_ops.c
> > > > > > @@ -22,7 +22,7 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > > > >  rte_mempool_register_ops(const struct rte_mempool_ops *h)
> > > > > >  {
> > > > > >         struct rte_mempool_ops *ops;
> > > > > > -       int16_t ops_index;
> > > > > > +       unsigned ops_index, i;
> > > > > >
> > > > > >         rte_spinlock_lock(&rte_mempool_ops_table.sl);
> > > > > >
> > > > > > @@ -50,7 +50,19 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > > > >                 return -EEXIST;
> > > > > >         }
> > > > > >
> > > > > > -       ops_index = rte_mempool_ops_table.num_ops++;
> > > > > > +       /* sort the rte_mempool_ops by name. the order of the mempool
> > > > > > +        * lib initiation will not affect rte_mempool_ops index. */
> > > > >
> > > > > +1 for the fix.
> > > > > For the implementation, why not use qsort_r() for sorting?
> > > > The implementation is easy, and the number of mempool driver is not too large.
> > > > But we can use the qsort_r to implement it.
> > >
> > > Since it is in a slow path, IMO, better to use standard sort functions
> > > for better readability.
> > Agree, can you help me review the patch:
> >
> > diff --git a/lib/librte_mempool/rte_mempool_ops.c
> > b/lib/librte_mempool/rte_mempool_ops.c
> > index 22c5251..1acee58 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,9 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> >         ops->get_info = h->get_info;
> >         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
> >
> > +       qsort(rte_mempool_ops_table.ops, rte_mempool_ops_table.num_ops,
> > +             sizeof(rte_mempool_ops_table.ops[0]), compare_mempool_ops);
>
> Looks good.
>
> Not tested. Please check qsort behavior when
> rte_mempool_ops_table.num_ops == 0 case.
If nmemb (qsort arg) == 0, qsort will do nothing.
>
>
> > +
> >         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> >
> >         return ops_index;
> >
> >
> > >
> > > > >
> > > > > > +       ops_index = rte_mempool_ops_table.num_ops;
> > > > > > +       for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> > > > > > +               if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
> > > > > > +                       do {
> > > > > > +                               rte_mempool_ops_table.ops[ops_index] =
> > > > > > +                                       rte_mempool_ops_table.ops[ops_index -1];
> > > > > > +                       } while (--ops_index > i);
> > > > > > +                       break;
> > > > > > +               }
> > > > > > +       }
> > > > > > +
> > > > > >         ops = &rte_mempool_ops_table.ops[ops_index];
> > > > > >         strlcpy(ops->name, h->name, sizeof(ops->name));
> > > > > >         ops->alloc = h->alloc;
> > > > > > @@ -63,6 +75,8 @@ struct rte_mempool_ops_table rte_mempool_ops_table = {
> > > > > >         ops->get_info = h->get_info;
> > > > > >         ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
> > > > > >
> > > > > > +       rte_mempool_ops_table.num_ops++;
> > > > > > +
> > > > > >         rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> > > > > >
> > > > > >         return ops_index;
> > > > > > --
> > > > > > 1.8.3.1
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > > Tonghao
> >
> >
> >
> > --
> > Thanks,
> > Tonghao
diff mbox series

Patch

diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index 22c5251..06dfe16 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -22,7 +22,7 @@  struct rte_mempool_ops_table rte_mempool_ops_table = {
 rte_mempool_register_ops(const struct rte_mempool_ops *h)
 {
 	struct rte_mempool_ops *ops;
-	int16_t ops_index;
+	unsigned ops_index, i;
 
 	rte_spinlock_lock(&rte_mempool_ops_table.sl);
 
@@ -50,7 +50,19 @@  struct rte_mempool_ops_table rte_mempool_ops_table = {
 		return -EEXIST;
 	}
 
-	ops_index = rte_mempool_ops_table.num_ops++;
+	/* sort the rte_mempool_ops by name. the order of the mempool
+	 * lib initiation will not affect rte_mempool_ops index. */
+	ops_index = rte_mempool_ops_table.num_ops;
+	for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
+		if (strcmp(h->name, rte_mempool_ops_table.ops[i].name) < 0) {
+			do {
+				rte_mempool_ops_table.ops[ops_index] =
+					rte_mempool_ops_table.ops[ops_index -1];
+			} while (--ops_index > i);
+			break;
+		}
+	}
+
 	ops = &rte_mempool_ops_table.ops[ops_index];
 	strlcpy(ops->name, h->name, sizeof(ops->name));
 	ops->alloc = h->alloc;
@@ -63,6 +75,8 @@  struct rte_mempool_ops_table rte_mempool_ops_table = {
 	ops->get_info = h->get_info;
 	ops->dequeue_contig_blocks = h->dequeue_contig_blocks;
 
+	rte_mempool_ops_table.num_ops++;
+
 	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
 
 	return ops_index;