| Message ID | 1395148387-8912-1-git-send-email-petri.savolainen@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 03/18/2014 05:13 PM, Petri Savolainen wrote: > There was a race condition with the "in" variable. This patch > uses more robust barrier exit synchronization to avoid the race. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > include/odp_barrier.h | 5 ++- > platform/linux-generic/source/odp_barrier.c | 52 +++++++++++++++++++++-------- > test/example/odp_example.c | 4 +++ > 3 files changed, 47 insertions(+), 14 deletions(-) > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > index bb4a6c5..19dfc06 100644 > --- a/include/odp_barrier.h > +++ b/include/odp_barrier.h > @@ -28,8 +28,11 @@ extern "C" { > */ > typedef struct odp_barrier_t { > int count; > + volatile int wait; > + volatile int wait_exit; is there any reason to use volatile int instead of odp_atomic_int_t ? Please also run ./scripts/cleanfile to remove DOS endings in the patch. And do clean ups ./scripts/checkpatch.pl warnings. > odp_atomic_int_t in; > - odp_atomic_int_t out; > + odp_atomic_int_t waiting; > + odp_atomic_int_t waiting_exit; > } odp_barrier_t; > > > diff --git a/platform/linux-generic/source/odp_barrier.c b/platform/linux-generic/source/odp_barrier.c > index 64fbdb9..b27afc7 100644 > --- a/platform/linux-generic/source/odp_barrier.c > +++ b/platform/linux-generic/source/odp_barrier.c > @@ -10,9 +10,13 @@ > > void odp_barrier_init_count(odp_barrier_t *barrier, int count) > { > - barrier->count = count; > - barrier->in = 0; > - barrier->out = count - 1; > + barrier->count = count; > + barrier->wait = 1; > + barrier->wait_exit = 0; > + barrier->in = 0; > + barrier->waiting = 0; > + barrier->waiting_exit = 0; > + > odp_sync_stores(); > } > > @@ -26,25 +30,47 @@ void odp_barrier_sync(odp_barrier_t *barrier) > count = odp_atomic_fetch_inc_int(&barrier->in); > > if (count == barrier->count - 1) { > - /* If last thread, release others */ > - barrier->in = 0; > + /* > + * This is the last thread. > + * Spin until other threads are waiting. > + */ > + while (barrier->waiting != barrier->count-1) > + odp_spin(); > + > + /* Reset barrier entry */ > + barrier->wait_exit = 1; > + barrier->in = 0; > + barrier->waiting = 0; > + barrier->waiting_exit = 0; > odp_sync_stores(); > > - /* Wait for others to exit */ > - while (barrier->out) > + /* Move other threads to wait for exit */ > + barrier->wait = 0; > + > + /* Spin until other threads are waiting for exit */ > + while (barrier->waiting_exit != barrier->count-1) > odp_spin(); > > - /* Ready, reset out counter */ > - barrier->out = barrier->count - 1; > + /* Reset barrier exit */ > + barrier->waiting_exit = 0; > + barrier->wait = 1; > odp_sync_stores(); > > + /* Let other threads to exit */ > + barrier->wait_exit = 0; > + > + odp_sync_stores(); > } else { > - /* Wait for the last thread*/ > - while (barrier->in) > + odp_atomic_inc_int(&barrier->waiting); > + > + while (barrier->wait) > + odp_spin(); > + > + odp_atomic_inc_int(&barrier->waiting_exit); > + > + while (barrier->wait_exit) > odp_spin(); > > - /* Ready */ > - odp_atomic_dec_int(&barrier->out); > odp_mem_barrier(); > } > } > diff --git a/test/example/odp_example.c b/test/example/odp_example.c > index 4764657..a6ab89e 100644 > --- a/test/example/odp_example.c > +++ b/test/example/odp_example.c > @@ -507,6 +507,10 @@ static void *run_thread(void *arg) > odp_barrier_sync(&test_barrier); > odp_barrier_sync(&test_barrier); > odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > > /* > * Find the buffer pool
On Tuesday, 18 March 2014 15:31:06 UTC+2, Maxim Uvarov wrote: > On 03/18/2014 05:13 PM, Petri Savolainen wrote: > > There was a race condition with the "in" variable. This patch > > uses more robust barrier exit synchronization to avoid the race. > > > > Signed-off-by: Petri Savolainen <petri.sa...@linaro.org <javascript:>> > > --- > > include/odp_barrier.h | 5 ++- > > platform/linux-generic/source/odp_barrier.c | 52 > +++++++++++++++++++++-------- > > test/example/odp_example.c | 4 +++ > > 3 files changed, 47 insertions(+), 14 deletions(-) > > > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > > index bb4a6c5..19dfc06 100644 > > --- a/include/odp_barrier.h > > +++ b/include/odp_barrier.h > > @@ -28,8 +28,11 @@ extern "C" { > > */ > > typedef struct odp_barrier_t { > > int count; > > + volatile int wait; > > + volatile int wait_exit; > is there any reason to use volatile int instead of odp_atomic_int_t ? > > Because those are not atomic counters, but status variables. > > Please also run ./scripts/cleanfile to remove DOS endings in the patch. > And do clean ups > ./scripts/checkpatch.pl warnings. > What DOS endings? Checkpatch didn't show other warnings than volatiles. -Petri
On 03/18/2014 05:46 PM, Petri Savolainen wrote: > > > On Tuesday, 18 March 2014 15:31:06 UTC+2, Maxim Uvarov wrote: > > On 03/18/2014 05:13 PM, Petri Savolainen wrote: > > There was a race condition with the "in" variable. This patch > > uses more robust barrier exit synchronization to avoid the race. > > > > Signed-off-by: Petri Savolainen <petri.sa...@linaro.org > <javascript:>> > > --- > > include/odp_barrier.h | 5 ++- > > platform/linux-generic/source/odp_barrier.c | 52 > +++++++++++++++++++++-------- > > test/example/odp_example.c | 4 +++ > > 3 files changed, 47 insertions(+), 14 deletions(-) > > > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > > index bb4a6c5..19dfc06 100644 > > --- a/include/odp_barrier.h > > +++ b/include/odp_barrier.h > > @@ -28,8 +28,11 @@ extern "C" { > > */ > > typedef struct odp_barrier_t { > > int count; > > + volatile int wait; > > + volatile int wait_exit; > is there any reason to use volatile int instead of odp_atomic_int_t ? > > Because those are not atomic counters, but status variables. > > > Please also run ./scripts/cleanfile to remove DOS endings in the > patch. > And do clean ups > ./scripts/checkpatch.pl <http://checkpatch.pl> warnings. > > > What DOS endings? Checkpatch didn't show other warnings than volatiles. > > -Petri I don't know why but this email has ^M at the end of the lines. However git am cleanly removes them. Don't worry about that. Best regards, Maxim. > -- > You received this message because you are subscribed to the Google > Groups "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to lng-odp+unsubscribe@linaro.org > <mailto:lng-odp+unsubscribe@linaro.org>. > To post to this group, send email to lng-odp@linaro.org > <mailto:lng-odp@linaro.org>. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/964440bc-af68-4841-aa1b-3752170f66f7%40linaro.org > <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/964440bc-af68-4841-aa1b-3752170f66f7%40linaro.org?utm_medium=email&utm_source=footer>. > For more options, visit https://groups.google.com/a/linaro.org/d/optout.
On 18 March 2014 15:02, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/18/2014 05:46 PM, Petri Savolainen wrote: > >> >> >> On Tuesday, 18 March 2014 15:31:06 UTC+2, Maxim Uvarov wrote: >> >> On 03/18/2014 05:13 PM, Petri Savolainen wrote: >> > There was a race condition with the "in" variable. This patch >> > uses more robust barrier exit synchronization to avoid the race. >> > >> > Signed-off-by: Petri Savolainen <petri.sa...@linaro.org >> <javascript:>> >> >> > --- >> > include/odp_barrier.h | 5 ++- >> > platform/linux-generic/source/odp_barrier.c | 52 >> +++++++++++++++++++++-------- >> > test/example/odp_example.c | 4 +++ >> > 3 files changed, 47 insertions(+), 14 deletions(-) >> > >> > diff --git a/include/odp_barrier.h b/include/odp_barrier.h >> > index bb4a6c5..19dfc06 100644 >> > --- a/include/odp_barrier.h >> > +++ b/include/odp_barrier.h >> > @@ -28,8 +28,11 @@ extern "C" { >> > */ >> > typedef struct odp_barrier_t { >> > int count; >> > + volatile int wait; >> > + volatile int wait_exit; >> is there any reason to use volatile int instead of odp_atomic_int_t ? >> >> Because those are not atomic counters, but status variables. >> >> >> Please also run ./scripts/cleanfile to remove DOS endings in the >> patch. >> And do clean ups >> ./scripts/checkpatch.pl <http://checkpatch.pl> warnings. >> >> >> >> What DOS endings? Checkpatch didn't show other warnings than volatiles. >> >> -Petri >> > I don't know why but this email has ^M at the end of the lines. However > git am cleanly removes them. Don't worry about that. > where did you see them? specify the lines please, I could not see any ^M... Cheers, Anders > > Best regards, > Maxim. > > -- >> You received this message because you are subscribed to the Google Groups >> "LNG ODP Sub-team - lng-odp@linaro.org" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to lng-odp+unsubscribe@linaro.org <mailto:lng-odp+unsubscribe@ >> linaro.org>. >> To post to this group, send email to lng-odp@linaro.org <mailto: >> lng-odp@linaro.org>. >> >> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. >> To view this discussion on the web visit https://groups.google.com/a/ >> linaro.org/d/msgid/lng-odp/964440bc-af68-4841-aa1b- >> 3752170f66f7%40linaro.org <https://groups.google.com/a/ >> linaro.org/d/msgid/lng-odp/964440bc-af68-4841-aa1b- >> 3752170f66f7%40linaro.org?utm_medium=email&utm_source=footer>. >> >> For more options, visit https://groups.google.com/a/linaro.org/d/optout. >> > > -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit https://groups.google.com/a/ > linaro.org/d/msgid/lng-odp/53285207.40704%40linaro.org. > > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
On 03/18/2014 06:07 PM, Anders Roxell wrote: > > > > On 18 March 2014 15:02, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 03/18/2014 05:46 PM, Petri Savolainen wrote: > > > > On Tuesday, 18 March 2014 15:31:06 UTC+2, Maxim Uvarov wrote: > > On 03/18/2014 05:13 PM, Petri Savolainen wrote: > > There was a race condition with the "in" variable. This > patch > > uses more robust barrier exit synchronization to avoid > the race. > > > > Signed-off-by: Petri Savolainen <petri.sa...@linaro.org > <mailto:petri.sa...@linaro.org> > <javascript:>> > > > --- > > include/odp_barrier.h | 5 ++- > > platform/linux-generic/source/odp_barrier.c | 52 > +++++++++++++++++++++-------- > > test/example/odp_example.c | 4 +++ > > 3 files changed, 47 insertions(+), 14 deletions(-) > > > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > > index bb4a6c5..19dfc06 100644 > > --- a/include/odp_barrier.h > > +++ b/include/odp_barrier.h > > @@ -28,8 +28,11 @@ extern "C" { > > */ > > typedef struct odp_barrier_t { > > int count; > > + volatile int wait; > > + volatile int wait_exit; > is there any reason to use volatile int instead of > odp_atomic_int_t ? > > Because those are not atomic counters, but status variables. > > > Please also run ./scripts/cleanfile to remove DOS endings > in the > patch. > And do clean ups > ./scripts/checkpatch.pl <http://checkpatch.pl> > <http://checkpatch.pl> warnings. > > > > What DOS endings? Checkpatch didn't show other warnings than > volatiles. > > -Petri > > I don't know why but this email has ^M at the end of the lines. > However git am cleanly removes them. Don't worry about that. > > > where did you see them? > specify the lines please, I could not see any ^M... > > Cheers, > Anders bunch of places. But after I git am this patch and do git format-patch again ending ^M disappear. So that is not the problem: ERROR: DOS line endings #190: FILE: test/example/odp_example.c:510: +^Iodp_barrier_sync(&test_barrier);^M$ ERROR: DOS line endings #191: FILE: test/example/odp_example.c:511: +^Iodp_barrier_sync(&test_barrier);^M$ ERROR: DOS line endings #192: FILE: test/example/odp_example.c:512: +^Iodp_barrier_sync(&test_barrier);^M$ > > Best regards, > Maxim. > > -- > You received this message because you are subscribed to the > Google Groups "LNG ODP Sub-team - lng-odp@linaro.org > <mailto:lng-odp@linaro.org>" group. > To unsubscribe from this group and stop receiving emails from > it, send an email to lng-odp+unsubscribe@linaro.org > <mailto:lng-odp%2Bunsubscribe@linaro.org> > <mailto:lng-odp+unsubscribe@linaro.org > <mailto:lng-odp%2Bunsubscribe@linaro.org>>. > To post to this group, send email to lng-odp@linaro.org > <mailto:lng-odp@linaro.org> <mailto:lng-odp@linaro.org > <mailto:lng-odp@linaro.org>>. > > Visit this group at > http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/964440bc-af68-4841-aa1b-3752170f66f7%40linaro.org > <https://groups.google.com/a/linaro.org/d/msgid/lng-odp/964440bc-af68-4841-aa1b-3752170f66f7%40linaro.org?utm_medium=email&utm_source=footer>. > > > For more options, visit > https://groups.google.com/a/linaro.org/d/optout. > > > -- > You received this message because you are subscribed to the Google > Groups "LNG ODP Sub-team - lng-odp@linaro.org > <mailto:lng-odp@linaro.org>" group. > To unsubscribe from this group and stop receiving emails from it, > send an email to lng-odp+unsubscribe@linaro.org > <mailto:lng-odp%2Bunsubscribe@linaro.org>. > To post to this group, send email to lng-odp@linaro.org > <mailto:lng-odp@linaro.org>. > Visit this group at > http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/53285207.40704%40linaro.org. > > > For more options, visit > https://groups.google.com/a/linaro.org/d/optout. > >
On Tue, Mar 18, 2014 at 01:13:07PM +0000, Petri Savolainen wrote: > There was a race condition with the "in" variable. This patch > uses more robust barrier exit synchronization to avoid the race. > This patch does seem to fix the race, but it feels a little over engineered, some comments below.. > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > include/odp_barrier.h | 5 ++- > platform/linux-generic/source/odp_barrier.c | 52 +++++++++++++++++++++-------- > test/example/odp_example.c | 4 +++ > 3 files changed, 47 insertions(+), 14 deletions(-) > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > index bb4a6c5..19dfc06 100644 > --- a/include/odp_barrier.h > +++ b/include/odp_barrier.h > @@ -28,8 +28,11 @@ extern "C" { > */ > typedef struct odp_barrier_t { > int count; > + volatile int wait; > + volatile int wait_exit; > odp_atomic_int_t in; > - odp_atomic_int_t out; > + odp_atomic_int_t waiting; > + odp_atomic_int_t waiting_exit; > } odp_barrier_t; > This is quite complicated and specific given that it's part of the external API. > > diff --git a/platform/linux-generic/source/odp_barrier.c b/platform/linux-generic/source/odp_barrier.c > index 64fbdb9..b27afc7 100644 > --- a/platform/linux-generic/source/odp_barrier.c > +++ b/platform/linux-generic/source/odp_barrier.c > @@ -10,9 +10,13 @@ > > void odp_barrier_init_count(odp_barrier_t *barrier, int count) > { > - barrier->count = count; > - barrier->in = 0; > - barrier->out = count - 1; > + barrier->count = count; > + barrier->wait = 1; > + barrier->wait_exit = 0; > + barrier->in = 0; > + barrier->waiting = 0; > + barrier->waiting_exit = 0; > + > odp_sync_stores(); > } > > @@ -26,25 +30,47 @@ void odp_barrier_sync(odp_barrier_t *barrier) > count = odp_atomic_fetch_inc_int(&barrier->in); > > if (count == barrier->count - 1) { > - /* If last thread, release others */ > - barrier->in = 0; > + /* > + * This is the last thread. > + * Spin until other threads are waiting. > + */ > + while (barrier->waiting != barrier->count-1) > + odp_spin(); I don't think barrier->waiting is needed at all, how would you reach here without the other threads already waiting? > + > + /* Reset barrier entry */ > + barrier->wait_exit = 1; > + barrier->in = 0; > + barrier->waiting = 0; > + barrier->waiting_exit = 0; > odp_sync_stores(); > > - /* Wait for others to exit */ > - while (barrier->out) > + /* Move other threads to wait for exit */ > + barrier->wait = 0; > + > + /* Spin until other threads are waiting for exit */ > + while (barrier->waiting_exit != barrier->count-1) > odp_spin(); Also not sure why this is needed, why not decrement ->in where you're currently incrementing ->waiting_exit and check here for ->in != 1? > > - /* Ready, reset out counter */ > - barrier->out = barrier->count - 1; > + /* Reset barrier exit */ > + barrier->waiting_exit = 0; > + barrier->wait = 1; > odp_sync_stores(); > > + /* Let other threads to exit */ > + barrier->wait_exit = 0; > + > + odp_sync_stores(); > } else { > - /* Wait for the last thread*/ > - while (barrier->in) > + odp_atomic_inc_int(&barrier->waiting); > + > + while (barrier->wait) > + odp_spin(); > + > + odp_atomic_inc_int(&barrier->waiting_exit); > + > + while (barrier->wait_exit) > odp_spin(); > > - /* Ready */ > - odp_atomic_dec_int(&barrier->out); > odp_mem_barrier(); > } > } > diff --git a/test/example/odp_example.c b/test/example/odp_example.c > index 4764657..a6ab89e 100644 > --- a/test/example/odp_example.c > +++ b/test/example/odp_example.c > @@ -507,6 +507,10 @@ static void *run_thread(void *arg) > odp_barrier_sync(&test_barrier); > odp_barrier_sync(&test_barrier); > odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > + odp_barrier_sync(&test_barrier); > > /* > * Find the buffer pool > -- > 1.8.5.3 >
I've submitted alternate fix for the reported bug which is also simpler and more efficient. It should be used instead of this patch. Bill On Tue, Mar 18, 2014 at 2:09 PM, Stuart Haslam <stuart.haslam@arm.com>wrote: > On Tue, Mar 18, 2014 at 01:13:07PM +0000, Petri Savolainen wrote: > > There was a race condition with the "in" variable. This patch > > uses more robust barrier exit synchronization to avoid the race. > > > > This patch does seem to fix the race, but it feels a little over > engineered, some comments below.. > > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > > --- > > include/odp_barrier.h | 5 ++- > > platform/linux-generic/source/odp_barrier.c | 52 > +++++++++++++++++++++-------- > > test/example/odp_example.c | 4 +++ > > 3 files changed, 47 insertions(+), 14 deletions(-) > > > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > > index bb4a6c5..19dfc06 100644 > > --- a/include/odp_barrier.h > > +++ b/include/odp_barrier.h > > @@ -28,8 +28,11 @@ extern "C" { > > */ > > typedef struct odp_barrier_t { > > int count; > > + volatile int wait; > > + volatile int wait_exit; > > odp_atomic_int_t in; > > - odp_atomic_int_t out; > > + odp_atomic_int_t waiting; > > + odp_atomic_int_t waiting_exit; > > } odp_barrier_t; > > > > This is quite complicated and specific given that it's part of the > external API. > > > > > diff --git a/platform/linux-generic/source/odp_barrier.c > b/platform/linux-generic/source/odp_barrier.c > > index 64fbdb9..b27afc7 100644 > > --- a/platform/linux-generic/source/odp_barrier.c > > +++ b/platform/linux-generic/source/odp_barrier.c > > @@ -10,9 +10,13 @@ > > > > void odp_barrier_init_count(odp_barrier_t *barrier, int count) > > { > > - barrier->count = count; > > - barrier->in = 0; > > - barrier->out = count - 1; > > + barrier->count = count; > > + barrier->wait = 1; > > + barrier->wait_exit = 0; > > + barrier->in = 0; > > + barrier->waiting = 0; > > + barrier->waiting_exit = 0; > > + > > odp_sync_stores(); > > } > > > > @@ -26,25 +30,47 @@ void odp_barrier_sync(odp_barrier_t *barrier) > > count = odp_atomic_fetch_inc_int(&barrier->in); > > > > if (count == barrier->count - 1) { > > - /* If last thread, release others */ > > - barrier->in = 0; > > + /* > > + * This is the last thread. > > + * Spin until other threads are waiting. > > + */ > > + while (barrier->waiting != barrier->count-1) > > + odp_spin(); > > I don't think barrier->waiting is needed at all, how would you reach > here without the other threads already waiting? > > > + > > + /* Reset barrier entry */ > > + barrier->wait_exit = 1; > > + barrier->in = 0; > > + barrier->waiting = 0; > > + barrier->waiting_exit = 0; > > odp_sync_stores(); > > > > - /* Wait for others to exit */ > > - while (barrier->out) > > + /* Move other threads to wait for exit */ > > + barrier->wait = 0; > > + > > + /* Spin until other threads are waiting for exit */ > > + while (barrier->waiting_exit != barrier->count-1) > > odp_spin(); > > Also not sure why this is needed, why not decrement ->in where you're > currently incrementing ->waiting_exit and check here for ->in != 1? > > > > > - /* Ready, reset out counter */ > > - barrier->out = barrier->count - 1; > > + /* Reset barrier exit */ > > + barrier->waiting_exit = 0; > > + barrier->wait = 1; > > odp_sync_stores(); > > > > + /* Let other threads to exit */ > > + barrier->wait_exit = 0; > > + > > + odp_sync_stores(); > > } else { > > - /* Wait for the last thread*/ > > - while (barrier->in) > > + odp_atomic_inc_int(&barrier->waiting); > > + > > + while (barrier->wait) > > + odp_spin(); > > + > > + odp_atomic_inc_int(&barrier->waiting_exit); > > + > > + while (barrier->wait_exit) > > odp_spin(); > > > > - /* Ready */ > > - odp_atomic_dec_int(&barrier->out); > > odp_mem_barrier(); > > } > > } > > diff --git a/test/example/odp_example.c b/test/example/odp_example.c > > index 4764657..a6ab89e 100644 > > --- a/test/example/odp_example.c > > +++ b/test/example/odp_example.c > > @@ -507,6 +507,10 @@ static void *run_thread(void *arg) > > odp_barrier_sync(&test_barrier); > > odp_barrier_sync(&test_barrier); > > odp_barrier_sync(&test_barrier); > > + odp_barrier_sync(&test_barrier); > > + odp_barrier_sync(&test_barrier); > > + odp_barrier_sync(&test_barrier); > > + odp_barrier_sync(&test_barrier); > > > > /* > > * Find the buffer pool > > -- > > 1.8.5.3 > > > > -- > Stuart. > > -- > You received this message because you are subscribed to the Google Groups > "LNG ODP Sub-team - lng-odp@linaro.org" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to lng-odp+unsubscribe@linaro.org. > To post to this group, send email to lng-odp@linaro.org. > Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/. > To view this discussion on the web visit > https://groups.google.com/a/linaro.org/d/msgid/lng-odp/20140318190951.GA28006%40e106441 > . > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
diff --git a/include/odp_barrier.h b/include/odp_barrier.h index bb4a6c5..19dfc06 100644 --- a/include/odp_barrier.h +++ b/include/odp_barrier.h @@ -28,8 +28,11 @@ extern "C" { */ typedef struct odp_barrier_t { int count; + volatile int wait; + volatile int wait_exit; odp_atomic_int_t in; - odp_atomic_int_t out; + odp_atomic_int_t waiting; + odp_atomic_int_t waiting_exit; } odp_barrier_t; diff --git a/platform/linux-generic/source/odp_barrier.c b/platform/linux-generic/source/odp_barrier.c index 64fbdb9..b27afc7 100644 --- a/platform/linux-generic/source/odp_barrier.c +++ b/platform/linux-generic/source/odp_barrier.c @@ -10,9 +10,13 @@ void odp_barrier_init_count(odp_barrier_t *barrier, int count) { - barrier->count = count; - barrier->in = 0; - barrier->out = count - 1; + barrier->count = count; + barrier->wait = 1; + barrier->wait_exit = 0; + barrier->in = 0; + barrier->waiting = 0; + barrier->waiting_exit = 0; + odp_sync_stores(); } @@ -26,25 +30,47 @@ void odp_barrier_sync(odp_barrier_t *barrier) count = odp_atomic_fetch_inc_int(&barrier->in); if (count == barrier->count - 1) { - /* If last thread, release others */ - barrier->in = 0; + /* + * This is the last thread. + * Spin until other threads are waiting. + */ + while (barrier->waiting != barrier->count-1) + odp_spin(); + + /* Reset barrier entry */ + barrier->wait_exit = 1; + barrier->in = 0; + barrier->waiting = 0; + barrier->waiting_exit = 0; odp_sync_stores(); - /* Wait for others to exit */ - while (barrier->out) + /* Move other threads to wait for exit */ + barrier->wait = 0; + + /* Spin until other threads are waiting for exit */ + while (barrier->waiting_exit != barrier->count-1) odp_spin(); - /* Ready, reset out counter */ - barrier->out = barrier->count - 1; + /* Reset barrier exit */ + barrier->waiting_exit = 0; + barrier->wait = 1; odp_sync_stores(); + /* Let other threads to exit */ + barrier->wait_exit = 0; + + odp_sync_stores(); } else { - /* Wait for the last thread*/ - while (barrier->in) + odp_atomic_inc_int(&barrier->waiting); + + while (barrier->wait) + odp_spin(); + + odp_atomic_inc_int(&barrier->waiting_exit); + + while (barrier->wait_exit) odp_spin(); - /* Ready */ - odp_atomic_dec_int(&barrier->out); odp_mem_barrier(); } } diff --git a/test/example/odp_example.c b/test/example/odp_example.c index 4764657..a6ab89e 100644 --- a/test/example/odp_example.c +++ b/test/example/odp_example.c @@ -507,6 +507,10 @@ static void *run_thread(void *arg) odp_barrier_sync(&test_barrier); odp_barrier_sync(&test_barrier); odp_barrier_sync(&test_barrier); + odp_barrier_sync(&test_barrier); + odp_barrier_sync(&test_barrier); + odp_barrier_sync(&test_barrier); + odp_barrier_sync(&test_barrier); /* * Find the buffer pool
There was a race condition with the "in" variable. This patch uses more robust barrier exit synchronization to avoid the race. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- include/odp_barrier.h | 5 ++- platform/linux-generic/source/odp_barrier.c | 52 +++++++++++++++++++++-------- test/example/odp_example.c | 4 +++ 3 files changed, 47 insertions(+), 14 deletions(-)