[v1,2/2] test/ring: fix wrong param passed to the enqueue APIs
diff mbox series

Message ID 20200729063105.11299-3-feifei.wang2@arm.com
State Superseded
Delegated to: David Marchand
Headers show
Series
  • wrong pointer passed of ring
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/checkpatch success coding style OK

Commit Message

Feifei Wang July 29, 2020, 6:31 a.m. UTC
When enqueue one element (object of type void*) to ring in the
performance test, a pointer (the object to be enqueued) should be
passed to rte_ring_[sp|mp]enqueue APIs, not the pointer to a table
of void *pointers (objects).

Fixes: a9fe152363e2 ("test/ring: add custom element size functional tests")
Cc: honnappa.nagarahalli@arm.com
Cc: stable@dpdk.org

Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test/test_ring.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

David Marchand July 29, 2020, 1:48 p.m. UTC | #1
Hello Feifei,

On Wed, Jul 29, 2020 at 8:32 AM Feifei Wang <feifei.wang2@arm.com> wrote:
>
> When enqueue one element (object of type void*) to ring in the
> performance test, a pointer (the object to be enqueued) should be
> passed to rte_ring_[sp|mp]enqueue APIs, not the pointer to a table
> of void *pointers (objects).

Good catch.
Are we missing a check in the UT so that dequeued object is what had
been enqueued?


