* [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue
@ 2017-10-10 9:56 Jia He
2017-10-12 15:53 ` Olivier MATZ
0 siblings, 1 reply; 41+ messages in thread
From: Jia He @ 2017-10-10 9:56 UTC (permalink / raw)
To: olivier.matz; +Cc: dev, Jia He, jia.he, jie2.liu, bing.zhao
Before this patch:
In __rte_ring_move_cons_head()
...
do {
/* Restore n as it may change every loop */
n = max;
*old_head = r->cons.head; //1st load
const uint32_t prod_tail = r->prod.tail; //2nd load
In weak memory order architectures(powerpc,arm), the 2nd load might be
reodered before the 1st load, that makes *entries is bigger than we wanted.
This nasty reording messed enque/deque up.
cpu1(producer) cpu2(consumer) cpu3(consumer)
load r->prod.tail
in enqueue:
load r->cons.tail
load r->prod.head
store r->prod.tail
load r->cons.head
load r->prod.tail
...
store r->cons.{head,tail}
load r->cons.head
THEN,r->cons.head will be bigger than prod_tail, then make *entries very big
After this patch, the old cons.head will be recaculated after failure of
rte_atomic32_cmpset
There is no such issue in X86 cpu, because X86 is strong memory order model
Signed-off-by: Jia He <hejianet@gmail.com>
Signed-off-by: jia.he@hxt-semitech.com
Signed-off-by: jie2.liu@hxt-semitech.com
Signed-off-by: bing.zhao@hxt-semitech.com
---
lib/librte_ring/rte_ring.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 5e9b3b7..15c72e2 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
n = max;
*old_head = r->prod.head;
+
+ /* load of prod.tail can't be reordered before cons.head */
+ rte_smp_rmb();
+
const uint32_t cons_tail = r->cons.tail;
/*
* The subtraction is done between two unsigned 32bits value
@@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
n = max;
*old_head = r->cons.head;
+
+ /* load of prod.tail can't be reordered before cons.head */
+ rte_smp_rmb();
+
const uint32_t prod_tail = r->prod.tail;
/* The subtraction is done between two unsigned 32bits value
* (the result is always modulo 32 bits even if we have
--
2.7.4
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-10 9:56 [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue Jia He @ 2017-10-12 15:53 ` Olivier MATZ 2017-10-12 16:15 ` Stephen Hemminger 2017-10-12 17:05 ` Ananyev, Konstantin 0 siblings, 2 replies; 41+ messages in thread From: Olivier MATZ @ 2017-10-12 15:53 UTC (permalink / raw) To: Jia He; +Cc: dev, jia.he, jie2.liu, bing.zhao, konstantin.ananyev, jerin.jacob Hi, On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > Before this patch: > In __rte_ring_move_cons_head() > ... > do { > /* Restore n as it may change every loop */ > n = max; > > *old_head = r->cons.head; //1st load > const uint32_t prod_tail = r->prod.tail; //2nd load > > In weak memory order architectures(powerpc,arm), the 2nd load might be > reodered before the 1st load, that makes *entries is bigger than we wanted. > This nasty reording messed enque/deque up. > > cpu1(producer) cpu2(consumer) cpu3(consumer) > load r->prod.tail > in enqueue: > load r->cons.tail > load r->prod.head > > store r->prod.tail > > load r->cons.head > load r->prod.tail > ... > store r->cons.{head,tail} > load r->cons.head > > THEN,r->cons.head will be bigger than prod_tail, then make *entries very big > > After this patch, the old cons.head will be recaculated after failure of > rte_atomic32_cmpset > > There is no such issue in X86 cpu, because X86 is strong memory order model > > Signed-off-by: Jia He <hejianet@gmail.com> > Signed-off-by: jia.he@hxt-semitech.com > Signed-off-by: jie2.liu@hxt-semitech.com > Signed-off-by: bing.zhao@hxt-semitech.com > > --- > lib/librte_ring/rte_ring.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > index 5e9b3b7..15c72e2 100644 > --- a/lib/librte_ring/rte_ring.h > +++ b/lib/librte_ring/rte_ring.h > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > n = max; > > *old_head = r->prod.head; > + > + /* load of prod.tail can't be reordered before cons.head */ > + rte_smp_rmb(); > + > const uint32_t cons_tail = r->cons.tail; > /* > * The subtraction is done between two unsigned 32bits value > @@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > n = max; > > *old_head = r->cons.head; > + > + /* load of prod.tail can't be reordered before cons.head */ > + rte_smp_rmb(); > + > const uint32_t prod_tail = r->prod.tail; > /* The subtraction is done between two unsigned 32bits value > * (the result is always modulo 32 bits even if we have > -- > 2.7.4 > The explanation convinces me. However, since it's in a critical path, it would be good to have other opinions. This patch reminds me this discussion, that was also related to memory barrier, but at another place: http://dpdk.org/ml/archives/dev/2016-July/043765.html Lead to that patch: http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e But finally reverted: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 Konstatin, Jerin, do you have any comment? ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-12 15:53 ` Olivier MATZ @ 2017-10-12 16:15 ` Stephen Hemminger 2017-10-12 17:05 ` Ananyev, Konstantin 1 sibling, 0 replies; 41+ messages in thread From: Stephen Hemminger @ 2017-10-12 16:15 UTC (permalink / raw) To: Olivier MATZ Cc: Jia He, dev, jia.he, jie2.liu, bing.zhao, konstantin.ananyev, jerin.jacob On Thu, 12 Oct 2017 17:53:51 +0200 Olivier MATZ <olivier.matz@6wind.com> wrote: > Hi, > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > Before this patch: > > In __rte_ring_move_cons_head() > > ... > > do { > > /* Restore n as it may change every loop */ > > n = max; > > > > *old_head = r->cons.head; //1st load > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > In weak memory order architectures(powerpc,arm), the 2nd load might be > > reodered before the 1st load, that makes *entries is bigger than we wanted. > > This nasty reording messed enque/deque up. > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > load r->prod.tail > > in enqueue: > > load r->cons.tail > > load r->prod.head > > > > store r->prod.tail > > > > load r->cons.head > > load r->prod.tail > > ... > > store r->cons.{head,tail} > > load r->cons.head > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries very big > > > > After this patch, the old cons.head will be recaculated after failure of > > rte_atomic32_cmpset > > > > There is no such issue in X86 cpu, because X86 is strong memory order model > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > Signed-off-by: jia.he@hxt-semitech.com > > Signed-off-by: jie2.liu@hxt-semitech.com > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > --- > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index 5e9b3b7..15c72e2 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > > n = max; > > > > *old_head = r->prod.head; > > + > > + /* load of prod.tail can't be reordered before cons.head */ > > + rte_smp_rmb(); > > + > > const uint32_t cons_tail = r->cons.tail; > > /* > > * The subtraction is done between two unsigned 32bits value > > @@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > > n = max; > > > > *old_head = r->cons.head; > > + > > + /* load of prod.tail can't be reordered before cons.head */ > > + rte_smp_rmb(); > > + > > const uint32_t prod_tail = r->prod.tail; > > /* The subtraction is done between two unsigned 32bits value > > * (the result is always modulo 32 bits even if we have > > -- > > 2.7.4 > > > > The explanation convinces me. > > However, since it's in a critical path, it would be good to have other > opinions. This patch reminds me this discussion, that was also related to > memory barrier, but at another place: > http://dpdk.org/ml/archives/dev/2016-July/043765.html > Lead to that patch: http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > But finally reverted: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > Konstatin, Jerin, do you have any comment? On strongly ordered architectures like x86 rte_smp_rmb() turns into compiler_barrier(). Compiler barriers generate no instructions, they just tell the compiler to not do data flow optimization across. Since the pointers are mostly volatile it shouldn't even change the generated code. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-12 15:53 ` Olivier MATZ 2017-10-12 16:15 ` Stephen Hemminger @ 2017-10-12 17:05 ` Ananyev, Konstantin 2017-10-12 17:23 ` Jerin Jacob 2017-10-13 0:24 ` Liu, Jie2 1 sibling, 2 replies; 41+ messages in thread From: Ananyev, Konstantin @ 2017-10-12 17:05 UTC (permalink / raw) To: Olivier MATZ, Jia He; +Cc: dev, jia.he, jie2.liu, bing.zhao, jerin.jacob Hi guys, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Thursday, October 12, 2017 4:54 PM > To: Jia He <hejianet@gmail.com> > Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt-semitech.com; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue > > Hi, > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > Before this patch: > > In __rte_ring_move_cons_head() > > ... > > do { > > /* Restore n as it may change every loop */ > > n = max; > > > > *old_head = r->cons.head; //1st load > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > In weak memory order architectures(powerpc,arm), the 2nd load might be > > reodered before the 1st load, that makes *entries is bigger than we wanted. > > This nasty reording messed enque/deque up. > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > load r->prod.tail > > in enqueue: > > load r->cons.tail > > load r->prod.head > > > > store r->prod.tail > > > > load r->cons.head > > load r->prod.tail > > ... > > store r->cons.{head,tail} > > load r->cons.head > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries very big > > > > After this patch, the old cons.head will be recaculated after failure of > > rte_atomic32_cmpset > > > > There is no such issue in X86 cpu, because X86 is strong memory order model > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > Signed-off-by: jia.he@hxt-semitech.com > > Signed-off-by: jie2.liu@hxt-semitech.com > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > --- > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index 5e9b3b7..15c72e2 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > > n = max; > > > > *old_head = r->prod.head; > > + > > + /* load of prod.tail can't be reordered before cons.head */ > > + rte_smp_rmb(); > > + > > const uint32_t cons_tail = r->cons.tail; > > /* > > * The subtraction is done between two unsigned 32bits value > > @@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > > n = max; > > > > *old_head = r->cons.head; > > + > > + /* load of prod.tail can't be reordered before cons.head */ > > + rte_smp_rmb(); > > + > > const uint32_t prod_tail = r->prod.tail; > > /* The subtraction is done between two unsigned 32bits value > > * (the result is always modulo 32 bits even if we have > > -- > > 2.7.4 > > > > The explanation convinces me. > > However, since it's in a critical path, it would be good to have other > opinions. This patch reminds me this discussion, that was also related to > memory barrier, but at another place: > http://dpdk.org/ml/archives/dev/2016-July/043765.html > Lead to that patch: http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > But finally reverted: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > Konstatin, Jerin, do you have any comment? For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, but I can't see how read reordering would screw things up here... Probably just me and arm or ppc guys could explain what will be the problem if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce it with existing one? Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-12 17:05 ` Ananyev, Konstantin @ 2017-10-12 17:23 ` Jerin Jacob 2017-10-13 1:02 ` Jia He 2017-10-13 0:24 ` Liu, Jie2 1 sibling, 1 reply; 41+ messages in thread From: Jerin Jacob @ 2017-10-12 17:23 UTC (permalink / raw) To: Ananyev, Konstantin Cc: Olivier MATZ, Jia He, dev, jia.he, jie2.liu, bing.zhao -----Original Message----- > Date: Thu, 12 Oct 2017 17:05:50 +0000 > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com> > To: Olivier MATZ <olivier.matz@6wind.com>, Jia He <hejianet@gmail.com> > CC: "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "jerin.jacob@caviumnetworks.com" > <jerin.jacob@caviumnetworks.com> > Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when > doing enqueue/dequeue > > Hi guys, > > > -----Original Message----- > > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > > Sent: Thursday, October 12, 2017 4:54 PM > > To: Jia He <hejianet@gmail.com> > > Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt-semitech.com; Ananyev, Konstantin > > <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com > > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue > > > > Hi, > > > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > > Before this patch: > > > In __rte_ring_move_cons_head() > > > ... > > > do { > > > /* Restore n as it may change every loop */ > > > n = max; > > > > > > *old_head = r->cons.head; //1st load > > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > > > In weak memory order architectures(powerpc,arm), the 2nd load might be > > > reodered before the 1st load, that makes *entries is bigger than we wanted. > > > This nasty reording messed enque/deque up. > > > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > > load r->prod.tail > > > in enqueue: > > > load r->cons.tail > > > load r->prod.head > > > > > > store r->prod.tail > > > > > > load r->cons.head > > > load r->prod.tail > > > ... > > > store r->cons.{head,tail} > > > load r->cons.head > > > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries very big > > > > > > After this patch, the old cons.head will be recaculated after failure of > > > rte_atomic32_cmpset > > > > > > There is no such issue in X86 cpu, because X86 is strong memory order model > > > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > > Signed-off-by: jia.he@hxt-semitech.com > > > Signed-off-by: jie2.liu@hxt-semitech.com > > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > > > --- > > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > > index 5e9b3b7..15c72e2 100644 > > > --- a/lib/librte_ring/rte_ring.h > > > +++ b/lib/librte_ring/rte_ring.h > > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > > > n = max; > > > > > > *old_head = r->prod.head; > > > + > > > + /* load of prod.tail can't be reordered before cons.head */ > > > + rte_smp_rmb(); > > > + > > > const uint32_t cons_tail = r->cons.tail; > > > /* > > > * The subtraction is done between two unsigned 32bits value > > > @@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > > > n = max; > > > > > > *old_head = r->cons.head; > > > + > > > + /* load of prod.tail can't be reordered before cons.head */ > > > + rte_smp_rmb(); > > > + > > > const uint32_t prod_tail = r->prod.tail; > > > /* The subtraction is done between two unsigned 32bits value > > > * (the result is always modulo 32 bits even if we have > > > -- > > > 2.7.4 > > > > > > > The explanation convinces me. > > > > However, since it's in a critical path, it would be good to have other > > opinions. This patch reminds me this discussion, that was also related to > > memory barrier, but at another place: > > http://dpdk.org/ml/archives/dev/2016-July/043765.html > > Lead to that patch: http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > > But finally reverted: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > > > Konstatin, Jerin, do you have any comment? > > For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, > but I can't see how read reordering would screw things up here... > Probably just me and arm or ppc guys could explain what will be the problem > if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). > I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? > Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce > it with existing one? On the same lines, Jia He, jie2.liu, bing.zhao, Is this patch based on code review or do you saw this issue on any of the arm/ppc target? arm64 will have performance impact with this change. > Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-12 17:23 ` Jerin Jacob @ 2017-10-13 1:02 ` Jia He 2017-10-13 1:15 ` Jia He 2017-10-13 1:16 ` Jia He 0 siblings, 2 replies; 41+ messages in thread From: Jia He @ 2017-10-13 1:02 UTC (permalink / raw) To: Jerin Jacob, Ananyev, Konstantin Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao Hi Jerin On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Thu, 12 Oct 2017 17:05:50 +0000 >> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com> >> To: Olivier MATZ <olivier.matz@6wind.com>, Jia He <hejianet@gmail.com> >> CC: "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "jerin.jacob@caviumnetworks.com" >> <jerin.jacob@caviumnetworks.com> >> Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when >> doing enqueue/dequeue >> >> Hi guys, >> >>> -----Original Message----- >>> From: Olivier MATZ [mailto:olivier.matz@6wind.com] >>> Sent: Thursday, October 12, 2017 4:54 PM >>> To: Jia He <hejianet@gmail.com> >>> Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt-semitech.com; Ananyev, Konstantin >>> <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com >>> Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue >>> >>> Hi, >>> >>> On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: >>>> Before this patch: >>>> In __rte_ring_move_cons_head() >>>> ... >>>> do { >>>> /* Restore n as it may change every loop */ >>>> n = max; >>>> >>>> *old_head = r->cons.head; //1st load >>>> const uint32_t prod_tail = r->prod.tail; //2nd load >>>> >>>> In weak memory order architectures(powerpc,arm), the 2nd load might be >>>> reodered before the 1st load, that makes *entries is bigger than we wanted. >>>> This nasty reording messed enque/deque up. >>>> >>>> cpu1(producer) cpu2(consumer) cpu3(consumer) >>>> load r->prod.tail >>>> in enqueue: >>>> load r->cons.tail >>>> load r->prod.head >>>> >>>> store r->prod.tail >>>> >>>> load r->cons.head >>>> load r->prod.tail >>>> ... >>>> store r->cons.{head,tail} >>>> load r->cons.head >>>> >>>> THEN,r->cons.head will be bigger than prod_tail, then make *entries very big >>>> >>>> After this patch, the old cons.head will be recaculated after failure of >>>> rte_atomic32_cmpset >>>> >>>> There is no such issue in X86 cpu, because X86 is strong memory order model >>>> >>>> Signed-off-by: Jia He <hejianet@gmail.com> >>>> Signed-off-by: jia.he@hxt-semitech.com >>>> Signed-off-by: jie2.liu@hxt-semitech.com >>>> Signed-off-by: bing.zhao@hxt-semitech.com >>>> >>>> --- >>>> lib/librte_ring/rte_ring.h | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h >>>> index 5e9b3b7..15c72e2 100644 >>>> --- a/lib/librte_ring/rte_ring.h >>>> +++ b/lib/librte_ring/rte_ring.h >>>> @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, >>>> n = max; >>>> >>>> *old_head = r->prod.head; >>>> + >>>> + /* load of prod.tail can't be reordered before cons.head */ >>>> + rte_smp_rmb(); >>>> + >>>> const uint32_t cons_tail = r->cons.tail; >>>> /* >>>> * The subtraction is done between two unsigned 32bits value >>>> @@ -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, >>>> n = max; >>>> >>>> *old_head = r->cons.head; >>>> + >>>> + /* load of prod.tail can't be reordered before cons.head */ >>>> + rte_smp_rmb(); >>>> + >>>> const uint32_t prod_tail = r->prod.tail; >>>> /* The subtraction is done between two unsigned 32bits value >>>> * (the result is always modulo 32 bits even if we have >>>> -- >>>> 2.7.4 >>>> >>> The explanation convinces me. >>> >>> However, since it's in a critical path, it would be good to have other >>> opinions. This patch reminds me this discussion, that was also related to >>> memory barrier, but at another place: >>> http://dpdk.org/ml/archives/dev/2016-July/043765.html >>> Lead to that patch: http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e >>> But finally reverted: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 >>> >>> Konstatin, Jerin, do you have any comment? >> For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, >> but I can't see how read reordering would screw things up here... >> Probably just me and arm or ppc guys could explain what will be the problem >> if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). >> I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? >> Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce >> it with existing one? > On the same lines, > > Jia He, jie2.liu, bing.zhao, > > Is this patch based on code review or do you saw this issue on any of the > arm/ppc target? arm64 will have performance impact with this change. Based on mbuf_autotest, the rte_panic will be invoked in seconds. PANIC in test_refcnt_iter(): (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free 1: [./test(rte_dump_stack+0x38) [0x58d868]] Aborted (core dumped) Cheers, Jia > > >> Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 1:02 ` Jia He @ 2017-10-13 1:15 ` Jia He 2017-10-13 1:16 ` Jia He 1 sibling, 0 replies; 41+ messages in thread From: Jia He @ 2017-10-13 1:15 UTC (permalink / raw) To: Jerin Jacob, Ananyev, Konstantin Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao On 10/13/2017 9:02 AM, Jia He Wrote: > Hi Jerin > > > On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >> -----Original Message----- >>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>> [...] >> On the same lines, >> >> Jia He, jie2.liu, bing.zhao, >> >> Is this patch based on code review or do you saw this issue on any of >> the >> arm/ppc target? arm64 will have performance impact with this change. sorry, miss one important information Our platform is an aarch64 server with 46 cpus. If we reduced the involved cpu numbers, the bug occurred less frequently. Yes, mb barrier impact the performance, but correctness is more important, isn't it ;-) Maybe we can find any other lightweight barrier here? Cheers, Jia > Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > PANIC in test_refcnt_iter(): > (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > 1: [./test(rte_dump_stack+0x38) [0x58d868]] > Aborted (core dumped) > > Cheers, > Jia >> >> >>> Konstantin > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 1:02 ` Jia He 2017-10-13 1:15 ` Jia He @ 2017-10-13 1:16 ` Jia He 2017-10-13 1:49 ` Jerin Jacob 2017-10-19 10:02 ` Ananyev, Konstantin 1 sibling, 2 replies; 41+ messages in thread From: Jia He @ 2017-10-13 1:16 UTC (permalink / raw) To: Jerin Jacob, Ananyev, Konstantin Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao Hi On 10/13/2017 9:02 AM, Jia He Wrote: > Hi Jerin > > > On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >> -----Original Message----- >>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>> [...] >> On the same lines, >> >> Jia He, jie2.liu, bing.zhao, >> >> Is this patch based on code review or do you saw this issue on any of >> the >> arm/ppc target? arm64 will have performance impact with this change. sorry, miss one important information Our platform is an aarch64 server with 46 cpus. If we reduced the involved cpu numbers, the bug occurred less frequently. Yes, mb barrier impact the performance, but correctness is more important, isn't it ;-) Maybe we can find any other lightweight barrier here? Cheers, Jia > Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > PANIC in test_refcnt_iter(): > (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > 1: [./test(rte_dump_stack+0x38) [0x58d868]] > Aborted (core dumped) > > Cheers, > Jia >> >> >>> Konstantin > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 1:16 ` Jia He @ 2017-10-13 1:49 ` Jerin Jacob 2017-10-13 3:23 ` Jia He 2017-10-13 7:33 ` Jianbo Liu 2017-10-19 10:02 ` Ananyev, Konstantin 1 sibling, 2 replies; 41+ messages in thread From: Jerin Jacob @ 2017-10-13 1:49 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao -----Original Message----- > Date: Fri, 13 Oct 2017 09:16:31 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Ananyev, Konstantin" > <konstantin.ananyev@intel.com> > Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>, > "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, > "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, > "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com> > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when > doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.3.0 > > Hi > > > On 10/13/2017 9:02 AM, Jia He Wrote: > > Hi Jerin > > > > > > On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > > > -----Original Message----- > > > > Date: Thu, 12 Oct 2017 17:05:50 +0000 > > > > > [...] > > > On the same lines, > > > > > > Jia He, jie2.liu, bing.zhao, > > > > > > Is this patch based on code review or do you saw this issue on any > > > of the > > > arm/ppc target? arm64 will have performance impact with this change. > sorry, miss one important information > Our platform is an aarch64 server with 46 cpus. Is this an OOO(Out of order execution) aarch64 CPU implementation? > If we reduced the involved cpu numbers, the bug occurred less frequently. > > Yes, mb barrier impact the performance, but correctness is more important, > isn't it ;-) Yes. > Maybe we can find any other lightweight barrier here? Yes, Regarding the lightweight barrier, arm64 has native support for acquire and release semantics, which is exposed through gcc as architecture agnostic functions. https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html http://preshing.com/20130922/acquire-and-release-fences/ Good to know, 1) How much overhead this patch in your platform? Just relative numbers are enough 2) As a prototype, Is Changing to acquire and release schematics reduces the overhead in your platform? Reference FreeBSD ring/DPDK style ring implementation through acquire and release schematics https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c I will also spend on cycles on this. > > Cheers, > Jia > > Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > > > PANIC in test_refcnt_iter(): > > (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > > 1: [./test(rte_dump_stack+0x38) [0x58d868]] > > Aborted (core dumped) > > > > Cheers, > > Jia > > > > > > > > > > Konstantin > > > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 1:49 ` Jerin Jacob @ 2017-10-13 3:23 ` Jia He 2017-10-13 5:57 ` Zhao, Bing 2017-10-13 7:33 ` Jianbo Liu 1 sibling, 1 reply; 41+ messages in thread From: Jia He @ 2017-10-13 3:23 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao [-- Attachment #1: Type: text/plain, Size: 3425 bytes --] Hi Jerin On 10/13/2017 9:49 AM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Fri, 13 Oct 2017 09:16:31 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Ananyev, Konstantin" >> <konstantin.ananyev@intel.com> >> Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>, >> "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, >> "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, >> "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com> >> Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when >> doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.3.0 >> >> Hi >> >> >> On 10/13/2017 9:02 AM, Jia He Wrote: >>> Hi Jerin >>> >>> >>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >>>> -----Original Message----- >>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>>>> >> [...] >>>> On the same lines, >>>> >>>> Jia He, jie2.liu, bing.zhao, >>>> >>>> Is this patch based on code review or do you saw this issue on any >>>> of the >>>> arm/ppc target? arm64 will have performance impact with this change. >> sorry, miss one important information >> Our platform is an aarch64 server with 46 cpus. > Is this an OOO(Out of order execution) aarch64 CPU implementation? I think so, it is a server cpu (ARMv8-A), but do you know how to confirm it? cat /proc/cpuinfo processor : 0 BogoMIPS : 40.00 Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid asimdrdm CPU implementer : 0x51 CPU architecture: 8 CPU variant : 0x0 CPU part : 0x800 CPU revision : 0 >> If we reduced the involved cpu numbers, the bug occurred less frequently. >> >> Yes, mb barrier impact the performance, but correctness is more important, >> isn't it ;-) > Yes. > >> Maybe we can find any other lightweight barrier here? > Yes, Regarding the lightweight barrier, arm64 has native support for acquire and release > semantics, which is exposed through gcc as architecture agnostic > functions. > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html > http://preshing.com/20130922/acquire-and-release-fences/ > > Good to know, > 1) How much overhead this patch in your platform? Just relative > numbers are enough I create a *standalone* test case for test_mbuf Attached the debug patch It is hard to believe but the truth is that the performance after adding rmb barrier is better than no adding. With this patch (4 times running) time ./test_good --no-huge -l 1-20 real 0m23.311s user 7m21.870s sys 0m0.021s time ./test_bad --no-huge -l 1-20 Without this patch real 0m38.972s user 12m35.271s sys 0m0.030s Cheers, Jia > 2) As a prototype, Is Changing to acquire and release schematics > reduces the overhead in your platform? > > Reference FreeBSD ring/DPDK style ring implementation through acquire > and release schematics > https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c > > I will also spend on cycles on this. > > >> Cheers, >> Jia >>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. >>> >>> PANIC in test_refcnt_iter(): >>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free >>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] >>> Aborted (core dumped) >>> >>> Cheers, >>> Jia >>>> >>>>> Konstantin [-- Attachment #2: barrier_performance_debug.patch --] [-- Type: text/plain, Size: 3180 bytes --] diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index 5e9b3b7..23168e7 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -517,6 +517,7 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, n = max; *old_head = r->cons.head; + rte_smp_rmb(); const uint32_t prod_tail = r->prod.tail; /* The subtraction is done between two unsigned 32bits value * (the result is always modulo 32 bits even if we have diff --git a/test/test/test.c b/test/test/test.c index 9accbd1..6910b06 100644 --- a/test/test/test.c +++ b/test/test/test.c @@ -61,6 +61,7 @@ extern cmdline_parse_ctx_t main_ctx[]; #include "test.h" +extern int test_mbuf(void); #define RTE_LOGTYPE_APP RTE_LOGTYPE_USER1 const char *prgname; /* to be set to argv[0] */ @@ -135,6 +136,9 @@ main(int argc, char **argv) RTE_LOG(INFO, APP, "HPET is not enabled, using TSC as default timer\n"); + test_mbuf(); + printf("test_mbuf done\n"); + return 0; #ifdef RTE_LIBRTE_CMDLINE cl = cmdline_stdin_new(main_ctx, "RTE>>"); diff --git a/test/test/test_mbuf.c b/test/test/test_mbuf.c index 3396b4a..7277260 100644 --- a/test/test/test_mbuf.c +++ b/test/test/test_mbuf.c @@ -59,7 +59,6 @@ #include <rte_cycles.h> #include "test.h" - #define MBUF_DATA_SIZE 2048 #define NB_MBUF 128 #define MBUF_TEST_DATA_LEN 1464 @@ -85,7 +84,7 @@ static volatile uint32_t refcnt_stop_slaves; static unsigned refcnt_lcore[RTE_MAX_LCORE]; - +int test_mbuf(void); #endif /* @@ -718,7 +717,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, for (i = 0, n = rte_mempool_avail_count(refcnt_pool); i != n && (m = rte_pktmbuf_alloc(refcnt_pool)) != NULL; i++) { - ref = RTE_MAX(rte_rand() % REFCNT_MAX_REF, 1UL); + ref = REFCNT_MAX_REF; tref += ref; if ((ref & 1) != 0) { rte_pktmbuf_refcnt_update(m, ref); @@ -745,14 +744,17 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { refcnt_lcore[lcore] += tref; - printf("%s(lcore=%u, iter=%u) completed, " + /*printf("%s(lcore=%u, iter=%u) completed, " "%u references processed\n", - __func__, lcore, iter, tref); + __func__, lcore, iter, tref);*/ return; } rte_delay_ms(100); } + rte_mempool_dump(stdout, refcnt_pool); + rte_ring_dump(stdout, refcnt_pool->pool_data); + rte_ring_dump(stdout, refcnt_mbuf_ring); rte_panic("(lcore=%u, iter=%u): after %us only " "%u of %u mbufs left free\n", lcore, iter, wn, i, n); } @@ -766,7 +768,7 @@ test_refcnt_master(struct rte_mempool *refcnt_pool, lcore = rte_lcore_id(); printf("%s started at lcore %u\n", __func__, lcore); - for (i = 0; i != REFCNT_MAX_ITER; i++) + for (i = 0; i != 10*REFCNT_MAX_ITER; i++) test_refcnt_iter(lcore, i, refcnt_pool, refcnt_mbuf_ring); refcnt_stop_slaves = 1; @@ -1058,8 +1060,7 @@ test_mbuf_linearize_check(struct rte_mempool *pktmbuf_pool) return 0; } -static int -test_mbuf(void) +int test_mbuf(void) { int ret = -1; struct rte_mempool *pktmbuf_pool = NULL; ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 3:23 ` Jia He @ 2017-10-13 5:57 ` Zhao, Bing 0 siblings, 0 replies; 41+ messages in thread From: Zhao, Bing @ 2017-10-13 5:57 UTC (permalink / raw) To: Jia He, Jerin Jacob Cc: Ananyev, Konstantin, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao Hi Jerin, The patch is as below. Jie and I just modified from the original case. 1. If all consumers(slave lcore) start to work after the producer fill the ring in every iteration test, there will be no problem. 2. If only one consumer with one producer, there will not be any problem either. 3. The more consumers are involved in, the higher reproduction rate will be observed. 4. With the rmb, no issue is found in more than 100 thousands iteration tests. ./test/test/test -l 4-11 -n 6 test_refcnt_iter(lcore=4, iter=0) completed, 10 references processed test_refcnt_iter(lcore=4, iter=1) completed, 10 references processed test_refcnt_iter(lcore=4, iter=2) completed, 10 references processed test_refcnt_iter(lcore=4, iter=3) completed, 10 references processed test_refcnt_iter(lcore=4, iter=4) completed, 10 references processed m ref is 2 test_refcnt_iter(lcore=4, iter=5) completed, 10 references processed test_refcnt_iter(lcore=4, iter=6) completed, 10 references processed test_refcnt_iter(lcore=4, iter=7) completed, 10 references processed PANIC in __rte_ring_move_cons_head(): test_refcnt_iter(lcore=4, iter=8) completed, 10 references processed test_refcnt_iter(lcore=4, iter=9) completed, 10 references processed test_refcnt_iter(lcore=4, iter=10) completed, 10 references processed test_refcnt_iter(lcore=4, iter=11) completed, 10 references processed m ref is 2 test_refcnt_iter(lcore=4, iter=12) completed, 10 references processed m ref is 2 test_refcnt_iter(lcore=4, iter=13) completed, 10 references processed test_refcnt_iter(lcore=4, iter=14) completed, 10 references processed test_refcnt_iter(lcore=4, iter=15) completed, 10 references processed bizhao 4294967295 0x50 0x51 0x4f 0x50 0x50 m ref is 2 Note: only 0x50 of the filling operation and the PT is different in every read act (0x4f != 0x50), and CH > PT. It does not exceed the ring size(512) so the subtraction is overflow. Then since the CH is correct, so the CH with +1 is considered to be correct and over the PT. Test Patch: -------------------------------- diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c old mode 100644 new mode 100755 index 26a62b8..ba0a6db --- a/lib/librte_mbuf/rte_mbuf.c +++ b/lib/librte_mbuf/rte_mbuf.c @@ -243,8 +243,8 @@ rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len) __rte_mbuf_sanity_check(m, 1); - fprintf(f, "dump mbuf at %p, phys=%"PRIx64", buf_len=%u\n", - m, (uint64_t)m->buf_physaddr, (unsigned)m->buf_len); + fprintf(f, "dump mbuf at %p, phys=%"PRIx64", buf_len=%u, ref=%u\n", + m, (uint64_t)m->buf_physaddr, (unsigned)m->buf_len, (unsigned)m->refcnt); fprintf(f, " pkt_len=%"PRIu32", ol_flags=%"PRIx64", nb_segs=%u, " "in_port=%u\n", m->pkt_len, m->ol_flags, (unsigned)m->nb_segs, (unsigned)m->port); diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h old mode 100644 new mode 100755 index 8f5a493..3456b69 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -502,6 +502,7 @@ __rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table, * - Actual number of objects dequeued. * If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only. */ +#include <rte_debug.h> static __rte_always_inline unsigned int __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, unsigned int n, enum rte_ring_queue_behavior behavior, @@ -517,6 +518,7 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, n = max; *old_head = r->cons.head; + //rte_smp_rmb(); const uint32_t prod_tail = r->prod.tail; /* The subtraction is done between two unsigned 32bits value * (the result is always modulo 32 bits even if we have @@ -532,6 +534,8 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, return 0; *new_head = *old_head + n; + if (*new_head > prod_tail) //|| r->cons.head > r->prod.tail) + rte_panic("bizhao %u %#x %#x %#x %#x %#x\n", *entries, *old_head, *new_head, prod_tail, r->cons.head, r->prod.tail); if (is_sc) r->cons.head = *new_head, success = 1; else diff --git a/test/test/test_mbuf.c b/test/test/test_mbuf.c old mode 100644 new mode 100755 index 3396b4a..731535c --- a/test/test/test_mbuf.c +++ b/test/test/test_mbuf.c @@ -71,10 +71,12 @@ /* size of private data for mbuf in pktmbuf_pool2 */ #define MBUF2_PRIV_SIZE 128 -#define REFCNT_MAX_ITER 64 +// #define REFCNT_MAX_ITER 64 +#define REFCNT_MAX_ITER 32 #define REFCNT_MAX_TIMEOUT 10 -#define REFCNT_MAX_REF (RTE_MAX_LCORE) -#define REFCNT_MBUF_NUM 64 +// #define REFCNT_MAX_REF (RTE_MAX_LCORE) +#define REFCNT_MAX_REF 512 +#define REFCNT_MBUF_NUM 1 #define REFCNT_RING_SIZE (REFCNT_MBUF_NUM * REFCNT_MAX_REF) #define MAGIC_DATA 0x42424242 @@ -683,12 +685,18 @@ test_refcnt_slave(void *arg) lcore = rte_lcore_id(); printf("%s started at lcore %u\n", __func__, lcore); + uint32_t ava; free = 0; while (refcnt_stop_slaves == 0) { - if (rte_ring_dequeue(refcnt_mbuf_ring, &mp) == 0) { + ava = rte_ring_dequeue_bulk(refcnt_mbuf_ring, &mp ,1, NULL); + if (ava > 0) { + if (ava > 10) { + rte_panic("OMG %u %u\n", refcnt_mbuf_ring->prod.tail, refcnt_mbuf_ring->cons.tail); + } free++; - rte_pktmbuf_free(mp); + // rte_pktmbuf_free(mp); + rte_pktmbuf_refcnt_update(mp, -1); } } @@ -719,6 +727,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, i != n && (m = rte_pktmbuf_alloc(refcnt_pool)) != NULL; i++) { ref = RTE_MAX(rte_rand() % REFCNT_MAX_REF, 1UL); + ref = 10; tref += ref; if ((ref & 1) != 0) { rte_pktmbuf_refcnt_update(m, ref); @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, rte_ring_enqueue(refcnt_mbuf_ring, m); } } - rte_pktmbuf_free(m); + // rte_pktmbuf_free(m); } if (i != n) @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, while (!rte_ring_empty(refcnt_mbuf_ring)) ; + if (NULL != m) { + if (1 != rte_mbuf_refcnt_read(m)) + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); + rte_pktmbuf_free(m); + } + /* check that all mbufs are back into mempool by now */ for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { (END) -------------------------------- BR. Bing On 2017/10/13 11:23, Jia He wrote: > Hi Jerin > > > On 10/13/2017 9:49 AM, Jerin Jacob Wrote: >> -----Original Message----- >>> Date: Fri, 13 Oct 2017 09:16:31 +0800 >>> From: Jia He <hejianet@gmail.com> >>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Ananyev, Konstantin" >>> <konstantin.ananyev@intel.com> >>> Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" >>> <dev@dpdk.org>, >>> "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, >>> "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, >>> "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com> >>> Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when >>> doing enqueue/dequeue >>> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >>> Thunderbird/52.3.0 >>> >>> Hi >>> >>> >>> On 10/13/2017 9:02 AM, Jia He Wrote: >>>> Hi Jerin >>>> >>>> >>>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >>>>> -----Original Message----- >>>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>>>>> >>> [...] >>>>> On the same lines, >>>>> >>>>> Jia He, jie2.liu, bing.zhao, >>>>> >>>>> Is this patch based on code review or do you saw this issue on any >>>>> of the >>>>> arm/ppc target? arm64 will have performance impact with this change. >>> sorry, miss one important information >>> Our platform is an aarch64 server with 46 cpus. >> Is this an OOO(Out of order execution) aarch64 CPU implementation? > I think so, it is a server cpu (ARMv8-A), but do you know how to > confirm it? > cat /proc/cpuinfo > processor : 0 > BogoMIPS : 40.00 > Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid > asimdrdm > CPU implementer : 0x51 > CPU architecture: 8 > CPU variant : 0x0 > CPU part : 0x800 > CPU revision : 0 >>> If we reduced the involved cpu numbers, the bug occurred less >>> frequently. >>> >>> Yes, mb barrier impact the performance, but correctness is more >>> important, >>> isn't it ;-) >> Yes. >> >>> Maybe we can find any other lightweight barrier here? >> Yes, Regarding the lightweight barrier, arm64 has native support for >> acquire and release >> semantics, which is exposed through gcc as architecture agnostic >> functions. >> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html >> http://preshing.com/20130922/acquire-and-release-fences/ >> >> Good to know, >> 1) How much overhead this patch in your platform? Just relative >> numbers are enough > I create a *standalone* test case for test_mbuf > Attached the debug patch > It is hard to believe but the truth is that the performance after > adding rmb barrier > is better than no adding. > > With this patch (4 times running) > time ./test_good --no-huge -l 1-20 > real 0m23.311s > user 7m21.870s > sys 0m0.021s > > time ./test_bad --no-huge -l 1-20 > Without this patch > real 0m38.972s > user 12m35.271s > sys 0m0.030s > > Cheers, > Jia >> 2) As a prototype, Is Changing to acquire and release schematics >> reduces the overhead in your platform? >> >> Reference FreeBSD ring/DPDK style ring implementation through acquire >> and release schematics >> https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c >> >> >> I will also spend on cycles on this. >> >> >>> Cheers, >>> Jia >>>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. >>>> >>>> PANIC in test_refcnt_iter(): >>>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free >>>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] >>>> Aborted (core dumped) >>>> >>>> Cheers, >>>> Jia >>>>> >>>>>> Konstantin > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 1:49 ` Jerin Jacob 2017-10-13 3:23 ` Jia He @ 2017-10-13 7:33 ` Jianbo Liu 2017-10-13 8:20 ` Jia He 1 sibling, 1 reply; 41+ messages in thread From: Jianbo Liu @ 2017-10-13 7:33 UTC (permalink / raw) To: Jerin Jacob Cc: Jia He, Ananyev, Konstantin, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao The 10/13/2017 07:19, Jerin Jacob wrote: > -----Original Message----- > > Date: Fri, 13 Oct 2017 09:16:31 +0800 > > From: Jia He <hejianet@gmail.com> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Ananyev, Konstantin" > > <konstantin.ananyev@intel.com> > > Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>, > > "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, > > "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, > > "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com> > > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when > > doing enqueue/dequeue > > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > > Thunderbird/52.3.0 > > > > Hi > > > > > > On 10/13/2017 9:02 AM, Jia He Wrote: > > > Hi Jerin > > > > > > > > > On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > > > > -----Original Message----- > > > > > Date: Thu, 12 Oct 2017 17:05:50 +0000 > > > > > > > [...] > > > > On the same lines, > > > > > > > > Jia He, jie2.liu, bing.zhao, > > > > > > > > Is this patch based on code review or do you saw this issue on any > > > > of the > > > > arm/ppc target? arm64 will have performance impact with this change. > > sorry, miss one important information > > Our platform is an aarch64 server with 46 cpus. > > Is this an OOO(Out of order execution) aarch64 CPU implementation? > > > If we reduced the involved cpu numbers, the bug occurred less frequently. > > > > Yes, mb barrier impact the performance, but correctness is more important, > > isn't it ;-) > > Yes. > > > Maybe we can find any other lightweight barrier here? > > Yes, Regarding the lightweight barrier, arm64 has native support for acquire and release > semantics, which is exposed through gcc as architecture agnostic > functions. > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html > http://preshing.com/20130922/acquire-and-release-fences/ > > Good to know, > 1) How much overhead this patch in your platform? Just relative > numbers are enough > 2) As a prototype, Is Changing to acquire and release schematics > reduces the overhead in your platform? > +1, can you try what ODP does in the link mentioned below? > Reference FreeBSD ring/DPDK style ring implementation through acquire > and release schematics > https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c > > I will also spend on cycles on this. > > > > > > Cheers, > > Jia > > > Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > > > > > PANIC in test_refcnt_iter(): > > > (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > > > 1: [./test(rte_dump_stack+0x38) [0x58d868]] > > > Aborted (core dumped) > > > > > > Cheers, > > > Jia > > > > > > > > > > > > > Konstantin > > > > > -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 7:33 ` Jianbo Liu @ 2017-10-13 8:20 ` Jia He 0 siblings, 0 replies; 41+ messages in thread From: Jia He @ 2017-10-13 8:20 UTC (permalink / raw) To: Jianbo Liu, Jerin Jacob Cc: Ananyev, Konstantin, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao Hi On 10/13/2017 3:33 PM, Jianbo Liu Wrote: > The 10/13/2017 07:19, Jerin Jacob wrote: >> -----Original Message----- >>> Date: Fri, 13 Oct 2017 09:16:31 +0800 >>> From: Jia He <hejianet@gmail.com> >>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Ananyev, Konstantin" >>> <konstantin.ananyev@intel.com> >>> Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>, >>> "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, >>> "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, >>> "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com> >>> Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading when >>> doing enqueue/dequeue >>> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >>> Thunderbird/52.3.0 >>> >>> Hi >>> >>> >>> On 10/13/2017 9:02 AM, Jia He Wrote: >>>> Hi Jerin >>>> >>>> >>>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >>>>> -----Original Message----- >>>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>>>>> >>> [...] >>>>> On the same lines, >>>>> >>>>> Jia He, jie2.liu, bing.zhao, >>>>> >>>>> Is this patch based on code review or do you saw this issue on any >>>>> of the >>>>> arm/ppc target? arm64 will have performance impact with this change. >>> sorry, miss one important information >>> Our platform is an aarch64 server with 46 cpus. >> Is this an OOO(Out of order execution) aarch64 CPU implementation? >> >>> If we reduced the involved cpu numbers, the bug occurred less frequently. >>> >>> Yes, mb barrier impact the performance, but correctness is more important, >>> isn't it ;-) >> Yes. >> >>> Maybe we can find any other lightweight barrier here? >> Yes, Regarding the lightweight barrier, arm64 has native support for acquire and release >> semantics, which is exposed through gcc as architecture agnostic >> functions. >> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html >> http://preshing.com/20130922/acquire-and-release-fences/ >> >> Good to know, >> 1) How much overhead this patch in your platform? Just relative >> numbers are enough >> 2) As a prototype, Is Changing to acquire and release schematics >> reduces the overhead in your platform? >> > +1, can you try what ODP does in the link mentioned below? Sure, pls see the result: root@server:~/odp/test/linux-generic/ring# ./ring_main HW time counter freq: 20000000 hz _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory _ishm.c:880:_odp_ishm_reserve():No huge pages, fall back to normal pages. check: /proc/sys/vm/nr_hugepages. _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory PKTIO: initialized loop interface. PKTIO: initialized ipc interface. PKTIO: initialized socket mmap, use export ODP_PKTIO_DISABLE_SOCKET_MMAP=1 to disable. PKTIO: initialized socket mmsg,use export ODP_PKTIO_DISABLE_SOCKET_MMSG=1 to disable. _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory ODP API version: 1.15.0 ODP implementation name: "odp-linux" ODP implementation version: "odp-linux" 1.15.0-0 (v1.15.0) 1.15.0.0 CUnit - A unit testing framework for C - Version 2.1-3 http://cunit.sourceforge.net/ Suite: ring basic Test: ring_test_basic_create ...pktio/ring.c:177:_ring_create():Requested size is invalid, must be power of 2,and do not exceed the size limit 268435455 _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory _ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory passed Test: ring_test_basic_burst ...passed Test: ring_test_basic_bulk ...passed Test: ring_test_basic_watermark ...passed_ishmphy.c:152:_odp_ishmphy_map():mmap failed:Cannot allocate memory Suite: ring stress Test: ring_test_stress_1_1_producer_consumer ...passed Test: ring_test_stress_1_N_producer_consumer ...passed Test: ring_test_stress_N_1_producer_consumer ...passed Test: ring_test_stress_N_M_producer_consumer ... <the test case has hung here for half an hour> Cheers, Jia >> Reference FreeBSD ring/DPDK style ring implementation through acquire >> and release schematics >> https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c >> >> I will also spend on cycles on this. >> >> >>> Cheers, >>> Jia >>>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. >>>> >>>> PANIC in test_refcnt_iter(): >>>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free >>>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] >>>> Aborted (core dumped) >>>> >>>> Cheers, >>>> Jia >>>>> >>>>>> Konstantin > -- > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 1:16 ` Jia He 2017-10-13 1:49 ` Jerin Jacob @ 2017-10-19 10:02 ` Ananyev, Konstantin 2017-10-19 11:18 ` Zhao, Bing 1 sibling, 1 reply; 41+ messages in thread From: Ananyev, Konstantin @ 2017-10-19 10:02 UTC (permalink / raw) To: Jia He, Jerin Jacob; +Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao Hi Jia, > > Hi > > > On 10/13/2017 9:02 AM, Jia He Wrote: > > Hi Jerin > > > > > > On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > >> -----Original Message----- > >>> Date: Thu, 12 Oct 2017 17:05:50 +0000 > >>> > [...] > >> On the same lines, > >> > >> Jia He, jie2.liu, bing.zhao, > >> > >> Is this patch based on code review or do you saw this issue on any of > >> the > >> arm/ppc target? arm64 will have performance impact with this change. > sorry, miss one important information > Our platform is an aarch64 server with 46 cpus. > If we reduced the involved cpu numbers, the bug occurred less frequently. > > Yes, mb barrier impact the performance, but correctness is more > important, isn't it ;-) > Maybe we can find any other lightweight barrier here? > > Cheers, > Jia > > Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > > > PANIC in test_refcnt_iter(): > > (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > > 1: [./test(rte_dump_stack+0x38) [0x58d868]] > > Aborted (core dumped) > > So is it only reproducible with mbuf refcnt test? Could it be reproduced with some 'pure' ring test (no mempools/mbufs refcnt, etc.)? The reason I am asking - in that test we also have mbuf refcnt updates (that's what for that test was created) and we are doing some optimizations here too to avoid excessive atomic updates. BTW, if the problem is not reproducible without mbuf refcnt, can I suggest to extend the test with: - add a check that enqueue() operation was successful - walk through the pool and check/printf refcnt of each mbuf. Hopefully that would give us some extra information what is going wrong here. Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-19 10:02 ` Ananyev, Konstantin @ 2017-10-19 11:18 ` Zhao, Bing 2017-10-19 14:15 ` Ananyev, Konstantin 0 siblings, 1 reply; 41+ messages in thread From: Zhao, Bing @ 2017-10-19 11:18 UTC (permalink / raw) To: Ananyev, Konstantin, Jia He, Jerin Jacob Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao Hi, On 2017/10/19 18:02, Ananyev, Konstantin wrote: > > Hi Jia, > >> >> Hi >> >> >> On 10/13/2017 9:02 AM, Jia He Wrote: >>> Hi Jerin >>> >>> >>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >>>> -----Original Message----- >>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>>>> >> [...] >>>> On the same lines, >>>> >>>> Jia He, jie2.liu, bing.zhao, >>>> >>>> Is this patch based on code review or do you saw this issue on any of >>>> the >>>> arm/ppc target? arm64 will have performance impact with this change. >> sorry, miss one important information >> Our platform is an aarch64 server with 46 cpus. >> If we reduced the involved cpu numbers, the bug occurred less frequently. >> >> Yes, mb barrier impact the performance, but correctness is more >> important, isn't it ;-) >> Maybe we can find any other lightweight barrier here? >> >> Cheers, >> Jia >>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. >>> >>> PANIC in test_refcnt_iter(): >>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free >>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] >>> Aborted (core dumped) >>> > > So is it only reproducible with mbuf refcnt test? > Could it be reproduced with some 'pure' ring test > (no mempools/mbufs refcnt, etc.)? > The reason I am asking - in that test we also have mbuf refcnt updates > (that's what for that test was created) and we are doing some optimizations here too > to avoid excessive atomic updates. > BTW, if the problem is not reproducible without mbuf refcnt, > can I suggest to extend the test with: > - add a check that enqueue() operation was successful > - walk through the pool and check/printf refcnt of each mbuf. > Hopefully that would give us some extra information what is going wrong here. > Konstantin > > Currently, the issue is only found in this case here on the ARM platform, not sure how it is going with the X86_64 platform. In another mail of this thread, we've made a simple test based on this and captured some information and I pasted there.(I pasted the patch there :-)) And it seems that Juhamatti & Jacod found some reverting action several months ago. BR. Bing ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-19 11:18 ` Zhao, Bing @ 2017-10-19 14:15 ` Ananyev, Konstantin 2017-10-19 20:02 ` Ananyev, Konstantin 0 siblings, 1 reply; 41+ messages in thread From: Ananyev, Konstantin @ 2017-10-19 14:15 UTC (permalink / raw) To: Zhao, Bing, Jia He, Jerin Jacob Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao > > Hi, > > On 2017/10/19 18:02, Ananyev, Konstantin wrote: > > > > Hi Jia, > > > >> > >> Hi > >> > >> > >> On 10/13/2017 9:02 AM, Jia He Wrote: > >>> Hi Jerin > >>> > >>> > >>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > >>>> -----Original Message----- > >>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 > >>>>> > >> [...] > >>>> On the same lines, > >>>> > >>>> Jia He, jie2.liu, bing.zhao, > >>>> > >>>> Is this patch based on code review or do you saw this issue on any of > >>>> the > >>>> arm/ppc target? arm64 will have performance impact with this change. > >> sorry, miss one important information > >> Our platform is an aarch64 server with 46 cpus. > >> If we reduced the involved cpu numbers, the bug occurred less frequently. > >> > >> Yes, mb barrier impact the performance, but correctness is more > >> important, isn't it ;-) > >> Maybe we can find any other lightweight barrier here? > >> > >> Cheers, > >> Jia > >>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. > >>> > >>> PANIC in test_refcnt_iter(): > >>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > >>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] > >>> Aborted (core dumped) > >>> > > > > So is it only reproducible with mbuf refcnt test? > > Could it be reproduced with some 'pure' ring test > > (no mempools/mbufs refcnt, etc.)? > > The reason I am asking - in that test we also have mbuf refcnt updates > > (that's what for that test was created) and we are doing some optimizations here too > > to avoid excessive atomic updates. > > BTW, if the problem is not reproducible without mbuf refcnt, > > can I suggest to extend the test with: > > - add a check that enqueue() operation was successful > > - walk through the pool and check/printf refcnt of each mbuf. > > Hopefully that would give us some extra information what is going wrong here. > > Konstantin > > > > > Currently, the issue is only found in this case here on the ARM > platform, not sure how it is going with the X86_64 platform I understand that it is only reproducible on arm so far. What I am asking - with dpdk is there any other way to reproduce it (on arm) except then running mbuf_autotest? Something really simple that not using mbuf/mempool etc? Just do dequeue/enqueue from multiple threads and check data integrity at the end? If not - what makes you think that the problem is precisely in rte_ring code? Why not in rte_mbuf let say? >. In another > mail of this thread, we've made a simple test based on this and captured > some information and I pasted there.(I pasted the patch there :-)) Are you talking about that one: http://dpdk.org/dev/patchwork/patch/30405/ ? It still uses test/test/test_mbuf.c..., but anyway I don't really understand how mbuf_autotest supposed to work with these changes: @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, rte_ring_enqueue(refcnt_mbuf_ring, m); } } - rte_pktmbuf_free(m); + // rte_pktmbuf_free(m); } @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, while (!rte_ring_empty(refcnt_mbuf_ring)) ; + if (NULL != m) { + if (1 != rte_mbuf_refcnt_read(m)) + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); + rte_pktmbuf_free(m); + } + /* check that all mbufs are back into mempool by now */ for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { That means all your mbufs (except the last one) will still be allocated. So the test would fail - as it should, I think. > And > it seems that Juhamatti & Jacod found some reverting action several > months ago. Didn't get that one either. Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-19 14:15 ` Ananyev, Konstantin @ 2017-10-19 20:02 ` Ananyev, Konstantin 2017-10-20 1:57 ` Jia He 0 siblings, 1 reply; 41+ messages in thread From: Ananyev, Konstantin @ 2017-10-19 20:02 UTC (permalink / raw) To: Ananyev, Konstantin, Zhao, Bing, Jia He, Jerin Jacob Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin > Sent: Thursday, October 19, 2017 3:15 PM > To: Zhao, Bing <ilovethull@163.com>; Jia He <hejianet@gmail.com>; Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: Olivier MATZ <olivier.matz@6wind.com>; dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt- > semitech.com > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue > > > > > Hi, > > > > On 2017/10/19 18:02, Ananyev, Konstantin wrote: > > > > > > Hi Jia, > > > > > >> > > >> Hi > > >> > > >> > > >> On 10/13/2017 9:02 AM, Jia He Wrote: > > >>> Hi Jerin > > >>> > > >>> > > >>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > > >>>> -----Original Message----- > > >>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 > > >>>>> > > >> [...] > > >>>> On the same lines, > > >>>> > > >>>> Jia He, jie2.liu, bing.zhao, > > >>>> > > >>>> Is this patch based on code review or do you saw this issue on any of > > >>>> the > > >>>> arm/ppc target? arm64 will have performance impact with this change. > > >> sorry, miss one important information > > >> Our platform is an aarch64 server with 46 cpus. > > >> If we reduced the involved cpu numbers, the bug occurred less frequently. > > >> > > >> Yes, mb barrier impact the performance, but correctness is more > > >> important, isn't it ;-) > > >> Maybe we can find any other lightweight barrier here? > > >> > > >> Cheers, > > >> Jia > > >>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > >>> > > >>> PANIC in test_refcnt_iter(): > > >>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > > >>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] > > >>> Aborted (core dumped) > > >>> > > > > > > So is it only reproducible with mbuf refcnt test? > > > Could it be reproduced with some 'pure' ring test > > > (no mempools/mbufs refcnt, etc.)? > > > The reason I am asking - in that test we also have mbuf refcnt updates > > > (that's what for that test was created) and we are doing some optimizations here too > > > to avoid excessive atomic updates. > > > BTW, if the problem is not reproducible without mbuf refcnt, > > > can I suggest to extend the test with: > > > - add a check that enqueue() operation was successful > > > - walk through the pool and check/printf refcnt of each mbuf. > > > Hopefully that would give us some extra information what is going wrong here. > > > Konstantin > > > > > > > > Currently, the issue is only found in this case here on the ARM > > platform, not sure how it is going with the X86_64 platform > > I understand that it is only reproducible on arm so far. > What I am asking - with dpdk is there any other way to reproduce it (on arm) > except then running mbuf_autotest? > Something really simple that not using mbuf/mempool etc? > Just do dequeue/enqueue from multiple threads and check data integrity at the end? > If not - what makes you think that the problem is precisely in rte_ring code? > Why not in rte_mbuf let say? Actually I reread your original mail and finally get your point. If I understand you correctly the problem with read reordering here is that after we read prot.tail but before we read cons.head both cons.head and prod.tail might be updated, but for us prod.tail change might be unnoticed. As an example: time 0 (cons.head == 0, prod.tail == 0): prod_tail = r->prod.tail; /* due read reordering */ /* prod_tail == 0 */ time 1 (cons.head ==5, prod.tail == 5): *old_head = r->cons.head; /* cons.head == 5 */ *entries = (prod_tail - *old_head); /* *entries == (0 - 5) == 0xFFFFFFFB */ And we will move our cons.head forward, even though there are no filled entries in the ring. Is that right? As I side notice, I wonder why we have here: *entries = (prod_tail - *old_head); instead of: *entries = r->size + prod_tail - *old_head; ? Konstantin > > >. In another > > mail of this thread, we've made a simple test based on this and captured > > some information and I pasted there.(I pasted the patch there :-)) > > Are you talking about that one: > http://dpdk.org/dev/patchwork/patch/30405/ > ? > It still uses test/test/test_mbuf.c..., > but anyway I don't really understand how mbuf_autotest supposed > to work with these changes: > @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, > rte_ring_enqueue(refcnt_mbuf_ring, m); > } > } > - rte_pktmbuf_free(m); > + // rte_pktmbuf_free(m); > } > @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, > while (!rte_ring_empty(refcnt_mbuf_ring)) > ; > > + if (NULL != m) { > + if (1 != rte_mbuf_refcnt_read(m)) > + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); > + rte_pktmbuf_free(m); > + } > + > /* check that all mbufs are back into mempool by now */ > for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { > if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { > > That means all your mbufs (except the last one) will still be allocated. > So the test would fail - as it should, I think. > > > And > > it seems that Juhamatti & Jacod found some reverting action several > > months ago. > > Didn't get that one either. > Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-19 20:02 ` Ananyev, Konstantin @ 2017-10-20 1:57 ` Jia He 2017-10-20 5:43 ` Jerin Jacob 2017-10-20 7:03 ` Ananyev, Konstantin 0 siblings, 2 replies; 41+ messages in thread From: Jia He @ 2017-10-20 1:57 UTC (permalink / raw) To: Ananyev, Konstantin, Zhao, Bing, Jerin Jacob Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce On 10/20/2017 4:02 AM, Ananyev, Konstantin Wrote: > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin >> Sent: Thursday, October 19, 2017 3:15 PM >> To: Zhao, Bing <ilovethull@163.com>; Jia He <hejianet@gmail.com>; Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: Olivier MATZ <olivier.matz@6wind.com>; dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt- >> semitech.com >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue >> >>> Hi, >>> >>> On 2017/10/19 18:02, Ananyev, Konstantin wrote: >>>> Hi Jia, >>>> >>>>> Hi >>>>> >>>>> >>>>> On 10/13/2017 9:02 AM, Jia He Wrote: >>>>>> Hi Jerin >>>>>> >>>>>> >>>>>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: >>>>>>> -----Original Message----- >>>>>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 >>>>>>>> >>>>> [...] >>>>>>> On the same lines, >>>>>>> >>>>>>> Jia He, jie2.liu, bing.zhao, >>>>>>> >>>>>>> Is this patch based on code review or do you saw this issue on any of >>>>>>> the >>>>>>> arm/ppc target? arm64 will have performance impact with this change. >>>>> sorry, miss one important information >>>>> Our platform is an aarch64 server with 46 cpus. >>>>> If we reduced the involved cpu numbers, the bug occurred less frequently. >>>>> >>>>> Yes, mb barrier impact the performance, but correctness is more >>>>> important, isn't it ;-) >>>>> Maybe we can find any other lightweight barrier here? >>>>> >>>>> Cheers, >>>>> Jia >>>>>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. >>>>>> >>>>>> PANIC in test_refcnt_iter(): >>>>>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free >>>>>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] >>>>>> Aborted (core dumped) >>>>>> >>>> So is it only reproducible with mbuf refcnt test? >>>> Could it be reproduced with some 'pure' ring test >>>> (no mempools/mbufs refcnt, etc.)? >>>> The reason I am asking - in that test we also have mbuf refcnt updates >>>> (that's what for that test was created) and we are doing some optimizations here too >>>> to avoid excessive atomic updates. >>>> BTW, if the problem is not reproducible without mbuf refcnt, >>>> can I suggest to extend the test with: >>>> - add a check that enqueue() operation was successful >>>> - walk through the pool and check/printf refcnt of each mbuf. >>>> Hopefully that would give us some extra information what is going wrong here. >>>> Konstantin >>>> >>>> >>> Currently, the issue is only found in this case here on the ARM >>> platform, not sure how it is going with the X86_64 platform >> I understand that it is only reproducible on arm so far. >> What I am asking - with dpdk is there any other way to reproduce it (on arm) >> except then running mbuf_autotest? >> Something really simple that not using mbuf/mempool etc? >> Just do dequeue/enqueue from multiple threads and check data integrity at the end? >> If not - what makes you think that the problem is precisely in rte_ring code? >> Why not in rte_mbuf let say? > Actually I reread your original mail and finally get your point. > If I understand you correctly the problem with read reordering here is that > after we read prot.tail but before we read cons.head > both cons.head and prod.tail might be updated, > but for us prod.tail change might be unnoticed. > As an example: > time 0 (cons.head == 0, prod.tail == 0): > prod_tail = r->prod.tail; /* due read reordering */ > /* prod_tail == 0 */ > > time 1 (cons.head ==5, prod.tail == 5): > *old_head = r->cons.head; > /* cons.head == 5 */ > *entries = (prod_tail - *old_head); > /* *entries == (0 - 5) == 0xFFFFFFFB */ > > And we will move our cons.head forward, even though there are no filled entries in the ring. > Is that right? Yes > As I side notice, I wonder why we have here: > *entries = (prod_tail - *old_head); > instead of: > *entries = r->size + prod_tail - *old_head; > ? Yes, I agree with you at this code line. But reordering will still mess up things even after this change(I have tested, still the same as before) I thought the *entries is a door to prevent consumer from moving forward too fast than the producer. But in some cases, it is correct that prod_tail is smaller than *old_head due to the cirular queue. In other cases, it is incorrect because of read/read reordering. AFAICT, the root cause here is the producer tail and cosumer head are dependant on each other. Thus a memory barrier is neccessary. Cheers, Jia > > Konstantin > >>> . In another >>> mail of this thread, we've made a simple test based on this and captured >>> some information and I pasted there.(I pasted the patch there :-)) >> Are you talking about that one: >> http://dpdk.org/dev/patchwork/patch/30405/ >> ? >> It still uses test/test/test_mbuf.c..., >> but anyway I don't really understand how mbuf_autotest supposed >> to work with these changes: >> @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, >> rte_ring_enqueue(refcnt_mbuf_ring, m); >> } >> } >> - rte_pktmbuf_free(m); >> + // rte_pktmbuf_free(m); >> } >> @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, >> while (!rte_ring_empty(refcnt_mbuf_ring)) >> ; >> >> + if (NULL != m) { >> + if (1 != rte_mbuf_refcnt_read(m)) >> + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); >> + rte_pktmbuf_free(m); >> + } >> + >> /* check that all mbufs are back into mempool by now */ >> for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { >> if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { >> >> That means all your mbufs (except the last one) will still be allocated. >> So the test would fail - as it should, I think. >> >>> And >>> it seems that Juhamatti & Jacod found some reverting action several >>> months ago. >> Didn't get that one either. >> Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-20 1:57 ` Jia He @ 2017-10-20 5:43 ` Jerin Jacob 2017-10-23 8:49 ` Jia He 2017-10-20 7:03 ` Ananyev, Konstantin 1 sibling, 1 reply; 41+ messages in thread From: Jerin Jacob @ 2017-10-20 5:43 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce -----Original Message----- > Date: Fri, 20 Oct 2017 09:57:58 +0800 > From: Jia He <hejianet@gmail.com> > To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>, > "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, > "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, > "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com>, "Richardson, > Bruce" <bruce.richardson@intel.com> > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > > > On 10/20/2017 4:02 AM, Ananyev, Konstantin Wrote: > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin > > > Sent: Thursday, October 19, 2017 3:15 PM > > > To: Zhao, Bing <ilovethull@163.com>; Jia He <hejianet@gmail.com>; Jerin Jacob <jerin.jacob@caviumnetworks.com> > > > Cc: Olivier MATZ <olivier.matz@6wind.com>; dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; bing.zhao@hxt- > > > semitech.com > > > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue > > > > > > > Hi, > > > > > > > > On 2017/10/19 18:02, Ananyev, Konstantin wrote: > > > > > Hi Jia, > > > > > > > > > > > Hi > > > > > > > > > > > > > > > > > > On 10/13/2017 9:02 AM, Jia He Wrote: > > > > > > > Hi Jerin > > > > > > > > > > > > > > > > > > > > > On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > > > > > > > > -----Original Message----- > > > > > > > > > Date: Thu, 12 Oct 2017 17:05:50 +0000 > > > > > > > > > > > > > > > [...] > > > > > > > > On the same lines, > > > > > > > > > > > > > > > > Jia He, jie2.liu, bing.zhao, > > > > > > > > > > > > > > > > Is this patch based on code review or do you saw this issue on any of > > > > > > > > the > > > > > > > > arm/ppc target? arm64 will have performance impact with this change. > > > > > > sorry, miss one important information > > > > > > Our platform is an aarch64 server with 46 cpus. > > > > > > If we reduced the involved cpu numbers, the bug occurred less frequently. > > > > > > > > > > > > Yes, mb barrier impact the performance, but correctness is more > > > > > > important, isn't it ;-) > > > > > > Maybe we can find any other lightweight barrier here? > > > > > > > > > > > > Cheers, > > > > > > Jia > > > > > > > Based on mbuf_autotest, the rte_panic will be invoked in seconds. > > > > > > > > > > > > > > PANIC in test_refcnt_iter(): > > > > > > > (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > > > > > > > 1: [./test(rte_dump_stack+0x38) [0x58d868]] > > > > > > > Aborted (core dumped) > > > > > > > > > > > > So is it only reproducible with mbuf refcnt test? > > > > > Could it be reproduced with some 'pure' ring test > > > > > (no mempools/mbufs refcnt, etc.)? > > > > > The reason I am asking - in that test we also have mbuf refcnt updates > > > > > (that's what for that test was created) and we are doing some optimizations here too > > > > > to avoid excessive atomic updates. > > > > > BTW, if the problem is not reproducible without mbuf refcnt, > > > > > can I suggest to extend the test with: > > > > > - add a check that enqueue() operation was successful > > > > > - walk through the pool and check/printf refcnt of each mbuf. > > > > > Hopefully that would give us some extra information what is going wrong here. > > > > > Konstantin > > > > > > > > > > > > > > Currently, the issue is only found in this case here on the ARM > > > > platform, not sure how it is going with the X86_64 platform > > > I understand that it is only reproducible on arm so far. > > > What I am asking - with dpdk is there any other way to reproduce it (on arm) > > > except then running mbuf_autotest? > > > Something really simple that not using mbuf/mempool etc? > > > Just do dequeue/enqueue from multiple threads and check data integrity at the end? > > > If not - what makes you think that the problem is precisely in rte_ring code? > > > Why not in rte_mbuf let say? > > Actually I reread your original mail and finally get your point. > > If I understand you correctly the problem with read reordering here is that > > after we read prot.tail but before we read cons.head > > both cons.head and prod.tail might be updated, > > but for us prod.tail change might be unnoticed. > > As an example: > > time 0 (cons.head == 0, prod.tail == 0): > > prod_tail = r->prod.tail; /* due read reordering */ > > /* prod_tail == 0 */ > > > > time 1 (cons.head ==5, prod.tail == 5): > > *old_head = r->cons.head; > > /* cons.head == 5 */ > > *entries = (prod_tail - *old_head); > > /* *entries == (0 - 5) == 0xFFFFFFFB */ > > > > And we will move our cons.head forward, even though there are no filled entries in the ring. > > Is that right? > Yes > > As I side notice, I wonder why we have here: > > *entries = (prod_tail - *old_head); > > instead of: > > *entries = r->size + prod_tail - *old_head; > > ? > Yes, I agree with you at this code line. > But reordering will still mess up things even after this change(I have > tested, still the same as before) > I thought the *entries is a door to prevent consumer from moving forward too > fast than the producer. > But in some cases, it is correct that prod_tail is smaller than *old_head > due to the cirular queue. > In other cases, it is incorrect because of read/read reordering. > > AFAICT, the root cause here is the producer tail and cosumer head are > dependant on each other. > Thus a memory barrier is neccessary. Yes. The barrier is necessary. In fact, upstream freebsd fixed this issue for arm64. DPDK ring implementation is derived from freebsd's buf_ring.h. https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 I think, the only outstanding issue is, how to reduce the performance impact for arm64. I believe using accurate/release semantics instead of rte_smp_rmb() will reduce the performance overhead like similar ring implementations below, freebsd: https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 odp: https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c Jia, 1) Can you verify the use of accurate/release semantics fixes the problem in your platform? like use of atomic_load_acq* in the reference code. 2) If so, What is the overhead between accurate/release and plane smp_smb() barriers. Based on that we need decide what path to take. Note: This issue wont come in all the arm64 implementation. it comes on arm64 implementation with OOO(out of order) implementations. > > Cheers, > Jia > > > > > Konstantin > > > > > > . In another > > > > mail of this thread, we've made a simple test based on this and captured > > > > some information and I pasted there.(I pasted the patch there :-)) > > > Are you talking about that one: > > > http://dpdk.org/dev/patchwork/patch/30405/ > > > ? > > > It still uses test/test/test_mbuf.c..., > > > but anyway I don't really understand how mbuf_autotest supposed > > > to work with these changes: > > > @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, > > > rte_ring_enqueue(refcnt_mbuf_ring, m); > > > } > > > } > > > - rte_pktmbuf_free(m); > > > + // rte_pktmbuf_free(m); > > > } > > > @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, > > > while (!rte_ring_empty(refcnt_mbuf_ring)) > > > ; > > > > > > + if (NULL != m) { > > > + if (1 != rte_mbuf_refcnt_read(m)) > > > + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); > > > + rte_pktmbuf_free(m); > > > + } > > > + > > > /* check that all mbufs are back into mempool by now */ > > > for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { > > > if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { > > > > > > That means all your mbufs (except the last one) will still be allocated. > > > So the test would fail - as it should, I think. > > > > > > > And > > > > it seems that Juhamatti & Jacod found some reverting action several > > > > months ago. > > > Didn't get that one either. > > > Konstantin > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-20 5:43 ` Jerin Jacob @ 2017-10-23 8:49 ` Jia He 2017-10-23 9:05 ` Kuusisaari, Juhamatti 2017-10-23 10:06 ` Jerin Jacob 0 siblings, 2 replies; 41+ messages in thread From: Jia He @ 2017-10-23 8:49 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce Hi Jerin On 10/20/2017 1:43 PM, Jerin Jacob Wrote: > -----Original Message----- >> [...] >> dependant on each other. >> Thus a memory barrier is neccessary. > Yes. The barrier is necessary. > In fact, upstream freebsd fixed this issue for arm64. DPDK ring > implementation is derived from freebsd's buf_ring.h. > https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > I think, the only outstanding issue is, how to reduce the performance > impact for arm64. I believe using accurate/release semantics instead > of rte_smp_rmb() will reduce the performance overhead like similar ring implementations below, > freebsd: https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > odp: https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c > > Jia, > 1) Can you verify the use of accurate/release semantics fixes the problem in your > platform? like use of atomic_load_acq* in the reference code. > 2) If so, What is the overhead between accurate/release and plane smp_smb() > barriers. Based on that we need decide what path to take. I've tested 3 cases. The new 3rd case is to use the load_acquire barrier (half barrier) you mentioned at above link. The patch seems like: @@ -408,8 +466,8 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, /* Reset n to the initial burst count */ n = max; - *old_head = r->prod.head; - const uint32_t cons_tail = r->cons.tail; + *old_head = atomic_load_acq_32(&r->prod.head); + const uint32_t cons_tail = atomic_load_acq_32(&r->cons.tail); @@ -516,14 +576,15 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_s /* Restore n as it may change every loop */ n = max; - *old_head = r->cons.head; - const uint32_t prod_tail = r->prod.tail; + *old_head = atomic_load_acq_32(&r->cons.head); + const uint32_t prod_tail = atomic_load_acq_32(&r->prod.tail) /* The subtraction is done between two unsigned 32bits value * (the result is always modulo 32 bits even if we have * cons_head > prod_tail). So 'entries' is always between 0 * and size(ring)-1. */ The half barrier patch passed the fuctional test. As for the performance comparision on *arm64*(the debug patch is at http://dpdk.org/ml/archives/dev/2017-October/079012.html), please see the test results below: [case 1] old codes, no barrier ============================================ Performance counter stats for './test --no-huge -l 1-10': 689275.001200 task-clock (msec) # 9.771 CPUs utilized 6223 context-switches # 0.009 K/sec 10 cpu-migrations # 0.000 K/sec 653 page-faults # 0.001 K/sec 1721190914583 cycles # 2.497 GHz 3363238266430 instructions # 1.95 insn per cycle <not supported> branches 27804740 branch-misses # 0.00% of all branches 70.540618825 seconds time elapsed [case 2] full barrier with rte_smp_rmb() ============================================ Performance counter stats for './test --no-huge -l 1-10': 582557.895850 task-clock (msec) # 9.752 CPUs utilized 5242 context-switches # 0.009 K/sec 10 cpu-migrations # 0.000 K/sec 665 page-faults # 0.001 K/sec 1454360730055 cycles # 2.497 GHz 587197839907 instructions # 0.40 insn per cycle <not supported> branches 27799687 branch-misses # 0.00% of all branches 59.735582356 seconds time elapse [case 1] half barrier with load_acquire ============================================ Performance counter stats for './test --no-huge -l 1-10': 660758.877050 task-clock (msec) # 9.764 CPUs utilized 5982 context-switches # 0.009 K/sec 11 cpu-migrations # 0.000 K/sec 657 page-faults # 0.001 K/sec 1649875318044 cycles # 2.497 GHz 591583257765 instructions # 0.36 insn per cycle <not supported> branches 27994903 branch-misses # 0.00% of all branches 67.672855107 seconds time elapsed Please see the context-switches in the perf results test result sorted by time is: full barrier < half barrier < no barrier AFAICT, in this case ,the cpu reordering will add the possibility for context switching and increase the running time. Any ideas? Cheers, Jia > > Note: > This issue wont come in all the arm64 implementation. it comes on arm64 > implementation with OOO(out of order) implementations. > > >> Cheers, >> Jia >> >>> Konstantin >>> >>>>> . In another >>>>> mail of this thread, we've made a simple test based on this and captured >>>>> some information and I pasted there.(I pasted the patch there :-)) >>>> Are you talking about that one: >>>> http://dpdk.org/dev/patchwork/patch/30405/ >>>> ? >>>> It still uses test/test/test_mbuf.c..., >>>> but anyway I don't really understand how mbuf_autotest supposed >>>> to work with these changes: >>>> @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, >>>> rte_ring_enqueue(refcnt_mbuf_ring, m); >>>> } >>>> } >>>> - rte_pktmbuf_free(m); >>>> + // rte_pktmbuf_free(m); >>>> } >>>> @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, >>>> while (!rte_ring_empty(refcnt_mbuf_ring)) >>>> ; >>>> >>>> + if (NULL != m) { >>>> + if (1 != rte_mbuf_refcnt_read(m)) >>>> + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); >>>> + rte_pktmbuf_free(m); >>>> + } >>>> + >>>> /* check that all mbufs are back into mempool by now */ >>>> for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { >>>> if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { >>>> >>>> That means all your mbufs (except the last one) will still be allocated. >>>> So the test would fail - as it should, I think. >>>> >>>>> And >>>>> it seems that Juhamatti & Jacod found some reverting action several >>>>> months ago. >>>> Didn't get that one either. >>>> Konstantin -- Cheers, Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-23 8:49 ` Jia He @ 2017-10-23 9:05 ` Kuusisaari, Juhamatti 2017-10-23 9:10 ` Bruce Richardson 2017-10-23 10:06 ` Jerin Jacob 1 sibling, 1 reply; 41+ messages in thread From: Kuusisaari, Juhamatti @ 2017-10-23 9:05 UTC (permalink / raw) To: Jia He, Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce Hi, > On 10/20/2017 1:43 PM, Jerin Jacob Wrote: > > -----Original Message----- > >> > [...] > >> dependant on each other. > >> Thus a memory barrier is neccessary. > > Yes. The barrier is necessary. > > In fact, upstream freebsd fixed this issue for arm64. DPDK ring > > implementation is derived from freebsd's buf_ring.h. > > > https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > > > I think, the only outstanding issue is, how to reduce the performance > > impact for arm64. I believe using accurate/release semantics instead > > of rte_smp_rmb() will reduce the performance overhead like similar > > ring implementations below, > > freebsd: > > > https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > odp: > > https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio > > /ring.c > > > > Jia, > > 1) Can you verify the use of accurate/release semantics fixes the > > problem in your platform? like use of atomic_load_acq* in the reference > code. > > 2) If so, What is the overhead between accurate/release and plane > > smp_smb() barriers. Based on that we need decide what path to take. > I've tested 3 cases. The new 3rd case is to use the load_acquire barrier (half > barrier) you mentioned at above link. > The patch seems like: > @@ -408,8 +466,8 @@ __rte_ring_move_prod_head(struct rte_ring *r, int > is_sp, > /* Reset n to the initial burst count */ > n = max; > > - *old_head = r->prod.head; > - const uint32_t cons_tail = r->cons.tail; > + *old_head = atomic_load_acq_32(&r->prod.head); > + const uint32_t cons_tail = > atomic_load_acq_32(&r->cons.tail); > > @@ -516,14 +576,15 @@ __rte_ring_move_cons_head(struct rte_ring *r, int > is_s > /* Restore n as it may change every loop */ > n = max; > > - *old_head = r->cons.head; > - const uint32_t prod_tail = r->prod.tail; > + *old_head = atomic_load_acq_32(&r->cons.head); > + const uint32_t prod_tail = atomic_load_acq_32(&r->prod.tail) > /* The subtraction is done between two unsigned 32bits > value > * (the result is always modulo 32 bits even if we have > * cons_head > prod_tail). So 'entries' is always between 0 > * and size(ring)-1. */ > > The half barrier patch passed the fuctional test. > > As for the performance comparision on *arm64*(the debug patch is at > http://dpdk.org/ml/archives/dev/2017-October/079012.html), please see > the test results > below: > > [case 1] old codes, no barrier > ============================================ > Performance counter stats for './test --no-huge -l 1-10': > > 689275.001200 task-clock (msec) # 9.771 CPUs utilized > 6223 context-switches # 0.009 K/sec > 10 cpu-migrations # 0.000 K/sec > 653 page-faults # 0.001 K/sec > 1721190914583 cycles # 2.497 GHz > 3363238266430 instructions # 1.95 insn per > cycle > <not supported> branches > 27804740 branch-misses # 0.00% of all > branches > > 70.540618825 seconds time elapsed > > [case 2] full barrier with rte_smp_rmb() > ============================================ > Performance counter stats for './test --no-huge -l 1-10': > > 582557.895850 task-clock (msec) # 9.752 CPUs utilized > 5242 context-switches # 0.009 K/sec > 10 cpu-migrations # 0.000 K/sec > 665 page-faults # 0.001 K/sec > 1454360730055 cycles # 2.497 GHz > 587197839907 instructions # 0.40 insn per > cycle > <not supported> branches > 27799687 branch-misses # 0.00% of all > branches > > 59.735582356 seconds time elapse > > [case 1] half barrier with load_acquire > ============================================ > Performance counter stats for './test --no-huge -l 1-10': > > 660758.877050 task-clock (msec) # 9.764 CPUs utilized > 5982 context-switches # 0.009 K/sec > 11 cpu-migrations # 0.000 K/sec > 657 page-faults # 0.001 K/sec > 1649875318044 cycles # 2.497 GHz > 591583257765 instructions # 0.36 insn per > cycle > <not supported> branches > 27994903 branch-misses # 0.00% of all > branches > > 67.672855107 seconds time elapsed > > Please see the context-switches in the perf results > test result sorted by time is: > full barrier < half barrier < no barrier > > AFAICT, in this case ,the cpu reordering will add the possibility for > context switching and > increase the running time. > > Any ideas? You could also try a case where you rearrange the rte_ring structure so that prod.head and cons.tail are parts of an union of a 64-bit variable and then read this 64-bit variable with one atomic read. I do not think that half barrier is even needed here with this approach, as long as you can really read the 64-bit variable fully at once. This should speed up. Cheers, -- Juhamatti > Cheers, > Jia > > > > > Note: > > This issue wont come in all the arm64 implementation. it comes on arm64 > > implementation with OOO(out of order) implementations. > > > > > >> Cheers, > >> Jia > >> > >>> Konstantin > >>> > >>>>> . In another > >>>>> mail of this thread, we've made a simple test based on this and > captured > >>>>> some information and I pasted there.(I pasted the patch there :-)) > >>>> Are you talking about that one: > >>>> http://dpdk.org/dev/patchwork/patch/30405/ > >>>> ? > >>>> It still uses test/test/test_mbuf.c..., > >>>> but anyway I don't really understand how mbuf_autotest supposed > >>>> to work with these changes: > >>>> @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int > iter, > >>>> rte_ring_enqueue(refcnt_mbuf_ring, m); > >>>> } > >>>> } > >>>> - rte_pktmbuf_free(m); > >>>> + // rte_pktmbuf_free(m); > >>>> } > >>>> @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned > int iter, > >>>> while (!rte_ring_empty(refcnt_mbuf_ring)) > >>>> ; > >>>> > >>>> + if (NULL != m) { > >>>> + if (1 != rte_mbuf_refcnt_read(m)) > >>>> + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); > >>>> + rte_pktmbuf_free(m); > >>>> + } > >>>> + > >>>> /* check that all mbufs are back into mempool by now */ > >>>> for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { > >>>> if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { > >>>> > >>>> That means all your mbufs (except the last one) will still be allocated. > >>>> So the test would fail - as it should, I think. > >>>> > >>>>> And > >>>>> it seems that Juhamatti & Jacod found some reverting action several > >>>>> months ago. > >>>> Didn't get that one either. > >>>> Konstantin > > -- > Cheers, > Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-23 9:05 ` Kuusisaari, Juhamatti @ 2017-10-23 9:10 ` Bruce Richardson 0 siblings, 0 replies; 41+ messages in thread From: Bruce Richardson @ 2017-10-23 9:10 UTC (permalink / raw) To: Kuusisaari, Juhamatti Cc: Jia He, Jerin Jacob, Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao On Mon, Oct 23, 2017 at 09:05:24AM +0000, Kuusisaari, Juhamatti wrote: > > Hi, > > > On 10/20/2017 1:43 PM, Jerin Jacob Wrote: > > > -----Original Message----- > > >> > > [...] > > >> dependant on each other. Thus a memory barrier is neccessary. > > > Yes. The barrier is necessary. In fact, upstream freebsd fixed > > > this issue for arm64. DPDK ring implementation is derived from > > > freebsd's buf_ring.h. > > > > > https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > > > > > I think, the only outstanding issue is, how to reduce the > > > performance impact for arm64. I believe using accurate/release > > > semantics instead of rte_smp_rmb() will reduce the performance > > > overhead like similar ring implementations below, freebsd: > > > > > https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > > odp: > > > https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio > > > /ring.c > > > > > > Jia, 1) Can you verify the use of accurate/release semantics fixes > > > the problem in your platform? like use of atomic_load_acq* in the > > > reference > > code. > > > 2) If so, What is the overhead between accurate/release and plane > > > smp_smb() barriers. Based on that we need decide what path to > > > take. > > I've tested 3 cases. The new 3rd case is to use the load_acquire > > barrier (half barrier) you mentioned at above link. The patch seems > > like: @@ -408,8 +466,8 @@ __rte_ring_move_prod_head(struct rte_ring > > *r, int is_sp, /* Reset n to the initial burst count > > */ n = max; > > > > - *old_head = r->prod.head; - const > > uint32_t cons_tail = r->cons.tail; + *old_head = > > atomic_load_acq_32(&r->prod.head); + const uint32_t > > cons_tail = atomic_load_acq_32(&r->cons.tail); > > > > @@ -516,14 +576,15 @@ __rte_ring_move_cons_head(struct rte_ring *r, > > int is_s /* Restore n as it may change every loop */ > > n = max; > > > > - *old_head = r->cons.head; - const > > uint32_t prod_tail = r->prod.tail; + *old_head = > > atomic_load_acq_32(&r->cons.head); + const uint32_t > > prod_tail = atomic_load_acq_32(&r->prod.tail) /* The > > subtraction is done between two unsigned 32bits value > > * (the result is always modulo 32 bits even if we > > have * cons_head > prod_tail). So 'entries' is > > always between 0 * and size(ring)-1. */ > > > > The half barrier patch passed the fuctional test. > > > > As for the performance comparision on *arm64*(the debug patch is at > > http://dpdk.org/ml/archives/dev/2017-October/079012.html), please > > see the test results below: > > > > [case 1] old codes, no barrier > > ============================================ Performance counter > > stats for './test --no-huge -l 1-10': > > > > 689275.001200 task-clock (msec) # 9.771 CPUs > > utilized 6223 context-switches # > > 0.009 K/sec 10 cpu-migrations # > > 0.000 K/sec 653 page-faults # > > 0.001 K/sec 1721190914583 cycles # > > 2.497 GHz 3363238266430 instructions # > > 1.95 insn per cycle <not supported> branches > > 27804740 branch-misses # 0.00% of all branches > > > > 70.540618825 seconds time elapsed > > > > [case 2] full barrier with rte_smp_rmb() > > ============================================ Performance counter > > stats for './test --no-huge -l 1-10': > > > > 582557.895850 task-clock (msec) # 9.752 CPUs > > utilized 5242 context-switches # > > 0.009 K/sec 10 cpu-migrations # > > 0.000 K/sec 665 page-faults # > > 0.001 K/sec 1454360730055 cycles # > > 2.497 GHz 587197839907 instructions # > > 0.40 insn per cycle <not supported> branches > > 27799687 branch-misses # 0.00% of all branches > > > > 59.735582356 seconds time elapse > > > > [case 1] half barrier with load_acquire > > ============================================ Performance counter > > stats for './test --no-huge -l 1-10': > > > > 660758.877050 task-clock (msec) # 9.764 CPUs > > utilized 5982 context-switches # > > 0.009 K/sec 11 cpu-migrations # > > 0.000 K/sec 657 page-faults # > > 0.001 K/sec 1649875318044 cycles # > > 2.497 GHz 591583257765 instructions # > > 0.36 insn per cycle <not supported> branches > > 27994903 branch-misses # 0.00% of all branches > > > > 67.672855107 seconds time elapsed > > > > Please see the context-switches in the perf results test result > > sorted by time is: full barrier < half barrier < no barrier > > > > AFAICT, in this case ,the cpu reordering will add the possibility > > for context switching and increase the running time. > > > > Any ideas? > > You could also try a case where you rearrange the rte_ring structure > so that prod.head and cons.tail are parts of an union of a 64-bit > variable and then read this 64-bit variable with one atomic read. I do > not think that half barrier is even needed here with this approach, as > long as you can really read the 64-bit variable fully at once. This > should speed up. > > Cheers, -- Yes, that would work, but unfortunately it means mixing producer and consumer data onto the same cacheline, which may have other negative effects on performance (depending on arch/platform) due to cores invalidating each other's cachelines. /Bruce ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-23 8:49 ` Jia He 2017-10-23 9:05 ` Kuusisaari, Juhamatti @ 2017-10-23 10:06 ` Jerin Jacob 2017-10-24 2:04 ` Jia He 1 sibling, 1 reply; 41+ messages in thread From: Jerin Jacob @ 2017-10-23 10:06 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce -----Original Message----- > Date: Mon, 23 Oct 2017 16:49:01 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > <bruce.richardson@intel.com> > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > Hi Jerin > > > On 10/20/2017 1:43 PM, Jerin Jacob Wrote: > > -----Original Message----- > > > > [...] > > > dependant on each other. > > > Thus a memory barrier is neccessary. > > Yes. The barrier is necessary. > > In fact, upstream freebsd fixed this issue for arm64. DPDK ring > > implementation is derived from freebsd's buf_ring.h. > > https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > > > I think, the only outstanding issue is, how to reduce the performance > > impact for arm64. I believe using accurate/release semantics instead > > of rte_smp_rmb() will reduce the performance overhead like similar ring implementations below, > > freebsd: https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 > > odp: https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c > > > > Jia, > > 1) Can you verify the use of accurate/release semantics fixes the problem in your > > platform? like use of atomic_load_acq* in the reference code. > > 2) If so, What is the overhead between accurate/release and plane smp_smb() > > barriers. Based on that we need decide what path to take. > I've tested 3 cases. The new 3rd case is to use the load_acquire barrier > (half barrier) you mentioned > at above link. > The patch seems like: > @@ -408,8 +466,8 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > /* Reset n to the initial burst count */ > n = max; > > - *old_head = r->prod.head; > - const uint32_t cons_tail = r->cons.tail; > + *old_head = atomic_load_acq_32(&r->prod.head); > + const uint32_t cons_tail = > atomic_load_acq_32(&r->cons.tail); > > @@ -516,14 +576,15 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_s > /* Restore n as it may change every loop */ > n = max; > > - *old_head = r->cons.head; > - const uint32_t prod_tail = r->prod.tail; > + *old_head = atomic_load_acq_32(&r->cons.head); > + const uint32_t prod_tail = atomic_load_acq_32(&r->prod.tail) > /* The subtraction is done between two unsigned 32bits value > * (the result is always modulo 32 bits even if we have > * cons_head > prod_tail). So 'entries' is always between 0 > * and size(ring)-1. */ > > The half barrier patch passed the fuctional test. > > As for the performance comparision on *arm64*(the debug patch is at > http://dpdk.org/ml/archives/dev/2017-October/079012.html), please see the > test results > below: > > [case 1] old codes, no barrier > ============================================ > Performance counter stats for './test --no-huge -l 1-10': > > 689275.001200 task-clock (msec) # 9.771 CPUs utilized > 6223 context-switches # 0.009 K/sec > 10 cpu-migrations # 0.000 K/sec > 653 page-faults # 0.001 K/sec > 1721190914583 cycles # 2.497 GHz > 3363238266430 instructions # 1.95 insn per cycle > <not supported> branches > 27804740 branch-misses # 0.00% of all branches > > 70.540618825 seconds time elapsed > > [case 2] full barrier with rte_smp_rmb() > ============================================ > Performance counter stats for './test --no-huge -l 1-10': > > 582557.895850 task-clock (msec) # 9.752 CPUs utilized > 5242 context-switches # 0.009 K/sec > 10 cpu-migrations # 0.000 K/sec > 665 page-faults # 0.001 K/sec > 1454360730055 cycles # 2.497 GHz > 587197839907 instructions # 0.40 insn per cycle > <not supported> branches > 27799687 branch-misses # 0.00% of all branches > > 59.735582356 seconds time elapse > > [case 1] half barrier with load_acquire > ============================================ > Performance counter stats for './test --no-huge -l 1-10': > > 660758.877050 task-clock (msec) # 9.764 CPUs utilized > 5982 context-switches # 0.009 K/sec > 11 cpu-migrations # 0.000 K/sec > 657 page-faults # 0.001 K/sec > 1649875318044 cycles # 2.497 GHz > 591583257765 instructions # 0.36 insn per cycle > <not supported> branches > 27994903 branch-misses # 0.00% of all branches > > 67.672855107 seconds time elapsed > > Please see the context-switches in the perf results > test result sorted by time is: > full barrier < half barrier < no barrier > > AFAICT, in this case ,the cpu reordering will add the possibility for > context switching and > increase the running time. > Any ideas? Regarding performance test, it better to use ring perf test case on _isolated_ cores to measure impact on number of enqueue/dequeue operations. example: ./build/app/test -c 0xff -n 4 >>ring_perf_autotest By default, arm64+dpdk will be using el0 counter to measure the cycles. I think, in your SoC, it will be running at 50MHz or 100MHz.So, You can follow the below scheme to get accurate cycle measurement scheme: See: http://dpdk.org/doc/guides/prog_guide/profile_app.html check: 44.2.2. High-resolution cycle counter ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-23 10:06 ` Jerin Jacob @ 2017-10-24 2:04 ` Jia He 2017-10-25 13:26 ` Jerin Jacob 0 siblings, 1 reply; 41+ messages in thread From: Jia He @ 2017-10-24 2:04 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce Hi Jerin On 10/23/2017 6:06 PM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Mon, 23 Oct 2017 16:49:01 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >> <bruce.richardson@intel.com> >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >> loading when doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.4.0 >> >> Hi Jerin >> >> >> On 10/20/2017 1:43 PM, Jerin Jacob Wrote: >>> -----Original Message----- >> [...] >>>> dependant on each other. >>>> Thus a memory barrier is neccessary. >>> Yes. The barrier is necessary. >>> In fact, upstream freebsd fixed this issue for arm64. DPDK ring >>> implementation is derived from freebsd's buf_ring.h. >>> https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 >>> >>> I think, the only outstanding issue is, how to reduce the performance >>> impact for arm64. I believe using accurate/release semantics instead >>> of rte_smp_rmb() will reduce the performance overhead like similar ring implementations below, >>> freebsd: https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L166 >>> odp: https://github.com/Linaro/odp/blob/master/platform/linux-generic/pktio/ring.c >>> >>> Jia, >>> 1) Can you verify the use of accurate/release semantics fixes the problem in your >>> platform? like use of atomic_load_acq* in the reference code. >>> 2) If so, What is the overhead between accurate/release and plane smp_smb() >>> barriers. Based on that we need decide what path to take. >> I've tested 3 cases. The new 3rd case is to use the load_acquire barrier >> (half barrier) you mentioned >> at above link. >> The patch seems like: >> @@ -408,8 +466,8 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, >> /* Reset n to the initial burst count */ >> n = max; >> >> - *old_head = r->prod.head; >> - const uint32_t cons_tail = r->cons.tail; >> + *old_head = atomic_load_acq_32(&r->prod.head); >> + const uint32_t cons_tail = >> atomic_load_acq_32(&r->cons.tail); >> >> @@ -516,14 +576,15 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_s >> /* Restore n as it may change every loop */ >> n = max; >> >> - *old_head = r->cons.head; >> - const uint32_t prod_tail = r->prod.tail; >> + *old_head = atomic_load_acq_32(&r->cons.head); >> + const uint32_t prod_tail = atomic_load_acq_32(&r->prod.tail) >> /* The subtraction is done between two unsigned 32bits value >> * (the result is always modulo 32 bits even if we have >> * cons_head > prod_tail). So 'entries' is always between 0 >> * and size(ring)-1. */ >> >> The half barrier patch passed the fuctional test. >> >> As for the performance comparision on *arm64*(the debug patch is at >> http://dpdk.org/ml/archives/dev/2017-October/079012.html), please see the >> test results >> below: >> >> [case 1] old codes, no barrier >> ============================================ >> Performance counter stats for './test --no-huge -l 1-10': >> >> 689275.001200 task-clock (msec) # 9.771 CPUs utilized >> 6223 context-switches # 0.009 K/sec >> 10 cpu-migrations # 0.000 K/sec >> 653 page-faults # 0.001 K/sec >> 1721190914583 cycles # 2.497 GHz >> 3363238266430 instructions # 1.95 insn per cycle >> <not supported> branches >> 27804740 branch-misses # 0.00% of all branches >> >> 70.540618825 seconds time elapsed >> >> [case 2] full barrier with rte_smp_rmb() >> ============================================ >> Performance counter stats for './test --no-huge -l 1-10': >> >> 582557.895850 task-clock (msec) # 9.752 CPUs utilized >> 5242 context-switches # 0.009 K/sec >> 10 cpu-migrations # 0.000 K/sec >> 665 page-faults # 0.001 K/sec >> 1454360730055 cycles # 2.497 GHz >> 587197839907 instructions # 0.40 insn per cycle >> <not supported> branches >> 27799687 branch-misses # 0.00% of all branches >> >> 59.735582356 seconds time elapse >> >> [case 1] half barrier with load_acquire >> ============================================ >> Performance counter stats for './test --no-huge -l 1-10': >> >> 660758.877050 task-clock (msec) # 9.764 CPUs utilized >> 5982 context-switches # 0.009 K/sec >> 11 cpu-migrations # 0.000 K/sec >> 657 page-faults # 0.001 K/sec >> 1649875318044 cycles # 2.497 GHz >> 591583257765 instructions # 0.36 insn per cycle >> <not supported> branches >> 27994903 branch-misses # 0.00% of all branches >> >> 67.672855107 seconds time elapsed >> >> Please see the context-switches in the perf results >> test result sorted by time is: >> full barrier < half barrier < no barrier >> >> AFAICT, in this case ,the cpu reordering will add the possibility for >> context switching and >> increase the running time. >> Any ideas? > Regarding performance test, it better to use ring perf test case > on _isolated_ cores to measure impact on number of enqueue/dequeue operations. > > example: > ./build/app/test -c 0xff -n 4 >>> ring_perf_autotest Seem in our arm64 server, the ring_perf_autotest will be finished in a few seconds: Anything wrong about configuration or environment setup? root@ubuntu:/home/hj/dpdk/build/build/test/test# ./test -c 0xff -n 4 EAL: Detected 44 lcore(s) EAL: Probing VFIO support... APP: HPET is not enabled, using TSC as default timer RTE>>per_lcore_autotest RTE>>ring_perf_autotest ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 0 MP/MC single enq/dequeue: 2 SP/SC burst enq/dequeue (size: 8): 0 MP/MC burst enq/dequeue (size: 8): 0 SP/SC burst enq/dequeue (size: 32): 0 MP/MC burst enq/dequeue (size: 32): 0 ### Testing empty dequeue ### SC empty dequeue: 0.02 MC empty dequeue: 0.04 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 0.12 MP/MC bulk enq/dequeue (size: 8): 0.31 SP/SC bulk enq/dequeue (size: 32): 0.05 MP/MC bulk enq/dequeue (size: 32): 0.09 ### Testing using two hyperthreads ### SP/SC bulk enq/dequeue (size: 8): 0.12 MP/MC bulk enq/dequeue (size: 8): 0.39 SP/SC bulk enq/dequeue (size: 32): 0.04 MP/MC bulk enq/dequeue (size: 32): 0.12 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 0.37 MP/MC bulk enq/dequeue (size: 8): 0.92 SP/SC bulk enq/dequeue (size: 32): 0.12 MP/MC bulk enq/dequeue (size: 32): 0.26 Test OK RTE>> Cheers, Jia > By default, arm64+dpdk will be using el0 counter to measure the cycles. I > think, in your SoC, it will be running at 50MHz or 100MHz.So, You can > follow the below scheme to get accurate cycle measurement scheme: > > See: http://dpdk.org/doc/guides/prog_guide/profile_app.html > check: 44.2.2. High-resolution cycle counter ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-24 2:04 ` Jia He @ 2017-10-25 13:26 ` Jerin Jacob 2017-10-26 2:27 ` Jia He 2017-10-31 2:55 ` Jia He 0 siblings, 2 replies; 41+ messages in thread From: Jerin Jacob @ 2017-10-25 13:26 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce -----Original Message----- > Date: Tue, 24 Oct 2017 10:04:26 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > <bruce.richardson@intel.com> > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > Hi Jerin Hi Jia, > > example: > > ./build/app/test -c 0xff -n 4 > > > > ring_perf_autotest > Seem in our arm64 server, the ring_perf_autotest will be finished in a few > seconds: Yes. It just need a few seconds. > Anything wrong about configuration or environment setup? By default, arm64+dpdk will be using el0 counter to measure the cycles. I think, in your SoC, it will be running at 50MHz or 100MHz.So, You can follow the below scheme to get accurate cycle measurement scheme: See: http://dpdk.org/doc/guides/prog_guide/profile_app.html check: 44.2.2. High-resolution cycle counter > > root@ubuntu:/home/hj/dpdk/build/build/test/test# ./test -c 0xff -n 4 > EAL: Detected 44 lcore(s) > EAL: Probing VFIO support... > APP: HPET is not enabled, using TSC as default timer > RTE>>per_lcore_autotest > RTE>>ring_perf_autotest > ### Testing single element and burst enq/deq ### > SP/SC single enq/dequeue: 0 > MP/MC single enq/dequeue: 2 > SP/SC burst enq/dequeue (size: 8): 0 If you follow the above link, The value '0' will be replaced with more meaning full data. > MP/MC burst enq/dequeue (size: 8): 0 > SP/SC burst enq/dequeue (size: 32): 0 > MP/MC burst enq/dequeue (size: 32): 0 > > ### Testing empty dequeue ### > SC empty dequeue: 0.02 > MC empty dequeue: 0.04 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 0.12 > MP/MC bulk enq/dequeue (size: 8): 0.31 > SP/SC bulk enq/dequeue (size: 32): 0.05 > MP/MC bulk enq/dequeue (size: 32): 0.09 > > ### Testing using two hyperthreads ### > SP/SC bulk enq/dequeue (size: 8): 0.12 > MP/MC bulk enq/dequeue (size: 8): 0.39 > SP/SC bulk enq/dequeue (size: 32): 0.04 > MP/MC bulk enq/dequeue (size: 32): 0.12 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 0.37 > MP/MC bulk enq/dequeue (size: 8): 0.92 > SP/SC bulk enq/dequeue (size: 32): 0.12 > MP/MC bulk enq/dequeue (size: 32): 0.26 > Test OK > RTE>> > > Cheers, > Jia > > By default, arm64+dpdk will be using el0 counter to measure the cycles. I > > think, in your SoC, it will be running at 50MHz or 100MHz.So, You can > > follow the below scheme to get accurate cycle measurement scheme: > > > > See: http://dpdk.org/doc/guides/prog_guide/profile_app.html > > check: 44.2.2. High-resolution cycle counter > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-25 13:26 ` Jerin Jacob @ 2017-10-26 2:27 ` Jia He 2017-10-31 2:55 ` Jia He 1 sibling, 0 replies; 41+ messages in thread From: Jia He @ 2017-10-26 2:27 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce Hi Jerin On 10/25/2017 9:26 PM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Tue, 24 Oct 2017 10:04:26 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >> <bruce.richardson@intel.com> >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >> loading when doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.4.0 >> >> Hi Jerin > Hi Jia, > > >>> example: >>> ./build/app/test -c 0xff -n 4 >>>>> ring_perf_autotest >> Seem in our arm64 server, the ring_perf_autotest will be finished in a few >> seconds: > Yes. It just need a few seconds. > >> Anything wrong about configuration or environment setup? > By default, arm64+dpdk will be using el0 counter to measure the cycles. I > think, in your SoC, it will be running at 50MHz or 100MHz.So, You can > follow the below scheme to get accurate cycle measurement scheme: > > See: http://dpdk.org/doc/guides/prog_guide/profile_app.html > check: 44.2.2. High-resolution cycle counter Thank you for the suggestions. But I tried your provided ko module to enable the accurate cycle measurement in user space, the test result is as below: root@nfv-demo01:~/dpdk/build/build/test/test# lsmod |grep pmu pmu_el0_cycle_counter 262144 0 [old codes, without any patches] ============================================ RTE>>ring_perf_autotest ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 0 MP/MC single enq/dequeue: 0 SP/SC burst enq/dequeue (size: 8): 0 MP/MC burst enq/dequeue (size: 8): 0 SP/SC burst enq/dequeue (size: 32): 0 MP/MC burst enq/dequeue (size: 32): 0 ### Testing empty dequeue ### SC empty dequeue: 0.00 MC empty dequeue: 0.00 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 0.00 MP/MC bulk enq/dequeue (size: 8): 0.00 SP/SC bulk enq/dequeue (size: 32): 0.00 MP/MC bulk enq/dequeue (size: 32): 0.00 ### Testing using two hyperthreads ### SP/SC bulk enq/dequeue (size: 8): 0.00 MP/MC bulk enq/dequeue (size: 8): 0.00 SP/SC bulk enq/dequeue (size: 32): 0.00 MP/MC bulk enq/dequeue (size: 32): 0.00 Test OK [with full rte_smp_rmb barrier patch] ====================================== RTE>>ring_perf_autotest ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 0 MP/MC single enq/dequeue: 0 SP/SC burst enq/dequeue (size: 8): 0 MP/MC burst enq/dequeue (size: 8): 0 SP/SC burst enq/dequeue (size: 32): 0 MP/MC burst enq/dequeue (size: 32): 0 ### Testing empty dequeue ### SC empty dequeue: 0.00 MC empty dequeue: 0.00 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 0.00 MP/MC bulk enq/dequeue (size: 8): 0.00 SP/SC bulk enq/dequeue (size: 32): 0.00 MP/MC bulk enq/dequeue (size: 32): 0.00 ### Testing using two hyperthreads ### SP/SC bulk enq/dequeue (size: 8): 0.00 MP/MC bulk enq/dequeue (size: 8): 0.00 SP/SC bulk enq/dequeue (size: 32): 0.00 MP/MC bulk enq/dequeue (size: 32): 0.00 Test OK RTE>> No difference，all time is 0 ? If I rmmod pmu_el0_cycle_counter and revise the ./build/.config to comment the config line #CONFIG_RTE_ARM_EAL_RDTSC_USE_PMU=y Then the time is bigger than 0 >> root@ubuntu:/home/hj/dpdk/build/build/test/test# ./test -c 0xff -n 4 >> EAL: Detected 44 lcore(s) >> EAL: Probing VFIO support... >> APP: HPET is not enabled, using TSC as default timer >> RTE>>per_lcore_autotest >> RTE>>ring_perf_autotest >> ### Testing single element and burst enq/deq ### >> SP/SC single enq/dequeue: 0 >> MP/MC single enq/dequeue: 2 >> SP/SC burst enq/dequeue (size: 8): 0 > If you follow the above link, The value '0' will be replaced with more meaning full data. > >> MP/MC burst enq/dequeue (size: 8): 0 >> SP/SC burst enq/dequeue (size: 32): 0 >> MP/MC burst enq/dequeue (size: 32): 0 >> >> ### Testing empty dequeue ### >> SC empty dequeue: 0.02 >> MC empty dequeue: 0.04 >> >> ### Testing using a single lcore ### >> SP/SC bulk enq/dequeue (size: 8): 0.12 >> MP/MC bulk enq/dequeue (size: 8): 0.31 >> SP/SC bulk enq/dequeue (size: 32): 0.05 >> MP/MC bulk enq/dequeue (size: 32): 0.09 >> >> ### Testing using two hyperthreads ### >> SP/SC bulk enq/dequeue (size: 8): 0.12 >> MP/MC bulk enq/dequeue (size: 8): 0.39 >> SP/SC bulk enq/dequeue (size: 32): 0.04 >> MP/MC bulk enq/dequeue (size: 32): 0.12 >> >> ### Testing using two physical cores ### >> SP/SC bulk enq/dequeue (size: 8): 0.37 >> MP/MC bulk enq/dequeue (size: 8): 0.92 >> SP/SC bulk enq/dequeue (size: 32): 0.12 >> MP/MC bulk enq/dequeue (size: 32): 0.26 >> Test OK >> RTE>> >> >> Cheers, >> Jia >>> By default, arm64+dpdk will be using el0 counter to measure the cycles. I >>> think, in your SoC, it will be running at 50MHz or 100MHz.So, You can >>> follow the below scheme to get accurate cycle measurement scheme: >>> >>> See: http://dpdk.org/doc/guides/prog_guide/profile_app.html >>> check: 44.2.2. High-resolution cycle counter -- Cheers, Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-25 13:26 ` Jerin Jacob 2017-10-26 2:27 ` Jia He @ 2017-10-31 2:55 ` Jia He 2017-10-31 11:14 ` Jerin Jacob 1 sibling, 1 reply; 41+ messages in thread From: Jia He @ 2017-10-31 2:55 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce Hi Jerin Do you think next step whether I need to implement the load_acquire half barrier as per freebsd or find any other performance test case to compare the performance impact? Thanks for any suggestions. Cheers, Jia On 10/25/2017 9:26 PM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Tue, 24 Oct 2017 10:04:26 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >> <bruce.richardson@intel.com> >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >> loading when doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.4.0 >> >> Hi Jerin > Hi Jia, > > >>> example: >>> ./build/app/test -c 0xff -n 4 >>>>> ring_perf_autotest >> Seem in our arm64 server, the ring_perf_autotest will be finished in a few >> seconds: > Yes. It just need a few seconds. > >> Anything wrong about configuration or environment setup? > By default, arm64+dpdk will be using el0 counter to measure the cycles. I > think, in your SoC, it will be running at 50MHz or 100MHz.So, You can > follow the below scheme to get accurate cycle measurement scheme: > > See: http://dpdk.org/doc/guides/prog_guide/profile_app.html > check: 44.2.2. High-resolution cycle counter > >> root@ubuntu:/home/hj/dpdk/build/build/test/test# ./test -c 0xff -n 4 >> EAL: Detected 44 lcore(s) >> EAL: Probing VFIO support... >> APP: HPET is not enabled, using TSC as default timer >> RTE>>per_lcore_autotest >> RTE>>ring_perf_autotest >> ### Testing single element and burst enq/deq ### >> SP/SC single enq/dequeue: 0 >> MP/MC single enq/dequeue: 2 >> SP/SC burst enq/dequeue (size: 8): 0 > If you follow the above link, The value '0' will be replaced with more meaning full data. > >> MP/MC burst enq/dequeue (size: 8): 0 >> SP/SC burst enq/dequeue (size: 32): 0 >> MP/MC burst enq/dequeue (size: 32): 0 >> >> ### Testing empty dequeue ### >> SC empty dequeue: 0.02 >> MC empty dequeue: 0.04 >> >> ### Testing using a single lcore ### >> SP/SC bulk enq/dequeue (size: 8): 0.12 >> MP/MC bulk enq/dequeue (size: 8): 0.31 >> SP/SC bulk enq/dequeue (size: 32): 0.05 >> MP/MC bulk enq/dequeue (size: 32): 0.09 >> >> ### Testing using two hyperthreads ### >> SP/SC bulk enq/dequeue (size: 8): 0.12 >> MP/MC bulk enq/dequeue (size: 8): 0.39 >> SP/SC bulk enq/dequeue (size: 32): 0.04 >> MP/MC bulk enq/dequeue (size: 32): 0.12 >> >> ### Testing using two physical cores ### >> SP/SC bulk enq/dequeue (size: 8): 0.37 >> MP/MC bulk enq/dequeue (size: 8): 0.92 >> SP/SC bulk enq/dequeue (size: 32): 0.12 >> MP/MC bulk enq/dequeue (size: 32): 0.26 >> Test OK >> RTE>> >> >> Cheers, >> Jia >>> By default, arm64+dpdk will be using el0 counter to measure the cycles. I >>> think, in your SoC, it will be running at 50MHz or 100MHz.So, You can >>> follow the below scheme to get accurate cycle measurement scheme: >>> >>> See: http://dpdk.org/doc/guides/prog_guide/profile_app.html >>> check: 44.2.2. High-resolution cycle counter -- Cheers, Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-31 2:55 ` Jia He @ 2017-10-31 11:14 ` Jerin Jacob 2017-11-01 2:53 ` Jia He 2017-11-01 4:48 ` Jia He 0 siblings, 2 replies; 41+ messages in thread From: Jerin Jacob @ 2017-10-31 11:14 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal -----Original Message----- > Date: Tue, 31 Oct 2017 10:55:15 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > <bruce.richardson@intel.com> > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > Hi Jerin Hi Jia, > > Do you think next step whether I need to implement the load_acquire half > barrier as per freebsd I did a quick prototype using C11 memory model(ACQUIRE/RELEASE) schematics and tested on two arm64 platform in Cavium(Platform A: Non arm64 OOO machine) and Platform B: arm64 OOO machine) smp_rmb() performs better in Platform A: acquire/release semantics perform better in platform B: Here is the patch: https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch In terms of next step: - I am not sure the cost associated with acquire/release semantics on x86 or ppc. IMO, We need to have both options under conditional compilation flags and let the target platform choose the best one. Thoughts? Here is the performance numbers: - Both platforms are running at different frequency, So absolute numbers does not matter, Just check the relative numbers. Platform A: Performance numbers: ================================ no patch(Non arm64 OOO machine) ------------------------------- SP/SC single enq/dequeue: 40 MP/MC single enq/dequeue: 282 SP/SC burst enq/dequeue (size: 8): 11 MP/MC burst enq/dequeue (size: 8): 42 SP/SC burst enq/dequeue (size: 32): 8 MP/MC burst enq/dequeue (size: 32): 16 ### Testing empty dequeue ### SC empty dequeue: 8.01 MC empty dequeue: 11.01 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 11.30 MP/MC bulk enq/dequeue (size: 8): 42.85 SP/SC bulk enq/dequeue (size: 32): 8.25 MP/MC bulk enq/dequeue (size: 32): 16.46 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 20.62 MP/MC bulk enq/dequeue (size: 8): 56.30 SP/SC bulk enq/dequeue (size: 32): 10.94 MP/MC bulk enq/dequeue (size: 32): 18.66 Test OK # smp_rmb() patch((Non OOO arm64 machine) http://dpdk.org/dev/patchwork/patch/30029/ ----------------------------------------- SP/SC single enq/dequeue: 42 MP/MC single enq/dequeue: 291 SP/SC burst enq/dequeue (size: 8): 12 MP/MC burst enq/dequeue (size: 8): 44 SP/SC burst enq/dequeue (size: 32): 8 MP/MC burst enq/dequeue (size: 32): 16 ### Testing empty dequeue ### SC empty dequeue: 13.01 MC empty dequeue: 15.01 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 11.60 MP/MC bulk enq/dequeue (size: 8): 44.32 SP/SC bulk enq/dequeue (size: 32): 8.60 MP/MC bulk enq/dequeue (size: 32): 16.50 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 20.95 MP/MC bulk enq/dequeue (size: 8): 56.90 SP/SC bulk enq/dequeue (size: 32): 10.90 MP/MC bulk enq/dequeue (size: 32): 18.78 Test OK RTE>> # c11 memory model patch((Non OOO arm64 machine) https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch --------------------------------------------------------------------------------------------- ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 197 MP/MC single enq/dequeue: 328 SP/SC burst enq/dequeue (size: 8): 31 MP/MC burst enq/dequeue (size: 8): 50 SP/SC burst enq/dequeue (size: 32): 13 MP/MC burst enq/dequeue (size: 32): 18 ### Testing empty dequeue ### SC empty dequeue: 13.01 MC empty dequeue: 18.02 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 30.95 MP/MC bulk enq/dequeue (size: 8): 50.30 SP/SC bulk enq/dequeue (size: 32): 13.27 MP/MC bulk enq/dequeue (size: 32): 18.11 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 43.38 MP/MC bulk enq/dequeue (size: 8): 64.42 SP/SC bulk enq/dequeue (size: 32): 16.71 MP/MC bulk enq/dequeue (size: 32): 22.21 Platform B: Performance numbers: ============================== #no patch(OOO arm64 machine) ---------------------------- ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 81 MP/MC single enq/dequeue: 207 SP/SC burst enq/dequeue (size: 8): 15 MP/MC burst enq/dequeue (size: 8): 31 SP/SC burst enq/dequeue (size: 32): 7 MP/MC burst enq/dequeue (size: 32): 11 ### Testing empty dequeue ### SC empty dequeue: 3.00 MC empty dequeue: 5.00 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 15.38 MP/MC bulk enq/dequeue (size: 8): 30.64 SP/SC bulk enq/dequeue (size: 32): 7.25 MP/MC bulk enq/dequeue (size: 32): 11.06 ### Testing using two hyperthreads ### SP/SC bulk enq/dequeue (size: 8): 31.51 MP/MC bulk enq/dequeue (size: 8): 49.38 SP/SC bulk enq/dequeue (size: 32): 14.32 MP/MC bulk enq/dequeue (size: 32): 15.89 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 72.66 MP/MC bulk enq/dequeue (size: 8): 121.89 SP/SC bulk enq/dequeue (size: 32): 16.88 MP/MC bulk enq/dequeue (size: 32): 24.23 Test OK RTE>> # smp_rmb() patch((OOO arm64 machine) http://dpdk.org/dev/patchwork/patch/30029/ ------------------------------------------- ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 152 MP/MC single enq/dequeue: 265 SP/SC burst enq/dequeue (size: 8): 24 MP/MC burst enq/dequeue (size: 8): 39 SP/SC burst enq/dequeue (size: 32): 9 MP/MC burst enq/dequeue (size: 32): 13 ### Testing empty dequeue ### SC empty dequeue: 31.01 MC empty dequeue: 32.01 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 24.26 MP/MC bulk enq/dequeue (size: 8): 39.52 SP/SC bulk enq/dequeue (size: 32): 9.47 MP/MC bulk enq/dequeue (size: 32): 13.31 ### Testing using two hyperthreads ### SP/SC bulk enq/dequeue (size: 8): 40.29 MP/MC bulk enq/dequeue (size: 8): 59.57 SP/SC bulk enq/dequeue (size: 32): 17.34 MP/MC bulk enq/dequeue (size: 32): 21.58 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 79.05 MP/MC bulk enq/dequeue (size: 8): 153.46 SP/SC bulk enq/dequeue (size: 32): 26.41 MP/MC bulk enq/dequeue (size: 32): 38.37 Test OK RTE>> # c11 memory model patch((OOO arm64 machine) https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch ---------------------------------------------------------------------------------------------- ### Testing single element and burst enq/deq ### SP/SC single enq/dequeue: 98 MP/MC single enq/dequeue: 130 SP/SC burst enq/dequeue (size: 8): 18 MP/MC burst enq/dequeue (size: 8): 22 SP/SC burst enq/dequeue (size: 32): 7 MP/MC burst enq/dequeue (size: 32): 9 ### Testing empty dequeue ### SC empty dequeue: 4.00 MC empty dequeue: 5.00 ### Testing using a single lcore ### SP/SC bulk enq/dequeue (size: 8): 17.40 MP/MC bulk enq/dequeue (size: 8): 22.88 SP/SC bulk enq/dequeue (size: 32): 7.62 MP/MC bulk enq/dequeue (size: 32): 8.96 ### Testing using two hyperthreads ### SP/SC bulk enq/dequeue (size: 8): 20.24 MP/MC bulk enq/dequeue (size: 8): 25.83 SP/SC bulk enq/dequeue (size: 32): 12.21 MP/MC bulk enq/dequeue (size: 32): 13.20 ### Testing using two physical cores ### SP/SC bulk enq/dequeue (size: 8): 67.54 MP/MC bulk enq/dequeue (size: 8): 124.63 SP/SC bulk enq/dequeue (size: 32): 21.13 MP/MC bulk enq/dequeue (size: 32): 28.44 Test OK RTE>>quit > or find any other performance test case to compare the performance impact? As far as I know, ring_perf_autotest is the better performance test. If you have trouble in using "High-resolution cycle counter" in your platform then also you can use ring_perf_auto test to compare the performance(as relative number matters) Jerin > Thanks for any suggestions. > > Cheers, > Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-31 11:14 ` Jerin Jacob @ 2017-11-01 2:53 ` Jia He 2017-11-01 19:04 ` Jerin Jacob 2017-11-01 4:48 ` Jia He 1 sibling, 1 reply; 41+ messages in thread From: Jia He @ 2017-11-01 2:53 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal Hi Jerin Thanks for your suggestions. I will try to use config macro to let it be chosen by user. I need to point out one possible issue in your load_acq/store_rel patch at https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch @@ -516,8 +541,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, /* Restore n as it may change every loop */ n = max; +#if 0 *old_head = r->cons.head; const uint32_t prod_tail = r->prod.tail; +#else + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_RELAXED); --[1] + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, __ATOMIC_ACQUIRE); --[2] +#endif line [1] __ATOMIC_RELAXED is not enough for this case(tested in our ARM64 server). line [2] __ATOMIC_ACQUIRE guarantee the 2nd load will not be reorded before the 1st load, but will not guarantee the 1st load will not be reordered after the 2nd load. Please also refer to your mentioned freebsd implementation. They use __ATOMIC_ACQUIRE at line [1]. Should it be like instead? +#else + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, __ATOMIC_ACQUIRE); Cheers, Jia On 10/31/2017 7:14 PM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Tue, 31 Oct 2017 10:55:15 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >> <bruce.richardson@intel.com> >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >> loading when doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.4.0 >> >> Hi Jerin > Hi Jia, > >> Do you think next step whether I need to implement the load_acquire half >> barrier as per freebsd > I did a quick prototype using C11 memory model(ACQUIRE/RELEASE) schematics > and tested on two arm64 platform in Cavium(Platform A: Non arm64 OOO machine) > and Platform B: arm64 OOO machine) > > smp_rmb() performs better in Platform A: > acquire/release semantics perform better in platform B: > > Here is the patch: > https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > > In terms of next step: > - I am not sure the cost associated with acquire/release semantics on x86 or ppc. > IMO, We need to have both options under conditional compilation > flags and let the target platform choose the best one. > > Thoughts? > > Here is the performance numbers: > - Both platforms are running at different frequency, So absolute numbers does not > matter, Just check the relative numbers. > > Platform A: Performance numbers: > ================================ > no patch(Non arm64 OOO machine) > ------------------------------- > > SP/SC single enq/dequeue: 40 > MP/MC single enq/dequeue: 282 > SP/SC burst enq/dequeue (size: 8): 11 > MP/MC burst enq/dequeue (size: 8): 42 > SP/SC burst enq/dequeue (size: 32): 8 > MP/MC burst enq/dequeue (size: 32): 16 > > ### Testing empty dequeue ### > SC empty dequeue: 8.01 > MC empty dequeue: 11.01 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 11.30 > MP/MC bulk enq/dequeue (size: 8): 42.85 > SP/SC bulk enq/dequeue (size: 32): 8.25 > MP/MC bulk enq/dequeue (size: 32): 16.46 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 20.62 > MP/MC bulk enq/dequeue (size: 8): 56.30 > SP/SC bulk enq/dequeue (size: 32): 10.94 > MP/MC bulk enq/dequeue (size: 32): 18.66 > Test OK > > # smp_rmb() patch((Non OOO arm64 machine) > http://dpdk.org/dev/patchwork/patch/30029/ > ----------------------------------------- > > SP/SC single enq/dequeue: 42 > MP/MC single enq/dequeue: 291 > SP/SC burst enq/dequeue (size: 8): 12 > MP/MC burst enq/dequeue (size: 8): 44 > SP/SC burst enq/dequeue (size: 32): 8 > MP/MC burst enq/dequeue (size: 32): 16 > > ### Testing empty dequeue ### > SC empty dequeue: 13.01 > MC empty dequeue: 15.01 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 11.60 > MP/MC bulk enq/dequeue (size: 8): 44.32 > SP/SC bulk enq/dequeue (size: 32): 8.60 > MP/MC bulk enq/dequeue (size: 32): 16.50 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 20.95 > MP/MC bulk enq/dequeue (size: 8): 56.90 > SP/SC bulk enq/dequeue (size: 32): 10.90 > MP/MC bulk enq/dequeue (size: 32): 18.78 > Test OK > RTE>> > > # c11 memory model patch((Non OOO arm64 machine) > https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > --------------------------------------------------------------------------------------------- > ### Testing single element and burst enq/deq ### > SP/SC single enq/dequeue: 197 > MP/MC single enq/dequeue: 328 > SP/SC burst enq/dequeue (size: 8): 31 > MP/MC burst enq/dequeue (size: 8): 50 > SP/SC burst enq/dequeue (size: 32): 13 > MP/MC burst enq/dequeue (size: 32): 18 > > ### Testing empty dequeue ### > SC empty dequeue: 13.01 > MC empty dequeue: 18.02 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 30.95 > MP/MC bulk enq/dequeue (size: 8): 50.30 > SP/SC bulk enq/dequeue (size: 32): 13.27 > MP/MC bulk enq/dequeue (size: 32): 18.11 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 43.38 > MP/MC bulk enq/dequeue (size: 8): 64.42 > SP/SC bulk enq/dequeue (size: 32): 16.71 > MP/MC bulk enq/dequeue (size: 32): 22.21 > > > Platform B: Performance numbers: > ============================== > #no patch(OOO arm64 machine) > ---------------------------- > > ### Testing single element and burst enq/deq ### > SP/SC single enq/dequeue: 81 > MP/MC single enq/dequeue: 207 > SP/SC burst enq/dequeue (size: 8): 15 > MP/MC burst enq/dequeue (size: 8): 31 > SP/SC burst enq/dequeue (size: 32): 7 > MP/MC burst enq/dequeue (size: 32): 11 > > ### Testing empty dequeue ### > SC empty dequeue: 3.00 > MC empty dequeue: 5.00 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 15.38 > MP/MC bulk enq/dequeue (size: 8): 30.64 > SP/SC bulk enq/dequeue (size: 32): 7.25 > MP/MC bulk enq/dequeue (size: 32): 11.06 > > ### Testing using two hyperthreads ### > SP/SC bulk enq/dequeue (size: 8): 31.51 > MP/MC bulk enq/dequeue (size: 8): 49.38 > SP/SC bulk enq/dequeue (size: 32): 14.32 > MP/MC bulk enq/dequeue (size: 32): 15.89 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 72.66 > MP/MC bulk enq/dequeue (size: 8): 121.89 > SP/SC bulk enq/dequeue (size: 32): 16.88 > MP/MC bulk enq/dequeue (size: 32): 24.23 > Test OK > RTE>> > > > # smp_rmb() patch((OOO arm64 machine) > http://dpdk.org/dev/patchwork/patch/30029/ > ------------------------------------------- > > ### Testing single element and burst enq/deq ### > SP/SC single enq/dequeue: 152 > MP/MC single enq/dequeue: 265 > SP/SC burst enq/dequeue (size: 8): 24 > MP/MC burst enq/dequeue (size: 8): 39 > SP/SC burst enq/dequeue (size: 32): 9 > MP/MC burst enq/dequeue (size: 32): 13 > > ### Testing empty dequeue ### > SC empty dequeue: 31.01 > MC empty dequeue: 32.01 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 24.26 > MP/MC bulk enq/dequeue (size: 8): 39.52 > SP/SC bulk enq/dequeue (size: 32): 9.47 > MP/MC bulk enq/dequeue (size: 32): 13.31 > > ### Testing using two hyperthreads ### > SP/SC bulk enq/dequeue (size: 8): 40.29 > MP/MC bulk enq/dequeue (size: 8): 59.57 > SP/SC bulk enq/dequeue (size: 32): 17.34 > MP/MC bulk enq/dequeue (size: 32): 21.58 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 79.05 > MP/MC bulk enq/dequeue (size: 8): 153.46 > SP/SC bulk enq/dequeue (size: 32): 26.41 > MP/MC bulk enq/dequeue (size: 32): 38.37 > Test OK > RTE>> > > > # c11 memory model patch((OOO arm64 machine) > https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > ---------------------------------------------------------------------------------------------- > ### Testing single element and burst enq/deq ### > SP/SC single enq/dequeue: 98 > MP/MC single enq/dequeue: 130 > SP/SC burst enq/dequeue (size: 8): 18 > MP/MC burst enq/dequeue (size: 8): 22 > SP/SC burst enq/dequeue (size: 32): 7 > MP/MC burst enq/dequeue (size: 32): 9 > > ### Testing empty dequeue ### > SC empty dequeue: 4.00 > MC empty dequeue: 5.00 > > ### Testing using a single lcore ### > SP/SC bulk enq/dequeue (size: 8): 17.40 > MP/MC bulk enq/dequeue (size: 8): 22.88 > SP/SC bulk enq/dequeue (size: 32): 7.62 > MP/MC bulk enq/dequeue (size: 32): 8.96 > > ### Testing using two hyperthreads ### > SP/SC bulk enq/dequeue (size: 8): 20.24 > MP/MC bulk enq/dequeue (size: 8): 25.83 > SP/SC bulk enq/dequeue (size: 32): 12.21 > MP/MC bulk enq/dequeue (size: 32): 13.20 > > ### Testing using two physical cores ### > SP/SC bulk enq/dequeue (size: 8): 67.54 > MP/MC bulk enq/dequeue (size: 8): 124.63 > SP/SC bulk enq/dequeue (size: 32): 21.13 > MP/MC bulk enq/dequeue (size: 32): 28.44 > Test OK > RTE>>quit > > >> or find any other performance test case to compare the performance impact? > As far as I know, ring_perf_autotest is the better performance test. > If you have trouble in using "High-resolution cycle counter" in your platform then also > you can use ring_perf_auto test to compare the performance(as relative > number matters) > > Jerin > >> Thanks for any suggestions. >> >> Cheers, >> Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-11-01 2:53 ` Jia He @ 2017-11-01 19:04 ` Jerin Jacob 2017-11-02 1:09 ` Jia He 2017-11-02 8:57 ` Jia He 0 siblings, 2 replies; 41+ messages in thread From: Jerin Jacob @ 2017-11-01 19:04 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal Date: Thu, 2 Nov 2017 00:27:46 +0530 From: Jerin Jacob <jerin.jacob@caviumnetworks.com> To: Jia He <hejianet@gmail.com> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" <bruce.richardson@intel.com>, jianbo.liu@arm.com, hemant.agrawal@nxp.com Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue Message-ID: <20171101185723.GA18759@jerin> References: <2601191342CEEE43887BDE71AB9772585FAAB570@IRSMSX103.ger.corp.intel.com> <3e580cd7-2854-d855-be9c-7c4ce06e3ed5@gmail.com> <20171020054319.GA4249@jerin> <ab7154a2-a9f8-f12e-b6a0-2805c2065e2e@gmail.com> <20171023100617.GA17957@jerin> <b0e6eae2-e61b-1946-ac44-d781225778e5@gmail.com> <20171025132642.GA13977@jerin> <b54ce545-b75d-9813-209f-4125bd76c7db@gmail.com> <20171031111433.GA21742@jerin> <69adfb00-4582-b362-0540-d1d9d6bcf6aa@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <69adfb00-4582-b362-0540-d1d9d6bcf6aa@gmail.com> User-Agent: Mutt/1.9.1 (2017-09-22) -----Original Message----- > Date: Wed, 1 Nov 2017 10:53:12 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > <bruce.richardson@intel.com>, jianbo.liu@arm.com, hemant.agrawal@nxp.com > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > Hi Jerin > > Thanks for your suggestions. I will try to use config macro to let it be > chosen by user. It is better, if we avoid #ifdef's in the common code. I think, you can do the scheme like examples/l3fwd/l3fwd_em_hlm_neon.h. Where, the common code will have all generic routine, difference will be moved to a separate files to reduce #ifdef clutter. > > I need to point out one possible issue in your load_acq/store_rel patch > > at https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > > @@ -516,8 +541,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int > is_sc, > /* Restore n as it may change every loop */ > n = max; > > +#if 0 > *old_head = r->cons.head; > const uint32_t prod_tail = r->prod.tail; > +#else > + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_RELAXED); > --[1] > + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, > __ATOMIC_ACQUIRE); --[2] > +#endif > > line [1] __ATOMIC_RELAXED is not enough for this case(tested in our ARM64 > server). > > line [2] __ATOMIC_ACQUIRE guarantee the 2nd load will not be reorded before > the 1st load, but will not > > guarantee the 1st load will not be reordered after the 2nd load. Please also For me it looks same. [1] can not cross [2] is same as [2] cannot cross [1], if [1] are [2] at back to back. No ? > refer to your mentioned freebsd implementation. They use __ATOMIC_ACQUIRE at > line [1]. If I understand it correctly. in freebsd, they are planning to revisit the usage of first __ATOMIC_ACQUIRE in Freebsd-12 https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L170 > > Should it be like instead? > > +#else > + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); > + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, > __ATOMIC_ACQUIRE); It would be nice to see how much overhead it gives.ie back to back __ATOMIC_ACQUIRE. > > > Cheers, > > Jia > > > > On 10/31/2017 7:14 PM, Jerin Jacob Wrote: > > -----Original Message----- > > > Date: Tue, 31 Oct 2017 10:55:15 +0800 > > > From: Jia He <hejianet@gmail.com> > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > > > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > > > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > > > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > > > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > > > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > > > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > > > <bruce.richardson@intel.com> > > > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > > > loading when doing enqueue/dequeue > > > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > > > Thunderbird/52.4.0 > > > > > > Hi Jerin > > Hi Jia, > > > > > Do you think next step whether I need to implement the load_acquire half > > > barrier as per freebsd > > I did a quick prototype using C11 memory model(ACQUIRE/RELEASE) schematics > > and tested on two arm64 platform in Cavium(Platform A: Non arm64 OOO machine) > > and Platform B: arm64 OOO machine) > > > > smp_rmb() performs better in Platform A: > > acquire/release semantics perform better in platform B: > > > > Here is the patch: > > https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > > > > In terms of next step: > > - I am not sure the cost associated with acquire/release semantics on x86 or ppc. > > IMO, We need to have both options under conditional compilation > > flags and let the target platform choose the best one. > > > > Thoughts? > > > > Here is the performance numbers: > > - Both platforms are running at different frequency, So absolute numbers does not > > matter, Just check the relative numbers. > > > > Platform A: Performance numbers: > > ================================ > > no patch(Non arm64 OOO machine) > > ------------------------------- > > > > SP/SC single enq/dequeue: 40 > > MP/MC single enq/dequeue: 282 > > SP/SC burst enq/dequeue (size: 8): 11 > > MP/MC burst enq/dequeue (size: 8): 42 > > SP/SC burst enq/dequeue (size: 32): 8 > > MP/MC burst enq/dequeue (size: 32): 16 > > > > ### Testing empty dequeue ### > > SC empty dequeue: 8.01 > > MC empty dequeue: 11.01 > > > > ### Testing using a single lcore ### > > SP/SC bulk enq/dequeue (size: 8): 11.30 > > MP/MC bulk enq/dequeue (size: 8): 42.85 > > SP/SC bulk enq/dequeue (size: 32): 8.25 > > MP/MC bulk enq/dequeue (size: 32): 16.46 > > > > ### Testing using two physical cores ### > > SP/SC bulk enq/dequeue (size: 8): 20.62 > > MP/MC bulk enq/dequeue (size: 8): 56.30 > > SP/SC bulk enq/dequeue (size: 32): 10.94 > > MP/MC bulk enq/dequeue (size: 32): 18.66 > > Test OK > > > > # smp_rmb() patch((Non OOO arm64 machine) > > http://dpdk.org/dev/patchwork/patch/30029/ > > ----------------------------------------- > > > > SP/SC single enq/dequeue: 42 > > MP/MC single enq/dequeue: 291 > > SP/SC burst enq/dequeue (size: 8): 12 > > MP/MC burst enq/dequeue (size: 8): 44 > > SP/SC burst enq/dequeue (size: 32): 8 > > MP/MC burst enq/dequeue (size: 32): 16 > > > > ### Testing empty dequeue ### > > SC empty dequeue: 13.01 > > MC empty dequeue: 15.01 > > > > ### Testing using a single lcore ### > > SP/SC bulk enq/dequeue (size: 8): 11.60 > > MP/MC bulk enq/dequeue (size: 8): 44.32 > > SP/SC bulk enq/dequeue (size: 32): 8.60 > > MP/MC bulk enq/dequeue (size: 32): 16.50 > > > > ### Testing using two physical cores ### > > SP/SC bulk enq/dequeue (size: 8): 20.95 > > MP/MC bulk enq/dequeue (size: 8): 56.90 > > SP/SC bulk enq/dequeue (size: 32): 10.90 > > MP/MC bulk enq/dequeue (size: 32): 18.78 > > Test OK > > RTE>> > > > > # c11 memory model patch((Non OOO arm64 machine) > > https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > > --------------------------------------------------------------------------------------------- > > ### Testing single element and burst enq/deq ### > > SP/SC single enq/dequeue: 197 > > MP/MC single enq/dequeue: 328 > > SP/SC burst enq/dequeue (size: 8): 31 > > MP/MC burst enq/dequeue (size: 8): 50 > > SP/SC burst enq/dequeue (size: 32): 13 > > MP/MC burst enq/dequeue (size: 32): 18 > > > > ### Testing empty dequeue ### > > SC empty dequeue: 13.01 > > MC empty dequeue: 18.02 > > > > ### Testing using a single lcore ### > > SP/SC bulk enq/dequeue (size: 8): 30.95 > > MP/MC bulk enq/dequeue (size: 8): 50.30 > > SP/SC bulk enq/dequeue (size: 32): 13.27 > > MP/MC bulk enq/dequeue (size: 32): 18.11 > > > > ### Testing using two physical cores ### > > SP/SC bulk enq/dequeue (size: 8): 43.38 > > MP/MC bulk enq/dequeue (size: 8): 64.42 > > SP/SC bulk enq/dequeue (size: 32): 16.71 > > MP/MC bulk enq/dequeue (size: 32): 22.21 > > > > > > Platform B: Performance numbers: > > ============================== > > #no patch(OOO arm64 machine) > > ---------------------------- > > > > ### Testing single element and burst enq/deq ### > > SP/SC single enq/dequeue: 81 > > MP/MC single enq/dequeue: 207 > > SP/SC burst enq/dequeue (size: 8): 15 > > MP/MC burst enq/dequeue (size: 8): 31 > > SP/SC burst enq/dequeue (size: 32): 7 > > MP/MC burst enq/dequeue (size: 32): 11 > > > > ### Testing empty dequeue ### > > SC empty dequeue: 3.00 > > MC empty dequeue: 5.00 > > > > ### Testing using a single lcore ### > > SP/SC bulk enq/dequeue (size: 8): 15.38 > > MP/MC bulk enq/dequeue (size: 8): 30.64 > > SP/SC bulk enq/dequeue (size: 32): 7.25 > > MP/MC bulk enq/dequeue (size: 32): 11.06 > > > > ### Testing using two hyperthreads ### > > SP/SC bulk enq/dequeue (size: 8): 31.51 > > MP/MC bulk enq/dequeue (size: 8): 49.38 > > SP/SC bulk enq/dequeue (size: 32): 14.32 > > MP/MC bulk enq/dequeue (size: 32): 15.89 > > > > ### Testing using two physical cores ### > > SP/SC bulk enq/dequeue (size: 8): 72.66 > > MP/MC bulk enq/dequeue (size: 8): 121.89 > > SP/SC bulk enq/dequeue (size: 32): 16.88 > > MP/MC bulk enq/dequeue (size: 32): 24.23 > > Test OK > > RTE>> > > > > > > # smp_rmb() patch((OOO arm64 machine) > > http://dpdk.org/dev/patchwork/patch/30029/ > > ------------------------------------------- > > > > ### Testing single element and burst enq/deq ### > > SP/SC single enq/dequeue: 152 > > MP/MC single enq/dequeue: 265 > > SP/SC burst enq/dequeue (size: 8): 24 > > MP/MC burst enq/dequeue (size: 8): 39 > > SP/SC burst enq/dequeue (size: 32): 9 > > MP/MC burst enq/dequeue (size: 32): 13 > > > > ### Testing empty dequeue ### > > SC empty dequeue: 31.01 > > MC empty dequeue: 32.01 > > > > ### Testing using a single lcore ### > > SP/SC bulk enq/dequeue (size: 8): 24.26 > > MP/MC bulk enq/dequeue (size: 8): 39.52 > > SP/SC bulk enq/dequeue (size: 32): 9.47 > > MP/MC bulk enq/dequeue (size: 32): 13.31 > > > > ### Testing using two hyperthreads ### > > SP/SC bulk enq/dequeue (size: 8): 40.29 > > MP/MC bulk enq/dequeue (size: 8): 59.57 > > SP/SC bulk enq/dequeue (size: 32): 17.34 > > MP/MC bulk enq/dequeue (size: 32): 21.58 > > > > ### Testing using two physical cores ### > > SP/SC bulk enq/dequeue (size: 8): 79.05 > > MP/MC bulk enq/dequeue (size: 8): 153.46 > > SP/SC bulk enq/dequeue (size: 32): 26.41 > > MP/MC bulk enq/dequeue (size: 32): 38.37 > > Test OK > > RTE>> > > > > > > # c11 memory model patch((OOO arm64 machine) > > https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch > > ---------------------------------------------------------------------------------------------- > > ### Testing single element and burst enq/deq ### > > SP/SC single enq/dequeue: 98 > > MP/MC single enq/dequeue: 130 > > SP/SC burst enq/dequeue (size: 8): 18 > > MP/MC burst enq/dequeue (size: 8): 22 > > SP/SC burst enq/dequeue (size: 32): 7 > > MP/MC burst enq/dequeue (size: 32): 9 > > > > ### Testing empty dequeue ### > > SC empty dequeue: 4.00 > > MC empty dequeue: 5.00 > > > > ### Testing using a single lcore ### > > SP/SC bulk enq/dequeue (size: 8): 17.40 > > MP/MC bulk enq/dequeue (size: 8): 22.88 > > SP/SC bulk enq/dequeue (size: 32): 7.62 > > MP/MC bulk enq/dequeue (size: 32): 8.96 > > > > ### Testing using two hyperthreads ### > > SP/SC bulk enq/dequeue (size: 8): 20.24 > > MP/MC bulk enq/dequeue (size: 8): 25.83 > > SP/SC bulk enq/dequeue (size: 32): 12.21 > > MP/MC bulk enq/dequeue (size: 32): 13.20 > > > > ### Testing using two physical cores ### > > SP/SC bulk enq/dequeue (size: 8): 67.54 > > MP/MC bulk enq/dequeue (size: 8): 124.63 > > SP/SC bulk enq/dequeue (size: 32): 21.13 > > MP/MC bulk enq/dequeue (size: 32): 28.44 > > Test OK > > RTE>>quit > > > > > > > or find any other performance test case to compare the performance impact? > > As far as I know, ring_perf_autotest is the better performance test. > > If you have trouble in using "High-resolution cycle counter" in your platform then also > > you can use ring_perf_auto test to compare the performance(as relative > > number matters) > > > > Jerin > > > > > Thanks for any suggestions. > > > > > > Cheers, > > > Jia > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-11-01 19:04 ` Jerin Jacob @ 2017-11-02 1:09 ` Jia He 2017-11-02 8:57 ` Jia He 1 sibling, 0 replies; 41+ messages in thread From: Jia He @ 2017-11-02 1:09 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal Hi Jerin On 11/2/2017 3:04 AM, Jerin Jacob Wrote: > Date: Thu, 2 Nov 2017 00:27:46 +0530 > From: Jerin Jacob <jerin.jacob@caviumnetworks.com> > To: Jia He <hejianet@gmail.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, > "Zhao, Bing" <ilovethull@163.com>, > Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, > "jia.he@hxt-semitech.com" <jia.he@hxt-semitech.com>, > "jie2.liu@hxt-semitech.com" <jie2.liu@hxt-semitech.com>, > "bing.zhao@hxt-semitech.com" <bing.zhao@hxt-semitech.com>, > "Richardson, Bruce" <bruce.richardson@intel.com>, > jianbo.liu@arm.com, hemant.agrawal@nxp.com > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading > when doing enqueue/dequeue > Message-ID: <20171101185723.GA18759@jerin> > References: <2601191342CEEE43887BDE71AB9772585FAAB570@IRSMSX103.ger.corp.intel.com> > <3e580cd7-2854-d855-be9c-7c4ce06e3ed5@gmail.com> > <20171020054319.GA4249@jerin> > <ab7154a2-a9f8-f12e-b6a0-2805c2065e2e@gmail.com> > <20171023100617.GA17957@jerin> > <b0e6eae2-e61b-1946-ac44-d781225778e5@gmail.com> > <20171025132642.GA13977@jerin> > <b54ce545-b75d-9813-209f-4125bd76c7db@gmail.com> > <20171031111433.GA21742@jerin> > <69adfb00-4582-b362-0540-d1d9d6bcf6aa@gmail.com> > MIME-Version: 1.0 > Content-Type: text/plain; charset=iso-8859-1 > Content-Disposition: inline > Content-Transfer-Encoding: 8bit > In-Reply-To: <69adfb00-4582-b362-0540-d1d9d6bcf6aa@gmail.com> > User-Agent: Mutt/1.9.1 (2017-09-22) > > -----Original Message----- >> Date: Wed, 1 Nov 2017 10:53:12 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >> <bruce.richardson@intel.com>, jianbo.liu@arm.com, hemant.agrawal@nxp.com >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >> loading when doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.4.0 >> >> Hi Jerin >> >> Thanks for your suggestions. I will try to use config macro to let it be >> chosen by user. > It is better, if we avoid #ifdef's in the common code. I think, you can > do the scheme like examples/l3fwd/l3fwd_em_hlm_neon.h. Where, > the common code will have all generic routine, difference will be > moved to a separate files to reduce #ifdef clutter. > >> I need to point out one possible issue in your load_acq/store_rel patch >> >> at https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch >> >> @@ -516,8 +541,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int >> is_sc, >> /* Restore n as it may change every loop */ >> n = max; >> >> +#if 0 >> *old_head = r->cons.head; >> const uint32_t prod_tail = r->prod.tail; >> +#else >> + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_RELAXED); >> --[1] >> + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, >> __ATOMIC_ACQUIRE); --[2] >> +#endif >> >> line [1] __ATOMIC_RELAXED is not enough for this case(tested in our ARM64 >> server). >> >> line [2] __ATOMIC_ACQUIRE guarantee the 2nd load will not be reorded before >> the 1st load, but will not >> >> guarantee the 1st load will not be reordered after the 2nd load. Please also > For me it looks same. [1] can not cross [2] is same as [2] cannot cross > [1], if [1] are [2] at back to back. No ? No, IIUC, load_acquire(a) is equal to the pseudo codes: load(a) one-direction-barrier() instead of one-direction-barrier() load(a) Thus, in below cases, load(a) and load(b) can still be reordered, this is not the semantic violation of the load_acquire: load(a) load_acquire(b) IIUC, the orginal implementation in https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L170 is not optimal. I tested the changes as follow and it works fine: + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); const uint32_t prod_tail = r->prod.tail; i.e. load_acquire(a) load(b) Cheers, Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-11-01 19:04 ` Jerin Jacob 2017-11-02 1:09 ` Jia He @ 2017-11-02 8:57 ` Jia He 2017-11-03 2:55 ` Jia He 1 sibling, 1 reply; 41+ messages in thread From: Jia He @ 2017-11-02 8:57 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal Hi， Jerin please see my performance test below On 11/2/2017 3:04 AM, Jerin Jacob Wrote: [...] > Should it be like instead? > > +#else > + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); > + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, > __ATOMIC_ACQUIRE); > It would be nice to see how much overhead it gives.ie back to back > __ATOMIC_ACQUIRE. I can NOT test ring_perf_autotest in our server because of the something wrong in PMU counter. All the return value of rte_rdtsc is 0 with and without your provided ko module. I am still investigating the reason. I ever tested the difference with my debug patch, the difference is minor, less than +-1% -- Cheers, Jia > >> >> Cheers, >> >> Jia >> >> >> >> On 10/31/2017 7:14 PM, Jerin Jacob Wrote: >>> -----Original Message----- >>>> Date: Tue, 31 Oct 2017 10:55:15 +0800 >>>> From: Jia He <hejianet@gmail.com> >>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >>>> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >>>> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >>>> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >>>> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >>>> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >>>> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >>>> <bruce.richardson@intel.com> >>>> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >>>> loading when doing enqueue/dequeue >>>> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >>>> Thunderbird/52.4.0 >>>> >>>> Hi Jerin >>> Hi Jia, >>> >>>> Do you think next step whether I need to implement the load_acquire half >>>> barrier as per freebsd >>> I did a quick prototype using C11 memory model(ACQUIRE/RELEASE) schematics >>> and tested on two arm64 platform in Cavium(Platform A: Non arm64 OOO machine) >>> and Platform B: arm64 OOO machine) >>> >>> smp_rmb() performs better in Platform A: >>> acquire/release semantics perform better in platform B: >>> >>> Here is the patch: >>> https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch >>> >>> In terms of next step: >>> - I am not sure the cost associated with acquire/release semantics on x86 or ppc. >>> IMO, We need to have both options under conditional compilation >>> flags and let the target platform choose the best one. >>> >>> Thoughts? >>> >>> Here is the performance numbers: >>> - Both platforms are running at different frequency, So absolute numbers does not >>> matter, Just check the relative numbers. >>> >>> Platform A: Performance numbers: >>> ================================ >>> no patch(Non arm64 OOO machine) >>> ------------------------------- >>> >>> SP/SC single enq/dequeue: 40 >>> MP/MC single enq/dequeue: 282 >>> SP/SC burst enq/dequeue (size: 8): 11 >>> MP/MC burst enq/dequeue (size: 8): 42 >>> SP/SC burst enq/dequeue (size: 32): 8 >>> MP/MC burst enq/dequeue (size: 32): 16 >>> >>> ### Testing empty dequeue ### >>> SC empty dequeue: 8.01 >>> MC empty dequeue: 11.01 >>> >>> ### Testing using a single lcore ### >>> SP/SC bulk enq/dequeue (size: 8): 11.30 >>> MP/MC bulk enq/dequeue (size: 8): 42.85 >>> SP/SC bulk enq/dequeue (size: 32): 8.25 >>> MP/MC bulk enq/dequeue (size: 32): 16.46 >>> >>> ### Testing using two physical cores ### >>> SP/SC bulk enq/dequeue (size: 8): 20.62 >>> MP/MC bulk enq/dequeue (size: 8): 56.30 >>> SP/SC bulk enq/dequeue (size: 32): 10.94 >>> MP/MC bulk enq/dequeue (size: 32): 18.66 >>> Test OK >>> >>> # smp_rmb() patch((Non OOO arm64 machine) >>> http://dpdk.org/dev/patchwork/patch/30029/ >>> ----------------------------------------- >>> >>> SP/SC single enq/dequeue: 42 >>> MP/MC single enq/dequeue: 291 >>> SP/SC burst enq/dequeue (size: 8): 12 >>> MP/MC burst enq/dequeue (size: 8): 44 >>> SP/SC burst enq/dequeue (size: 32): 8 >>> MP/MC burst enq/dequeue (size: 32): 16 >>> >>> ### Testing empty dequeue ### >>> SC empty dequeue: 13.01 >>> MC empty dequeue: 15.01 >>> >>> ### Testing using a single lcore ### >>> SP/SC bulk enq/dequeue (size: 8): 11.60 >>> MP/MC bulk enq/dequeue (size: 8): 44.32 >>> SP/SC bulk enq/dequeue (size: 32): 8.60 >>> MP/MC bulk enq/dequeue (size: 32): 16.50 >>> >>> ### Testing using two physical cores ### >>> SP/SC bulk enq/dequeue (size: 8): 20.95 >>> MP/MC bulk enq/dequeue (size: 8): 56.90 >>> SP/SC bulk enq/dequeue (size: 32): 10.90 >>> MP/MC bulk enq/dequeue (size: 32): 18.78 >>> Test OK >>> RTE>> >>> >>> # c11 memory model patch((Non OOO arm64 machine) >>> https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch >>> --------------------------------------------------------------------------------------------- >>> ### Testing single element and burst enq/deq ### >>> SP/SC single enq/dequeue: 197 >>> MP/MC single enq/dequeue: 328 >>> SP/SC burst enq/dequeue (size: 8): 31 >>> MP/MC burst enq/dequeue (size: 8): 50 >>> SP/SC burst enq/dequeue (size: 32): 13 >>> MP/MC burst enq/dequeue (size: 32): 18 >>> >>> ### Testing empty dequeue ### >>> SC empty dequeue: 13.01 >>> MC empty dequeue: 18.02 >>> >>> ### Testing using a single lcore ### >>> SP/SC bulk enq/dequeue (size: 8): 30.95 >>> MP/MC bulk enq/dequeue (size: 8): 50.30 >>> SP/SC bulk enq/dequeue (size: 32): 13.27 >>> MP/MC bulk enq/dequeue (size: 32): 18.11 >>> >>> ### Testing using two physical cores ### >>> SP/SC bulk enq/dequeue (size: 8): 43.38 >>> MP/MC bulk enq/dequeue (size: 8): 64.42 >>> SP/SC bulk enq/dequeue (size: 32): 16.71 >>> MP/MC bulk enq/dequeue (size: 32): 22.21 >>> >>> >>> Platform B: Performance numbers: >>> ============================== >>> #no patch(OOO arm64 machine) >>> ---------------------------- >>> >>> ### Testing single element and burst enq/deq ### >>> SP/SC single enq/dequeue: 81 >>> MP/MC single enq/dequeue: 207 >>> SP/SC burst enq/dequeue (size: 8): 15 >>> MP/MC burst enq/dequeue (size: 8): 31 >>> SP/SC burst enq/dequeue (size: 32): 7 >>> MP/MC burst enq/dequeue (size: 32): 11 >>> >>> ### Testing empty dequeue ### >>> SC empty dequeue: 3.00 >>> MC empty dequeue: 5.00 >>> >>> ### Testing using a single lcore ### >>> SP/SC bulk enq/dequeue (size: 8): 15.38 >>> MP/MC bulk enq/dequeue (size: 8): 30.64 >>> SP/SC bulk enq/dequeue (size: 32): 7.25 >>> MP/MC bulk enq/dequeue (size: 32): 11.06 >>> >>> ### Testing using two hyperthreads ### >>> SP/SC bulk enq/dequeue (size: 8): 31.51 >>> MP/MC bulk enq/dequeue (size: 8): 49.38 >>> SP/SC bulk enq/dequeue (size: 32): 14.32 >>> MP/MC bulk enq/dequeue (size: 32): 15.89 >>> >>> ### Testing using two physical cores ### >>> SP/SC bulk enq/dequeue (size: 8): 72.66 >>> MP/MC bulk enq/dequeue (size: 8): 121.89 >>> SP/SC bulk enq/dequeue (size: 32): 16.88 >>> MP/MC bulk enq/dequeue (size: 32): 24.23 >>> Test OK >>> RTE>> >>> >>> >>> # smp_rmb() patch((OOO arm64 machine) >>> http://dpdk.org/dev/patchwork/patch/30029/ >>> ------------------------------------------- >>> >>> ### Testing single element and burst enq/deq ### >>> SP/SC single enq/dequeue: 152 >>> MP/MC single enq/dequeue: 265 >>> SP/SC burst enq/dequeue (size: 8): 24 >>> MP/MC burst enq/dequeue (size: 8): 39 >>> SP/SC burst enq/dequeue (size: 32): 9 >>> MP/MC burst enq/dequeue (size: 32): 13 >>> >>> ### Testing empty dequeue ### >>> SC empty dequeue: 31.01 >>> MC empty dequeue: 32.01 >>> >>> ### Testing using a single lcore ### >>> SP/SC bulk enq/dequeue (size: 8): 24.26 >>> MP/MC bulk enq/dequeue (size: 8): 39.52 >>> SP/SC bulk enq/dequeue (size: 32): 9.47 >>> MP/MC bulk enq/dequeue (size: 32): 13.31 >>> >>> ### Testing using two hyperthreads ### >>> SP/SC bulk enq/dequeue (size: 8): 40.29 >>> MP/MC bulk enq/dequeue (size: 8): 59.57 >>> SP/SC bulk enq/dequeue (size: 32): 17.34 >>> MP/MC bulk enq/dequeue (size: 32): 21.58 >>> >>> ### Testing using two physical cores ### >>> SP/SC bulk enq/dequeue (size: 8): 79.05 >>> MP/MC bulk enq/dequeue (size: 8): 153.46 >>> SP/SC bulk enq/dequeue (size: 32): 26.41 >>> MP/MC bulk enq/dequeue (size: 32): 38.37 >>> Test OK >>> RTE>> >>> >>> >>> # c11 memory model patch((OOO arm64 machine) >>> https://github.com/jerinjacobk/mytests/blob/master/ring/0001-ring-using-c11-memory-model.patch >>> ---------------------------------------------------------------------------------------------- >>> ### Testing single element and burst enq/deq ### >>> SP/SC single enq/dequeue: 98 >>> MP/MC single enq/dequeue: 130 >>> SP/SC burst enq/dequeue (size: 8): 18 >>> MP/MC burst enq/dequeue (size: 8): 22 >>> SP/SC burst enq/dequeue (size: 32): 7 >>> MP/MC burst enq/dequeue (size: 32): 9 >>> >>> ### Testing empty dequeue ### >>> SC empty dequeue: 4.00 >>> MC empty dequeue: 5.00 >>> >>> ### Testing using a single lcore ### >>> SP/SC bulk enq/dequeue (size: 8): 17.40 >>> MP/MC bulk enq/dequeue (size: 8): 22.88 >>> SP/SC bulk enq/dequeue (size: 32): 7.62 >>> MP/MC bulk enq/dequeue (size: 32): 8.96 >>> >>> ### Testing using two hyperthreads ### >>> SP/SC bulk enq/dequeue (size: 8): 20.24 >>> MP/MC bulk enq/dequeue (size: 8): 25.83 >>> SP/SC bulk enq/dequeue (size: 32): 12.21 >>> MP/MC bulk enq/dequeue (size: 32): 13.20 >>> >>> ### Testing using two physical cores ### >>> SP/SC bulk enq/dequeue (size: 8): 67.54 >>> MP/MC bulk enq/dequeue (size: 8): 124.63 >>> SP/SC bulk enq/dequeue (size: 32): 21.13 >>> MP/MC bulk enq/dequeue (size: 32): 28.44 >>> Test OK >>> RTE>>quit >>> >>> >>>> or find any other performance test case to compare the performance impact? >>> As far as I know, ring_perf_autotest is the better performance test. >>> If you have trouble in using "High-resolution cycle counter" in your platform then also >>> you can use ring_perf_auto test to compare the performance(as relative >>> number matters) >>> >>> Jerin >>> >>>> Thanks for any suggestions. >>>> >>>> Cheers, >>>> Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-11-02 8:57 ` Jia He @ 2017-11-03 2:55 ` Jia He 2017-11-03 12:47 ` Jerin Jacob 0 siblings, 1 reply; 41+ messages in thread From: Jia He @ 2017-11-03 2:55 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal Hi Jerin On 11/2/2017 4:57 PM, Jia He Wrote: > > Hi， Jerin > please see my performance test below > On 11/2/2017 3:04 AM, Jerin Jacob Wrote: > [...] >> Should it be like instead? >> >> +#else >> + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); >> + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, >> __ATOMIC_ACQUIRE); >> It would be nice to see how much overhead it gives.ie back to back >> __ATOMIC_ACQUIRE. > I can NOT test ring_perf_autotest in our server because of the > something wrong in PMU counter. > All the return value of rte_rdtsc is 0 with and without your provided > ko module. I am still > investigating the reason. > Hi Jerin As for the root cause of rte_rdtsc issue, it might be due to the pmu counter frequency is too low in our arm64 server("Amberwing" from qualcom) [586990.057779] arch_timer_get_cntfrq()=20000000 Only 20MHz instead of 100M/200MHz, and CNTFRQ_EL0 is not even writable in kernel space. Maybe the code in ring_perf_autotest needs to be changed? e.g. printf("SC empty dequeue: %.2F\n", (double)(sc_end-sc_start) / iterations); printf("MC empty dequeue: %.2F\n", (double)(mc_end-mc_start) / iterations); Otherwise it is always 0 if the time difference divides by iterations. -- Cheers, Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-11-03 2:55 ` Jia He @ 2017-11-03 12:47 ` Jerin Jacob 0 siblings, 0 replies; 41+ messages in thread From: Jerin Jacob @ 2017-11-03 12:47 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal -----Original Message----- > Date: Fri, 3 Nov 2017 10:55:40 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > <bruce.richardson@intel.com>, jianbo.liu@arm.com, hemant.agrawal@nxp.com > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > Hi Jerin > > > On 11/2/2017 4:57 PM, Jia He Wrote: > > > > Hi， Jerin > > please see my performance test below > > On 11/2/2017 3:04 AM, Jerin Jacob Wrote: > > [...] > > > Should it be like instead? > > > > > > +#else > > > + *old_head = __atomic_load_n(&r->cons.head, __ATOMIC_ACQUIRE); > > > + const uint32_t prod_tail = __atomic_load_n(&r->prod.tail, > > > __ATOMIC_ACQUIRE); > > > It would be nice to see how much overhead it gives.ie back to back > > > __ATOMIC_ACQUIRE. > > I can NOT test ring_perf_autotest in our server because of the something > > wrong in PMU counter. > > All the return value of rte_rdtsc is 0 with and without your provided ko > > module. I am still > > investigating the reason. > > > > Hi Jerin > > As for the root cause of rte_rdtsc issue, it might be due to the pmu counter > frequency is too low > > in our arm64 server("Amberwing" from qualcom) > > [586990.057779] arch_timer_get_cntfrq()=20000000 > > Only 20MHz instead of 100M/200MHz, and CNTFRQ_EL0 is not even writable in > kernel space. May not be true, as I guess, linux 'perf' write those register in kernel space. Another option could be write from ATF/Secure boot loader if that is the case. > > Maybe the code in ring_perf_autotest needs to be changed? Increase the "iterations" to measure @ 200MHz. > > e.g. > > printf("SC empty dequeue: %.2F\n", > (double)(sc_end-sc_start) / iterations); > printf("MC empty dequeue: %.2F\n", > (double)(mc_end-mc_start) / iterations); > > Otherwise it is always 0 if the time difference divides by iterations. > > > -- > Cheers, > Jia > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-31 11:14 ` Jerin Jacob 2017-11-01 2:53 ` Jia He @ 2017-11-01 4:48 ` Jia He 2017-11-01 19:10 ` Jerin Jacob 1 sibling, 1 reply; 41+ messages in thread From: Jia He @ 2017-11-01 4:48 UTC (permalink / raw) To: Jerin Jacob Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal Hi Jerin On 10/31/2017 7:14 PM, Jerin Jacob Wrote: > -----Original Message----- >> Date: Tue, 31 Oct 2017 10:55:15 +0800 >> From: Jia He <hejianet@gmail.com> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com> >> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" >> <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, >> "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" >> <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" >> <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" >> <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" >> <bruce.richardson@intel.com> >> Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod >> loading when doing enqueue/dequeue >> User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.4.0 >> >> Hi Jerin > Hi Jia, > >> Do you think next step whether I need to implement the load_acquire half >> barrier as per freebsd > I did a quick prototype using C11 memory model(ACQUIRE/RELEASE) schematics > and tested on two arm64 platform in Cavium(Platform A: Non arm64 OOO machine) > and Platform B: arm64 OOO machine) Can you elaborate anything about your Non arm64 OOO machine? As I know, all arm64 server is strong memory order. Am I missed anything? -- Cheers, Jia ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-11-01 4:48 ` Jia He @ 2017-11-01 19:10 ` Jerin Jacob 0 siblings, 0 replies; 41+ messages in thread From: Jerin Jacob @ 2017-11-01 19:10 UTC (permalink / raw) To: Jia He Cc: Ananyev, Konstantin, Zhao, Bing, Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce, jianbo.liu, hemant.agrawal -----Original Message----- > Date: Wed, 1 Nov 2017 12:48:31 +0800 > From: Jia He <hejianet@gmail.com> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > <bruce.richardson@intel.com>, jianbo.liu@arm.com, hemant.agrawal@nxp.com > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > Thunderbird/52.4.0 > > Hi Jerin > > > On 10/31/2017 7:14 PM, Jerin Jacob Wrote: > > -----Original Message----- > > > Date: Tue, 31 Oct 2017 10:55:15 +0800 > > > From: Jia He <hejianet@gmail.com> > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com> > > > Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Zhao, Bing" > > > <ilovethull@163.com>, Olivier MATZ <olivier.matz@6wind.com>, > > > "dev@dpdk.org" <dev@dpdk.org>, "jia.he@hxt-semitech.com" > > > <jia.he@hxt-semitech.com>, "jie2.liu@hxt-semitech.com" > > > <jie2.liu@hxt-semitech.com>, "bing.zhao@hxt-semitech.com" > > > <bing.zhao@hxt-semitech.com>, "Richardson, Bruce" > > > <bruce.richardson@intel.com> > > > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > > > loading when doing enqueue/dequeue > > > User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 > > > Thunderbird/52.4.0 > > > > > > Hi Jerin > > Hi Jia, > > > > > Do you think next step whether I need to implement the load_acquire half > > > barrier as per freebsd > > I did a quick prototype using C11 memory model(ACQUIRE/RELEASE) schematics > > and tested on two arm64 platform in Cavium(Platform A: Non arm64 OOO machine) > > and Platform B: arm64 OOO machine) > Can you elaborate anything about your Non arm64 OOO machine? As I know, all > arm64 server is strong > memory order. Am I missed anything? It is implementation defined. The low end arm64 boxes(non server) may not have the complete OOO support. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-20 1:57 ` Jia He 2017-10-20 5:43 ` Jerin Jacob @ 2017-10-20 7:03 ` Ananyev, Konstantin 1 sibling, 0 replies; 41+ messages in thread From: Ananyev, Konstantin @ 2017-10-20 7:03 UTC (permalink / raw) To: Jia He, Zhao, Bing, Jerin Jacob Cc: Olivier MATZ, dev, jia.he, jie2.liu, bing.zhao, Richardson, Bruce > >> > >>> Hi, > >>> > >>> On 2017/10/19 18:02, Ananyev, Konstantin wrote: > >>>> Hi Jia, > >>>> > >>>>> Hi > >>>>> > >>>>> > >>>>> On 10/13/2017 9:02 AM, Jia He Wrote: > >>>>>> Hi Jerin > >>>>>> > >>>>>> > >>>>>> On 10/13/2017 1:23 AM, Jerin Jacob Wrote: > >>>>>>> -----Original Message----- > >>>>>>>> Date: Thu, 12 Oct 2017 17:05:50 +0000 > >>>>>>>> > >>>>> [...] > >>>>>>> On the same lines, > >>>>>>> > >>>>>>> Jia He, jie2.liu, bing.zhao, > >>>>>>> > >>>>>>> Is this patch based on code review or do you saw this issue on any of > >>>>>>> the > >>>>>>> arm/ppc target? arm64 will have performance impact with this change. > >>>>> sorry, miss one important information > >>>>> Our platform is an aarch64 server with 46 cpus. > >>>>> If we reduced the involved cpu numbers, the bug occurred less frequently. > >>>>> > >>>>> Yes, mb barrier impact the performance, but correctness is more > >>>>> important, isn't it ;-) > >>>>> Maybe we can find any other lightweight barrier here? > >>>>> > >>>>> Cheers, > >>>>> Jia > >>>>>> Based on mbuf_autotest, the rte_panic will be invoked in seconds. > >>>>>> > >>>>>> PANIC in test_refcnt_iter(): > >>>>>> (lcore=0, iter=0): after 10s only 61 of 64 mbufs left free > >>>>>> 1: [./test(rte_dump_stack+0x38) [0x58d868]] > >>>>>> Aborted (core dumped) > >>>>>> > >>>> So is it only reproducible with mbuf refcnt test? > >>>> Could it be reproduced with some 'pure' ring test > >>>> (no mempools/mbufs refcnt, etc.)? > >>>> The reason I am asking - in that test we also have mbuf refcnt updates > >>>> (that's what for that test was created) and we are doing some optimizations here too > >>>> to avoid excessive atomic updates. > >>>> BTW, if the problem is not reproducible without mbuf refcnt, > >>>> can I suggest to extend the test with: > >>>> - add a check that enqueue() operation was successful > >>>> - walk through the pool and check/printf refcnt of each mbuf. > >>>> Hopefully that would give us some extra information what is going wrong here. > >>>> Konstantin > >>>> > >>>> > >>> Currently, the issue is only found in this case here on the ARM > >>> platform, not sure how it is going with the X86_64 platform > >> I understand that it is only reproducible on arm so far. > >> What I am asking - with dpdk is there any other way to reproduce it (on arm) > >> except then running mbuf_autotest? > >> Something really simple that not using mbuf/mempool etc? > >> Just do dequeue/enqueue from multiple threads and check data integrity at the end? > >> If not - what makes you think that the problem is precisely in rte_ring code? > >> Why not in rte_mbuf let say? > > Actually I reread your original mail and finally get your point. > > If I understand you correctly the problem with read reordering here is that > > after we read prot.tail but before we read cons.head > > both cons.head and prod.tail might be updated, > > but for us prod.tail change might be unnoticed. > > As an example: > > time 0 (cons.head == 0, prod.tail == 0): > > prod_tail = r->prod.tail; /* due read reordering */ > > /* prod_tail == 0 */ > > > > time 1 (cons.head ==5, prod.tail == 5): > > *old_head = r->cons.head; > > /* cons.head == 5 */ > > *entries = (prod_tail - *old_head); > > /* *entries == (0 - 5) == 0xFFFFFFFB */ > > > > And we will move our cons.head forward, even though there are no filled entries in the ring. > > Is that right? > Yes Ok, thanks for your patience - it took me a while to understand what you guys are talking about here. > > As I side notice, I wonder why we have here: > > *entries = (prod_tail - *old_head); > > instead of: > > *entries = r->size + prod_tail - *old_head; > > ? > Yes, I agree with you at this code line. > But reordering will still mess up things even after this change(I have > tested, still the same as before) Yes, I am convinced now that rmb is necessary here :) As I said this is just a different thing I noticed while looking at the code. Probably should discuss it in a different thread. Konstantin > I thought the *entries is a door to prevent consumer from moving forward > too fast than the producer. > But in some cases, it is correct that prod_tail is smaller than > *old_head due to the cirular queue. > In other cases, it is incorrect because of read/read reordering. > > AFAICT, the root cause here is the producer tail and cosumer head are > dependant on each other. > Thus a memory barrier is neccessary. > > Cheers, > Jia > > > > > Konstantin > > > >>> . In another > >>> mail of this thread, we've made a simple test based on this and captured > >>> some information and I pasted there.(I pasted the patch there :-)) > >> Are you talking about that one: > >> http://dpdk.org/dev/patchwork/patch/30405/ > >> ? > >> It still uses test/test/test_mbuf.c..., > >> but anyway I don't really understand how mbuf_autotest supposed > >> to work with these changes: > >> @@ -730,7 +739,7 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, > >> rte_ring_enqueue(refcnt_mbuf_ring, m); > >> } > >> } > >> - rte_pktmbuf_free(m); > >> + // rte_pktmbuf_free(m); > >> } > >> @@ -741,6 +750,12 @@ test_refcnt_iter(unsigned int lcore, unsigned int iter, > >> while (!rte_ring_empty(refcnt_mbuf_ring)) > >> ; > >> > >> + if (NULL != m) { > >> + if (1 != rte_mbuf_refcnt_read(m)) > >> + printf("m ref is %u\n", rte_mbuf_refcnt_read(m)); > >> + rte_pktmbuf_free(m); > >> + } > >> + > >> /* check that all mbufs are back into mempool by now */ > >> for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) { > >> if ((i = rte_mempool_avail_count(refcnt_pool)) == n) { > >> > >> That means all your mbufs (except the last one) will still be allocated. > >> So the test would fail - as it should, I think. > >> > >>> And > >>> it seems that Juhamatti & Jacod found some reverting action several > >>> months ago. > >> Didn't get that one either. > >> Konstantin ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-12 17:05 ` Ananyev, Konstantin 2017-10-12 17:23 ` Jerin Jacob @ 2017-10-13 0:24 ` Liu, Jie2 2017-10-13 2:12 ` Zhao, Bing 2017-10-16 10:51 ` Kuusisaari, Juhamatti 1 sibling, 2 replies; 41+ messages in thread From: Liu, Jie2 @ 2017-10-13 0:24 UTC (permalink / raw) To: Ananyev, Konstantin, Olivier MATZ, dev, jerin.jacob Cc: He, Jia, Zhao, Bing, Jia He Hi guys, We found this issue when we run mbuf_autotest. It failed on a aarch64 platform. I am not sure if it can be reproduced on other platforms. Regards, Jie Liu -----Original Message----- From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] Sent: 2017年10月13日 1:06 To: Olivier MATZ <olivier.matz@6wind.com>; Jia He <hejianet@gmail.com> Cc: dev@dpdk.org; He, Jia <jia.he@hxt-semitech.com>; Liu, Jie2 <jie2.liu@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt-semitech.com>; jerin.jacob@caviumnetworks.com Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue Hi guys, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Thursday, October 12, 2017 4:54 PM > To: Jia He <hejianet@gmail.com> > Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; > bing.zhao@hxt-semitech.com; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading > when doing enqueue/dequeue > > Hi, > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > Before this patch: > > In __rte_ring_move_cons_head() > > ... > > do { > > /* Restore n as it may change every loop */ > > n = max; > > > > *old_head = r->cons.head; //1st load > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > In weak memory order architectures(powerpc,arm), the 2nd load might > > be reodered before the 1st load, that makes *entries is bigger than we wanted. > > This nasty reording messed enque/deque up. > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > load r->prod.tail in enqueue: > > load r->cons.tail > > load r->prod.head > > > > store r->prod.tail > > > > load r->cons.head > > load r->prod.tail > > ... > > store r->cons.{head,tail} > > load r->cons.head > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries > > very big > > > > After this patch, the old cons.head will be recaculated after > > failure of rte_atomic32_cmpset > > > > There is no such issue in X86 cpu, because X86 is strong memory > > order model > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > Signed-off-by: jia.he@hxt-semitech.com > > Signed-off-by: jie2.liu@hxt-semitech.com > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > --- > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index 5e9b3b7..15c72e2 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > > n = max; > > > > *old_head = r->prod.head; > > + > > +/* load of prod.tail can't be reordered before cons.head */ > > +rte_smp_rmb(); > > + > > const uint32_t cons_tail = r->cons.tail; > > /* > > * The subtraction is done between two unsigned 32bits value @@ > > -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > > n = max; > > > > *old_head = r->cons.head; > > + > > +/* load of prod.tail can't be reordered before cons.head */ > > +rte_smp_rmb(); > > + > > const uint32_t prod_tail = r->prod.tail; > > /* The subtraction is done between two unsigned 32bits value > > * (the result is always modulo 32 bits even if we have > > -- > > 2.7.4 > > > > The explanation convinces me. > > However, since it's in a critical path, it would be good to have other > opinions. This patch reminds me this discussion, that was also related > to memory barrier, but at another place: > http://dpdk.org/ml/archives/dev/2016-July/043765.html > Lead to that patch: > http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > But finally reverted: > http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > Konstatin, Jerin, do you have any comment? For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, but I can't see how read reordering would screw things up here... Probably just me and arm or ppc guys could explain what will be the problem if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce it with existing one? Konstantin This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 0:24 ` Liu, Jie2 @ 2017-10-13 2:12 ` Zhao, Bing 2017-10-13 2:34 ` Jerin Jacob 2017-10-16 10:51 ` Kuusisaari, Juhamatti 1 sibling, 1 reply; 41+ messages in thread From: Zhao, Bing @ 2017-10-13 2:12 UTC (permalink / raw) To: Liu, Jie2, Ananyev, Konstantin, Olivier MATZ, dev, jerin.jacob Cc: He, Jia, Jia He Hi Guys, If needed, we can provide a simplified UT test case and ring patch to reproduce this. And then we can catch the information to prove that in a SP+MC scenario, when the tail and head pointers' number are smaller than the ring size, some lcore thread will have a fault judge and then make the CH over the PT. Thanks BR. Bing -----Original Message----- From: Liu, Jie2 Sent: 2017年10月13日 8:25 To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Olivier MATZ <olivier.matz@6wind.com>; dev@dpdk.org; jerin.jacob@caviumnetworks.com Cc: He, Jia <jia.he@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt-semitech.com>; Jia He <hejianet@gmail.com> Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue Hi guys, We found this issue when we run mbuf_autotest. It failed on a aarch64 platform. I am not sure if it can be reproduced on other platforms. Regards, Jie Liu -----Original Message----- From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] Sent: 2017年10月13日 1:06 To: Olivier MATZ <olivier.matz@6wind.com>; Jia He <hejianet@gmail.com> Cc: dev@dpdk.org; He, Jia <jia.he@hxt-semitech.com>; Liu, Jie2 <jie2.liu@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt-semitech.com>; jerin.jacob@caviumnetworks.com Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue Hi guys, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Thursday, October 12, 2017 4:54 PM > To: Jia He <hejianet@gmail.com> > Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; > bing.zhao@hxt-semitech.com; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading > when doing enqueue/dequeue > > Hi, > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > Before this patch: > > In __rte_ring_move_cons_head() > > ... > > do { > > /* Restore n as it may change every loop */ > > n = max; > > > > *old_head = r->cons.head; //1st load > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > In weak memory order architectures(powerpc,arm), the 2nd load might > > be reodered before the 1st load, that makes *entries is bigger than we wanted. > > This nasty reording messed enque/deque up. > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > load r->prod.tail in enqueue: > > load r->cons.tail > > load r->prod.head > > > > store r->prod.tail > > > > load r->cons.head > > load r->prod.tail > > ... > > store r->cons.{head,tail} > > load r->cons.head > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries > > very big > > > > After this patch, the old cons.head will be recaculated after > > failure of rte_atomic32_cmpset > > > > There is no such issue in X86 cpu, because X86 is strong memory > > order model > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > Signed-off-by: jia.he@hxt-semitech.com > > Signed-off-by: jie2.liu@hxt-semitech.com > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > --- > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index 5e9b3b7..15c72e2 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > > n = max; > > > > *old_head = r->prod.head; > > + > > +/* load of prod.tail can't be reordered before cons.head */ > > +rte_smp_rmb(); > > + > > const uint32_t cons_tail = r->cons.tail; > > /* > > * The subtraction is done between two unsigned 32bits value @@ > > -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > > n = max; > > > > *old_head = r->cons.head; > > + > > +/* load of prod.tail can't be reordered before cons.head */ > > +rte_smp_rmb(); > > + > > const uint32_t prod_tail = r->prod.tail; > > /* The subtraction is done between two unsigned 32bits value > > * (the result is always modulo 32 bits even if we have > > -- > > 2.7.4 > > > > The explanation convinces me. > > However, since it's in a critical path, it would be good to have other > opinions. This patch reminds me this discussion, that was also related > to memory barrier, but at another place: > http://dpdk.org/ml/archives/dev/2016-July/043765.html > Lead to that patch: > http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > But finally reverted: > http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > Konstatin, Jerin, do you have any comment? For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, but I can't see how read reordering would screw things up here... Probably just me and arm or ppc guys could explain what will be the problem if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce it with existing one? Konstantin This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 2:12 ` Zhao, Bing @ 2017-10-13 2:34 ` Jerin Jacob 0 siblings, 0 replies; 41+ messages in thread From: Jerin Jacob @ 2017-10-13 2:34 UTC (permalink / raw) To: Zhao, Bing Cc: Liu, Jie2, Ananyev, Konstantin, Olivier MATZ, dev, He, Jia, Jia He -----Original Message----- > Date: Fri, 13 Oct 2017 02:12:58 +0000 > From: "Zhao, Bing" <bing.zhao@hxt-semitech.com> > To: "Liu, Jie2" <jie2.liu@hxt-semitech.com>, "Ananyev, Konstantin" > <konstantin.ananyev@intel.com>, Olivier MATZ <olivier.matz@6wind.com>, > "dev@dpdk.org" <dev@dpdk.org>, "jerin.jacob@caviumnetworks.com" > <jerin.jacob@caviumnetworks.com> > CC: "He, Jia" <jia.he@hxt-semitech.com>, Jia He <hejianet@gmail.com> > Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when > doing enqueue/dequeue > > Hi Guys, Hi Bing, > If needed, we can provide a simplified UT test case and ring patch to reproduce this. And then we can catch the information to prove that in a SP+MC scenario, when the tail and head pointers' number are smaller than the ring size, some lcore thread will have a fault judge and then make the CH over the PT. Yes. Please provide simplified UT test case and ring patch. > > Thanks > > BR. Bing > > -----Original Message----- > From: Liu, Jie2 > Sent: 2017年10月13日 8:25 > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Olivier MATZ <olivier.matz@6wind.com>; dev@dpdk.org; jerin.jacob@caviumnetworks.com > Cc: He, Jia <jia.he@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt-semitech.com>; Jia He <hejianet@gmail.com> > Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue > > Hi guys, > We found this issue when we run mbuf_autotest. It failed on a aarch64 platform. I am not sure if it can be reproduced on other platforms. > Regards, > Jie Liu > > -----Original Message----- > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] > Sent: 2017年10月13日 1:06 > To: Olivier MATZ <olivier.matz@6wind.com>; Jia He <hejianet@gmail.com> > Cc: dev@dpdk.org; He, Jia <jia.he@hxt-semitech.com>; Liu, Jie2 <jie2.liu@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt-semitech.com>; jerin.jacob@caviumnetworks.com > Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue > > Hi guys, > > > -----Original Message----- > > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > > Sent: Thursday, October 12, 2017 4:54 PM > > To: Jia He <hejianet@gmail.com> > > Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; > > bing.zhao@hxt-semitech.com; Ananyev, Konstantin > > <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com > > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading > > when doing enqueue/dequeue > > > > Hi, > > > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > > Before this patch: > > > In __rte_ring_move_cons_head() > > > ... > > > do { > > > /* Restore n as it may change every loop */ > > > n = max; > > > > > > *old_head = r->cons.head; //1st load > > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > > > In weak memory order architectures(powerpc,arm), the 2nd load might > > > be reodered before the 1st load, that makes *entries is bigger than we wanted. > > > This nasty reording messed enque/deque up. > > > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > > load r->prod.tail in enqueue: > > > load r->cons.tail > > > load r->prod.head > > > > > > store r->prod.tail > > > > > > load r->cons.head > > > load r->prod.tail > > > ... > > > store r->cons.{head,tail} > > > load r->cons.head > > > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries > > > very big > > > > > > After this patch, the old cons.head will be recaculated after > > > failure of rte_atomic32_cmpset > > > > > > There is no such issue in X86 cpu, because X86 is strong memory > > > order model > > > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > > Signed-off-by: jia.he@hxt-semitech.com > > > Signed-off-by: jie2.liu@hxt-semitech.com > > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > > > --- > > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > > index 5e9b3b7..15c72e2 100644 > > > --- a/lib/librte_ring/rte_ring.h > > > +++ b/lib/librte_ring/rte_ring.h > > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp, > > > n = max; > > > > > > *old_head = r->prod.head; > > > + > > > +/* load of prod.tail can't be reordered before cons.head */ > > > +rte_smp_rmb(); > > > + > > > const uint32_t cons_tail = r->cons.tail; > > > /* > > > * The subtraction is done between two unsigned 32bits value @@ > > > -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > > > n = max; > > > > > > *old_head = r->cons.head; > > > + > > > +/* load of prod.tail can't be reordered before cons.head */ > > > +rte_smp_rmb(); > > > + > > > const uint32_t prod_tail = r->prod.tail; > > > /* The subtraction is done between two unsigned 32bits value > > > * (the result is always modulo 32 bits even if we have > > > -- > > > 2.7.4 > > > > > > > The explanation convinces me. > > > > However, since it's in a critical path, it would be good to have other > > opinions. This patch reminds me this discussion, that was also related > > to memory barrier, but at another place: > > http://dpdk.org/ml/archives/dev/2016-July/043765.html > > Lead to that patch: > > http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > > But finally reverted: > > http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > > > Konstatin, Jerin, do you have any comment? > > For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't make any difference, but I can't see how read reordering would screw things up here... > Probably just me and arm or ppc guys could explain what will be the problem if let say cons.tail will be read before prod.head in __rte_ring_move_prod_head(). > I wonder Is there a simple test-case to reproduce that problem (on arm or ppc)? > Probably new test-case for rte_ring autotest is needed, or is it possible to reproduce it with existing one? > Konstantin > > > > This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you. > > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue 2017-10-13 0:24 ` Liu, Jie2 2017-10-13 2:12 ` Zhao, Bing @ 2017-10-16 10:51 ` Kuusisaari, Juhamatti 1 sibling, 0 replies; 41+ messages in thread From: Kuusisaari, Juhamatti @ 2017-10-16 10:51 UTC (permalink / raw) To: Liu, Jie2, Ananyev, Konstantin, Olivier MATZ, dev, jerin.jacob Cc: He, Jia, Zhao, Bing, Jia He > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jie2 > Sent: Friday, October 13, 2017 3:25 AM > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Olivier MATZ > <olivier.matz@6wind.com>; dev@dpdk.org; > jerin.jacob@caviumnetworks.com > Cc: He, Jia <jia.he@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt- > semitech.com>; Jia He <hejianet@gmail.com> > Subject: Re: [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod > loading when doing enqueue/dequeue > > Hi guys, > We found this issue when we run mbuf_autotest. It failed on a aarch64 > platform. I am not sure if it can be reproduced on other platforms. > Regards, > Jie Liu > > -----Original Message----- > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] > Sent: 2017年10月13日 1:06 > To: Olivier MATZ <olivier.matz@6wind.com>; Jia He <hejianet@gmail.com> > Cc: dev@dpdk.org; He, Jia <jia.he@hxt-semitech.com>; Liu, Jie2 > <jie2.liu@hxt-semitech.com>; Zhao, Bing <bing.zhao@hxt-semitech.com>; > jerin.jacob@caviumnetworks.com > Subject: RE: [PATCH] ring: guarantee ordering of cons/prod loading when > doing enqueue/dequeue > > Hi guys, > > > -----Original Message----- > > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > > Sent: Thursday, October 12, 2017 4:54 PM > > To: Jia He <hejianet@gmail.com> > > Cc: dev@dpdk.org; jia.he@hxt-semitech.com; jie2.liu@hxt-semitech.com; > > bing.zhao@hxt-semitech.com; Ananyev, Konstantin > > <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com > > Subject: Re: [PATCH] ring: guarantee ordering of cons/prod loading > > when doing enqueue/dequeue > > > > Hi, > > > > On Tue, Oct 10, 2017 at 05:56:36PM +0800, Jia He wrote: > > > Before this patch: > > > In __rte_ring_move_cons_head() > > > ... > > > do { > > > /* Restore n as it may change every loop */ > > > n = max; > > > > > > *old_head = r->cons.head; //1st load > > > const uint32_t prod_tail = r->prod.tail; //2nd load > > > > > > In weak memory order architectures(powerpc,arm), the 2nd load might > > > be reodered before the 1st load, that makes *entries is bigger than we > wanted. > > > This nasty reording messed enque/deque up. > > > > > > cpu1(producer) cpu2(consumer) cpu3(consumer) > > > load r->prod.tail in enqueue: > > > load r->cons.tail > > > load r->prod.head > > > > > > store r->prod.tail > > > > > > load r->cons.head > > > load r->prod.tail > > > ... > > > store r->cons.{head,tail} > > > load r->cons.head > > > > > > THEN,r->cons.head will be bigger than prod_tail, then make *entries > > > very big > > > > > > After this patch, the old cons.head will be recaculated after > > > failure of rte_atomic32_cmpset > > > > > > There is no such issue in X86 cpu, because X86 is strong memory > > > order model > > > > > > Signed-off-by: Jia He <hejianet@gmail.com> > > > Signed-off-by: jia.he@hxt-semitech.com > > > Signed-off-by: jie2.liu@hxt-semitech.com > > > Signed-off-by: bing.zhao@hxt-semitech.com > > > > > > --- > > > lib/librte_ring/rte_ring.h | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > > index 5e9b3b7..15c72e2 100644 > > > --- a/lib/librte_ring/rte_ring.h > > > +++ b/lib/librte_ring/rte_ring.h > > > @@ -409,6 +409,10 @@ __rte_ring_move_prod_head(struct rte_ring *r, > > > int is_sp, n = max; > > > > > > *old_head = r->prod.head; > > > + > > > +/* load of prod.tail can't be reordered before cons.head */ > > > +rte_smp_rmb(); > > > + > > > const uint32_t cons_tail = r->cons.tail; > > > /* > > > * The subtraction is done between two unsigned 32bits value @@ > > > -517,6 +521,10 @@ __rte_ring_move_cons_head(struct rte_ring *r, int > > > is_sc, n = max; > > > > > > *old_head = r->cons.head; > > > + > > > +/* load of prod.tail can't be reordered before cons.head */ > > > +rte_smp_rmb(); > > > + > > > const uint32_t prod_tail = r->prod.tail; > > > /* The subtraction is done between two unsigned 32bits value > > > * (the result is always modulo 32 bits even if we have > > > -- > > > 2.7.4 > > > > > > > The explanation convinces me. > > > > However, since it's in a critical path, it would be good to have other > > opinions. This patch reminds me this discussion, that was also related > > to memory barrier, but at another place: > > http://dpdk.org/ml/archives/dev/2016-July/043765.html > > Lead to that patch: > > http://dpdk.org/browse/dpdk/commit/?id=ecc7d10e448e > > But finally reverted: > > http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 > > > > Konstatin, Jerin, do you have any comment? > > For IA, as rte_smp_rmb() is just a compiler_barrier, that patch shouldn't > make any difference, but I can't see how read reordering would screw things > up here... > Probably just me and arm or ppc guys could explain what will be the problem > if let say cons.tail will be read before prod.head in > __rte_ring_move_prod_head(). > I wonder Is there a simple test-case to reproduce that problem (on arm or > ppc)? > Probably new test-case for rte_ring autotest is needed, or is it possible to > reproduce it with existing one? > Konstantin Hi, I think this is a real problem here. We have fixed it so that we read both head and tail atomically as otherwise you may have this problem you are seeing. Works only on 64, of course. Thanks for pointing out the revert, I was unaware of it: http://dpdk.org/browse/dpdk/commit/?id=c3acd92746c3 Nevertheless, the original fix was as such correct on higher level, both PPC and ARM just happened to use strong enough default read barrier which already guaranteed the ordering to be good enough. As we optimize cycles here, I am of course just fine with the revert as long as we all remember what was going on there. BR, -- Juhamatti ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2017-11-03 12:48 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-10 9:56 [dpdk-dev] [PATCH] ring: guarantee ordering of cons/prod loading when doing enqueue/dequeue Jia He 2017-10-12 15:53 ` Olivier MATZ 2017-10-12 16:15 ` Stephen Hemminger 2017-10-12 17:05 ` Ananyev, Konstantin 2017-10-12 17:23 ` Jerin Jacob 2017-10-13 1:02 ` Jia He 2017-10-13 1:15 ` Jia He 2017-10-13 1:16 ` Jia He 2017-10-13 1:49 ` Jerin Jacob 2017-10-13 3:23 ` Jia He 2017-10-13 5:57 ` Zhao, Bing 2017-10-13 7:33 ` Jianbo Liu 2017-10-13 8:20 ` Jia He 2017-10-19 10:02 ` Ananyev, Konstantin 2017-10-19 11:18 ` Zhao, Bing 2017-10-19 14:15 ` Ananyev, Konstantin 2017-10-19 20:02 ` Ananyev, Konstantin 2017-10-20 1:57 ` Jia He 2017-10-20 5:43 ` Jerin Jacob 2017-10-23 8:49 ` Jia He 2017-10-23 9:05 ` Kuusisaari, Juhamatti 2017-10-23 9:10 ` Bruce Richardson 2017-10-23 10:06 ` Jerin Jacob 2017-10-24 2:04 ` Jia He 2017-10-25 13:26 ` Jerin Jacob 2017-10-26 2:27 ` Jia He 2017-10-31 2:55 ` Jia He 2017-10-31 11:14 ` Jerin Jacob 2017-11-01 2:53 ` Jia He 2017-11-01 19:04 ` Jerin Jacob 2017-11-02 1:09 ` Jia He 2017-11-02 8:57 ` Jia He 2017-11-03 2:55 ` Jia He 2017-11-03 12:47 ` Jerin Jacob 2017-11-01 4:48 ` Jia He 2017-11-01 19:10 ` Jerin Jacob 2017-10-20 7:03 ` Ananyev, Konstantin 2017-10-13 0:24 ` Liu, Jie2 2017-10-13 2:12 ` Zhao, Bing 2017-10-13 2:34 ` Jerin Jacob 2017-10-16 10:51 ` Kuusisaari, Juhamatti
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).