>
> Fixes: a9fe152363e2 ("test/ring: add custom element size functional tests")
> Cc: honnappa.nagarahalli@arm.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  app/test/test_ring.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/app/test/test_ring.h b/app/test/test_ring.h
> index aa6ae67ca..d4b15af7c 100644
> --- a/app/test/test_ring.h
> +++ b/app/test/test_ring.h
> @@ -50,11 +50,11 @@ test_ring_enqueue(struct rte_ring *r, void **obj, int esize, unsigned int n,
>         if ((esize) == -1)
>                 switch (api_type) {
>                 case (TEST_RING_THREAD_DEF | TEST_RING_ELEM_SINGLE):
> -                       return rte_ring_enqueue(r, obj);
> +                       return rte_ring_enqueue(r, *obj);
>                 case (TEST_RING_THREAD_SPSC | TEST_RING_ELEM_SINGLE):
> -                       return rte_ring_sp_enqueue(r, obj);
> +                       return rte_ring_sp_enqueue(r, *obj);
>                 case (TEST_RING_THREAD_MPMC | TEST_RING_ELEM_SINGLE):
> -                       return rte_ring_mp_enqueue(r, obj);
> +                       return rte_ring_mp_enqueue(r, *obj);
>                 case (TEST_RING_THREAD_DEF | TEST_RING_ELEM_BULK):
>                         return rte_ring_enqueue_bulk(r, obj, n, NULL);
>                 case (TEST_RING_THREAD_SPSC | TEST_RING_ELEM_BULK):
> --
> 2.17.1
>
Feifei Wang July 29, 2020, 2:16 p.m. UTC | #2
Hi, David

> -----邮件原件-----
> 发件人: David Marchand <david.marchand@redhat.com>
> 发送时间: 2020年7月29日 21:48
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> 抄送: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Konstantin
> Ananyev <konstantin.ananyev@intel.com>; Gavin Hu <Gavin.Hu@arm.com>;
> Olivier Matz <olivier.matz@6wind.com>; dev <dev@dpdk.org>; nd
> <nd@arm.com>; dpdk stable <stable@dpdk.org>
> 主题: Re: [dpdk-dev] [PATCH v1 2/2] test/ring: fix wrong param passed to the
> enqueue APIs
> 
> Hello Feifei,
> 
> On Wed, Jul 29, 2020 at 8:32 AM Feifei Wang <feifei.wang2@arm.com>
> wrote:
> >
> > When enqueue one element (object of type void*) to ring in the
> > performance test, a pointer (the object to be enqueued) should be
> > passed to rte_ring_[sp|mp]enqueue APIs, not the pointer to a table of
> > void *pointers (objects).
> 
> Good catch.
Thanks very much.
> Are we missing a check in the UT so that dequeued object is what had been
> enqueued?
> 
>  
Dequeue is not necessary to change because the param defined in rte_ring_dequeue
is different from that in rte_ring_enqueue:
rte_ring_enqueue(struct rte_ring *r, void *obj): obj is a pointer (object) to be added in the ring
rte_ring_dequeue(struct rte_ring *r, void **obj_p): obj_p is a pointer to a void * pointer
(object) that will be filled.
> >
> > Fixes: a9fe152363e2 ("test/ring: add custom element size functional
> > tests")
> > Cc: honnappa.nagarahalli@arm.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  app/test/test_ring.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test/test_ring.h b/app/test/test_ring.h index
> > aa6ae67ca..d4b15af7c 100644
> > --- a/app/test/test_ring.h
> > +++ b/app/test/test_ring.h
> > @@ -50,11 +50,11 @@ test_ring_enqueue(struct rte_ring *r, void **obj,
> int esize, unsigned int n,
> >         if ((esize) == -1)
> >                 switch (api_type) {
> >                 case (TEST_RING_THREAD_DEF | TEST_RING_ELEM_SINGLE):
> > -                       return rte_ring_enqueue(r, obj);
> > +                       return rte_ring_enqueue(r, *obj);
> >                 case (TEST_RING_THREAD_SPSC | TEST_RING_ELEM_SINGLE):
> > -                       return rte_ring_sp_enqueue(r, obj);
> > +                       return rte_ring_sp_enqueue(r, *obj);
> >                 case (TEST_RING_THREAD_MPMC | TEST_RING_ELEM_SINGLE):
> > -                       return rte_ring_mp_enqueue(r, obj);
> > +                       return rte_ring_mp_enqueue(r, *obj);
> >                 case (TEST_RING_THREAD_DEF | TEST_RING_ELEM_BULK):
> >                         return rte_ring_enqueue_bulk(r, obj, n, NULL);
> >                 case (TEST_RING_THREAD_SPSC | TEST_RING_ELEM_BULK):
> > --
> > 2.17.1
> >
> 
> 
> --
> David Marchand
David Marchand July 29, 2020, 2:21 p.m. UTC | #3
On Wed, Jul 29, 2020 at 4:16 PM Feifei Wang <Feifei.Wang2@arm.com> wrote:
> > Are we missing a check in the UT so that dequeued object is what had been
> > enqueued?
> >
> >
> Dequeue is not necessary to change because the param defined in rte_ring_dequeue
> is different from that in rte_ring_enqueue:
> rte_ring_enqueue(struct rte_ring *r, void *obj): obj is a pointer (object) to be added in the ring
> rte_ring_dequeue(struct rte_ring *r, void **obj_p): obj_p is a pointer to a void * pointer
> (object) that will be filled.

That I get it.

What I meant is that the test enqueues an object in a ring until it is
full [1], then dequeues all the ring [2].
1: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n814
2: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n825

If the test had checked that dequeued objects are the right one, we
would have caught it.

But on the other hand, maybe another part of the functionnal ring
tests already check this and we only need to fix this issue here.
Feifei Wang July 29, 2020, 3:03 p.m. UTC | #4
> -----邮件原件-----
> 发件人: David Marchand <david.marchand@redhat.com>
> 发送时间: 2020年7月29日 22:21
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> 抄送: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Konstantin
> Ananyev <konstantin.ananyev@intel.com>; Gavin Hu <Gavin.Hu@arm.com>;
> Olivier Matz <olivier.matz@6wind.com>; dev <dev@dpdk.org>; nd
> <nd@arm.com>; dpdk stable <stable@dpdk.org>
> 主题: Re: [dpdk-dev] [PATCH v1 2/2] test/ring: fix wrong param passed to the
> enqueue APIs
> 
> On Wed, Jul 29, 2020 at 4:16 PM Feifei Wang <Feifei.Wang2@arm.com>
> wrote:
> > > Are we missing a check in the UT so that dequeued object is what had
> > > been enqueued?
> > >
> > >
> > Dequeue is not necessary to change because the param defined in
> > rte_ring_dequeue is different from that in rte_ring_enqueue:
> > rte_ring_enqueue(struct rte_ring *r, void *obj): obj is a pointer
> > (object) to be added in the ring rte_ring_dequeue(struct rte_ring *r,
> > void **obj_p): obj_p is a pointer to a void * pointer
> > (object) that will be filled.
> 
> That I get it.
> 
> What I meant is that the test enqueues an object in a ring until it is full [1],
> then dequeues all the ring [2].
> 1: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n814
> 2: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n825
> 
> If the test had checked that dequeued objects are the right one, we would
> have caught it.
> 
> But on the other hand, maybe another part of the functionnal ring tests
> already check this and we only need to fix this issue here.

Sorry I just misunderstood you.
1. Actually, for the APIs of test_ring.h, we lack a test to check whether the
value of object enqueued into the ring matches that dequeued from the ring.
But it is mainly used to measure the length of time from enqueue to dequeue.
So I'm not sure it is necessary.
2. For the APIs of rte_ring.h, some tests can be used to test whether the
value of object enqueued into the ring matches that dequeued from the ring.
For example:
$table_autotest
$mbuf_autotest
> 
> 
> --
> David Marchand

--
Feifei
Honnappa Nagarahalli July 29, 2020, 9:24 p.m. UTC | #5
<snip>

> >
> > On Wed, Jul 29, 2020 at 4:16 PM Feifei Wang <Feifei.Wang2@arm.com>
> > wrote:
> > > > Are we missing a check in the UT so that dequeued object is what
> > > > had been enqueued?
Yes, missing for single element enqueue/dequeue

> > > >
> > > >
> > > Dequeue is not necessary to change because the param defined in
> > > rte_ring_dequeue is different from that in rte_ring_enqueue:
> > > rte_ring_enqueue(struct rte_ring *r, void *obj): obj is a pointer
> > > (object) to be added in the ring rte_ring_dequeue(struct rte_ring
> > > *r, void **obj_p): obj_p is a pointer to a void * pointer
> > > (object) that will be filled.
> >
> > That I get it.
> >
> > What I meant is that the test enqueues an object in a ring until it is
> > full [1], then dequeues all the ring [2].
> > 1: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n814
> > 2: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n825
> >
> > If the test had checked that dequeued objects are the right one, we
> > would have caught it.
> >
> > But on the other hand, maybe another part of the functionnal ring
> > tests already check this and we only need to fix this issue here.
> 
> Sorry I just misunderstood you.
> 1. Actually, for the APIs of test_ring.h, we lack a test to check whether the
> value of object enqueued into the ring matches that dequeued from the ring.
> But it is mainly used to measure the length of time from enqueue to dequeue.
> So I'm not sure it is necessary.
> 2. For the APIs of rte_ring.h, some tests can be used to test whether the value
> of object enqueued into the ring matches that dequeued from the ring.
> For example:
> $table_autotest
> $mbuf_autotest
The dequeued objects are checked against the enqueued objects for the bulk and burst APIs. Look at tests test_ring_burst_bulk_tests1..4. 'memcmp' is used to compare the enqueued objects against the dequeued ones.
Similar comparison can be added in test_ring_basic_ex, test_ring_with_exact_size functions.

> >
> >
> > --
> > David Marchand
> 
> --
> Feifei
>
Feifei Wang July 30, 2020, 10:28 a.m. UTC | #6
> -----邮件原件-----
> 发件人: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> 发送时间: 2020年7月30日 5:24
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; David Marchand
> <david.marchand@redhat.com>
> 抄送: Konstantin Ananyev <konstantin.ananyev@intel.com>; Gavin Hu
> <Gavin.Hu@arm.com>; Olivier Matz <olivier.matz@6wind.com>; dev
> <dev@dpdk.org>; nd <nd@arm.com>; dpdk stable <stable@dpdk.org>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> 主题: RE: [dpdk-dev] [PATCH v1 2/2] test/ring: fix wrong param passed to the
> enqueue APIs
> 
> <snip>
> 
> > >
> > > On Wed, Jul 29, 2020 at 4:16 PM Feifei Wang <Feifei.Wang2@arm.com>
> > > wrote:
> > > > > Are we missing a check in the UT so that dequeued object is what
> > > > > had been enqueued?
> Yes, missing for single element enqueue/dequeue
> 
> > > > >
> > > > >
> > > > Dequeue is not necessary to change because the param defined in
> > > > rte_ring_dequeue is different from that in rte_ring_enqueue:
> > > > rte_ring_enqueue(struct rte_ring *r, void *obj): obj is a pointer
> > > > (object) to be added in the ring rte_ring_dequeue(struct rte_ring
> > > > *r, void **obj_p): obj_p is a pointer to a void * pointer
> > > > (object) that will be filled.
> > >
> > > That I get it.
> > >
> > > What I meant is that the test enqueues an object in a ring until it
> > > is full [1], then dequeues all the ring [2].
> > > 1: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n814
> > > 2: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n825
> > >
> > > If the test had checked that dequeued objects are the right one, we
> > > would have caught it.
> > >
> > > But on the other hand, maybe another part of the functionnal ring
> > > tests already check this and we only need to fix this issue here.
> >
> > Sorry I just misunderstood you.
> > 1. Actually, for the APIs of test_ring.h, we lack a test to check
> > whether the value of object enqueued into the ring matches that dequeued
> from the ring.
> > But it is mainly used to measure the length of time from enqueue to
> dequeue.
> > So I'm not sure it is necessary.
> > 2. For the APIs of rte_ring.h, some tests can be used to test whether
> > the value of object enqueued into the ring matches that dequeued from the
> ring.
> > For example:
> > $table_autotest
> > $mbuf_autotest
> The dequeued objects are checked against the enqueued objects for the bulk
> and burst APIs. Look at tests test_ring_burst_bulk_tests1..4. 'memcmp' is used
> to compare the enqueued objects against the dequeued ones.
> Similar comparison can be added in test_ring_basic_ex,
> test_ring_with_exact_size functions.
Thank you for bringing that up.
> 
> > >
> > >
> > > --
> > > David Marchand
> >
> > --
> > Feifei
> >
Feifei Wang July 31, 2020, 6:25 a.m. UTC | #7
> -----邮件原件-----
> 发件人: Feifei Wang
> 发送时间: 2020年7月30日 18:29
> 收件人: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; David
> Marchand <david.marchand@redhat.com>
> 抄送: Konstantin Ananyev <konstantin.ananyev@intel.com>; Gavin Hu
> <Gavin.Hu@arm.com>; Olivier Matz <olivier.matz@6wind.com>; dev
> <dev@dpdk.org>; nd <nd@arm.com>; dpdk stable <stable@dpdk.org>; nd
> <nd@arm.com>; nd <nd@arm.com>
> 主题: 回复: [dpdk-dev] [PATCH v1 2/2] test/ring: fix wrong param passed to
> the enqueue APIs
> 
> 
> 
> > -----邮件原件-----
> > 发件人: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > 发送时间: 2020年7月30日 5:24
> > 收件人: Feifei Wang <Feifei.Wang2@arm.com>; David Marchand
> > <david.marchand@redhat.com>
> > 抄送: Konstantin Ananyev <konstantin.ananyev@intel.com>; Gavin Hu
> > <Gavin.Hu@arm.com>; Olivier Matz <olivier.matz@6wind.com>; dev
> > <dev@dpdk.org>; nd <nd@arm.com>; dpdk stable <stable@dpdk.org>;
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd
> <nd@arm.com>
> > 主题: RE: [dpdk-dev] [PATCH v1 2/2] test/ring: fix wrong param passed to
> > the enqueue APIs
> >
> > <snip>
> >
> > > >
> > > > On Wed, Jul 29, 2020 at 4:16 PM Feifei Wang <Feifei.Wang2@arm.com>
> > > > wrote:
> > > > > > Are we missing a check in the UT so that dequeued object is
> > > > > > what had been enqueued?
> > Yes, missing for single element enqueue/dequeue
> >
> > > > > >
> > > > > >
> > > > > Dequeue is not necessary to change because the param defined in
> > > > > rte_ring_dequeue is different from that in rte_ring_enqueue:
> > > > > rte_ring_enqueue(struct rte_ring *r, void *obj): obj is a
> > > > > pointer
> > > > > (object) to be added in the ring rte_ring_dequeue(struct
> > > > > rte_ring *r, void **obj_p): obj_p is a pointer to a void *
> > > > > pointer
> > > > > (object) that will be filled.
> > > >
> > > > That I get it.
> > > >
> > > > What I meant is that the test enqueues an object in a ring until
> > > > it is full [1], then dequeues all the ring [2].
> > > > 1: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n814
> > > > 2: https://git.dpdk.org/dpdk/tree/app/test/test_ring.c#n825
> > > >
> > > > If the test had checked that dequeued objects are the right one,
> > > > we would have caught it.
> > > >
> > > > But on the other hand, maybe another part of the functionnal ring
> > > > tests already check this and we only need to fix this issue here.
> > >
> > > Sorry I just misunderstood you.
> > > 1. Actually, for the APIs of test_ring.h, we lack a test to check
> > > whether the value of object enqueued into the ring matches that
> > > dequeued
> > from the ring.
> > > But it is mainly used to measure the length of time from enqueue to
> > dequeue.
> > > So I'm not sure it is necessary.
> > > 2. For the APIs of rte_ring.h, some tests can be used to test
> > > whether the value of object enqueued into the ring matches that
> > > dequeued from the
> > ring.
> > > For example:
> > > $table_autotest
> > > $mbuf_autotest
> > The dequeued objects are checked against the enqueued objects for the
> > bulk and burst APIs. Look at tests test_ring_burst_bulk_tests1..4.
> > 'memcmp' is used to compare the enqueued objects against the dequeued
> ones.
> > Similar comparison can be added in test_ring_basic_ex,
> > test_ring_with_exact_size functions.
> Thank you for bringing that up.
Next, I will adding the test to check the value of dequeue against enqueue in
test_ring_basic_ex,  test_ring_with_exact_size functions. Furthermore, I will pack
it and the current patch together and upload the new version to the community. 
> >
> > > >
> > > >
> > > > --
> > > > David Marchand
> > >
> > > --
> > > Feifei
> > >

Patch
diff mbox series

diff --git a/app/test/test_ring.h b/app/test/test_ring.h
index aa6ae67ca..d4b15af7c 100644
--- a/app/test/test_ring.h
+++ b/app/test/test_ring.h
@@ -50,11 +50,11 @@  test_ring_enqueue(struct rte_ring *r, void **obj, int esize, unsigned int n,
 	if ((esize) == -1)
 		switch (api_type) {
 		case (TEST_RING_THREAD_DEF | TEST_RING_ELEM_SINGLE):
-			return rte_ring_enqueue(r, obj);
+			return rte_ring_enqueue(r, *obj);
 		case (TEST_RING_THREAD_SPSC | TEST_RING_ELEM_SINGLE):
-			return rte_ring_sp_enqueue(r, obj);
+			return rte_ring_sp_enqueue(r, *obj);
 		case (TEST_RING_THREAD_MPMC | TEST_RING_ELEM_SINGLE):
-			return rte_ring_mp_enqueue(r, obj);
+			return rte_ring_mp_enqueue(r, *obj);
 		case (TEST_RING_THREAD_DEF | TEST_RING_ELEM_BULK):
 			return rte_ring_enqueue_bulk(r, obj, n, NULL);
 		case (TEST_RING_THREAD_SPSC | TEST_RING_ELEM_BULK):