| Message ID | 1395183533-5328-2-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Tried using the --compose option on git send-email and that obviously didn't work. This is a corrected/streamlined patch for the ODP barrier functions. The odp_barrier_t type is also simplified as the only variables that need to be tracked are the count and the barrier value which cycles from 0..2*count-1 to avoid race conditions and permit full reusability. This patch should be used instead of the patch that Petri submitted yesterday. Bill On Tue, Mar 18, 2014 at 5:58 PM, Bill Fischofer <bill.fischofer@linaro.org>wrote: > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > include/odp_barrier.h | 3 +- > platform/linux-generic/source/odp_barrier.c | 48 > +++++++++++++---------------- > 2 files changed, 23 insertions(+), 28 deletions(-) > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > index bb4a6c5..0a1404b 100644 > --- a/include/odp_barrier.h > +++ b/include/odp_barrier.h > @@ -28,8 +28,7 @@ extern "C" { > */ > typedef struct odp_barrier_t { > int count; > - odp_atomic_int_t in; > - odp_atomic_int_t out; > + odp_atomic_int_t bar; > } odp_barrier_t; > > > diff --git a/platform/linux-generic/source/odp_barrier.c > b/platform/linux-generic/source/odp_barrier.c > index 64fbdb9..fc0f13f 100644 > --- a/platform/linux-generic/source/odp_barrier.c > +++ b/platform/linux-generic/source/odp_barrier.c > @@ -11,40 +11,36 @@ > void odp_barrier_init_count(odp_barrier_t *barrier, int count) > { > barrier->count = count; > - barrier->in = 0; > - barrier->out = count - 1; > - odp_sync_stores(); > + barrier->bar = 0; > } > > +/* > + * Efficient barrier_sync - > + * > + * Barriers are initialized with a count of the number of callers > + * that must sync on the barrier before any may proceed. > + * > + * To avoid race conditions and to permit the barrier to be fully > + * reusable, the barrier value cycles between 0..2*count-1. When > + * synchronizing the wasless variable simply tracks which half of > + * the cycle the barrier was in upon entry. Exit is when the > + * barrier crosses to the other half of the cycle. > + */ > > void odp_barrier_sync(odp_barrier_t *barrier) > { > int count; > + int wasless; > > - odp_sync_stores(); > - > - count = odp_atomic_fetch_inc_int(&barrier->in); > - > - if (count == barrier->count - 1) { > - /* If last thread, release others */ > - barrier->in = 0; > - odp_sync_stores(); > - > - /* Wait for others to exit */ > - while (barrier->out) > - odp_spin(); > - > - /* Ready, reset out counter */ > - barrier->out = barrier->count - 1; > - odp_sync_stores(); > + wasless = barrier->bar < barrier->count; > + count = odp_atomic_fetch_inc_int(&barrier->bar); > > + if (count == 2*barrier->count-1) { > + barrier->bar = 0; > } else { > - /* Wait for the last thread*/ > - while (barrier->in) > - odp_spin(); > - > - /* Ready */ > - odp_atomic_dec_int(&barrier->out); > - odp_mem_barrier(); > + while ((barrier->bar < barrier->count) == wasless) { > + odp_spin(); > + } > } > + > } > -- > 1.8.3.2 > >
On 03/18/2014 06:58 PM, Bill Fischofer wrote: > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > include/odp_barrier.h | 3 +- > platform/linux-generic/source/odp_barrier.c | 48 +++++++++++++---------------- > 2 files changed, 23 insertions(+), 28 deletions(-) > > diff --git a/include/odp_barrier.h b/include/odp_barrier.h > index bb4a6c5..0a1404b 100644 > --- a/include/odp_barrier.h > +++ b/include/odp_barrier.h > @@ -28,8 +28,7 @@ extern "C" { > */ > typedef struct odp_barrier_t { > int count; > - odp_atomic_int_t in; > - odp_atomic_int_t out; > + odp_atomic_int_t bar; > } odp_barrier_t; > > > diff --git a/platform/linux-generic/source/odp_barrier.c b/platform/linux-generic/source/odp_barrier.c > index 64fbdb9..fc0f13f 100644 > --- a/platform/linux-generic/source/odp_barrier.c > +++ b/platform/linux-generic/source/odp_barrier.c > @@ -11,40 +11,36 @@ > void odp_barrier_init_count(odp_barrier_t *barrier, int count) > { > barrier->count = count; > - barrier->in = 0; > - barrier->out = count - 1; > - odp_sync_stores(); > + barrier->bar = 0; > } > > +/* > + * Efficient barrier_sync - > + * > + * Barriers are initialized with a count of the number of callers > + * that must sync on the barrier before any may proceed. > + * > + * To avoid race conditions and to permit the barrier to be fully > + * reusable, the barrier value cycles between 0..2*count-1. When > + * synchronizing the wasless variable simply tracks which half of > + * the cycle the barrier was in upon entry. Exit is when the > + * barrier crosses to the other half of the cycle. > + */ > > void odp_barrier_sync(odp_barrier_t *barrier) > { > int count; > + int wasless; > > - odp_sync_stores(); > - > - count = odp_atomic_fetch_inc_int(&barrier->in); > - > - if (count == barrier->count - 1) { > - /* If last thread, release others */ > - barrier->in = 0; > - odp_sync_stores(); > - > - /* Wait for others to exit */ > - while (barrier->out) > - odp_spin(); > - > - /* Ready, reset out counter */ > - barrier->out = barrier->count - 1; > - odp_sync_stores(); > + wasless = barrier->bar < barrier->count; > + count = odp_atomic_fetch_inc_int(&barrier->bar); > > + if (count == 2*barrier->count-1) { > + barrier->bar = 0; > } else { > - /* Wait for the last thread*/ > - while (barrier->in) > - odp_spin(); > - > - /* Ready */ > - odp_atomic_dec_int(&barrier->out); > - odp_mem_barrier(); > + while ((barrier->bar < barrier->count) == wasless) { > + odp_spin(); > + } > } > + > } I got some warnings with odp/scripts/checkpatch on the raw patch. And after patching, running odp/scripts/odp_check on the patched files themselves also generated warnings. WARNING: braces {} are not necessary for single statement blocks #41: FILE: linux-generic/source/odp_barrier.c:41: + while ((barrier->bar < barrier->count) == wasless) { + odp_spin(); + } CHECK: Blank lines aren't necessary before a close brace '}' #46: FILE: linux-generic/source/odp_barrier.c:46: + +} total: 0 errors, 1 warnings, 1 checks, 46 lines checked NOTE: Ignored message types: DEPRECATED_VARIABLE NEW_TYPEDEFS platform/linux-generic/source/odp_barrier.c has style problems, please review. If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
I saw those but I thought we decided that we wanted to use unnecessary braces for singleton blocks. I've corrected these and resubmitted the patch. Bill On Tue, Mar 18, 2014 at 6:45 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > On 03/18/2014 06:58 PM, Bill Fischofer wrote: > >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> include/odp_barrier.h | 3 +- >> platform/linux-generic/source/odp_barrier.c | 48 >> +++++++++++++---------------- >> 2 files changed, 23 insertions(+), 28 deletions(-) >> >> diff --git a/include/odp_barrier.h b/include/odp_barrier.h >> index bb4a6c5..0a1404b 100644 >> --- a/include/odp_barrier.h >> +++ b/include/odp_barrier.h >> @@ -28,8 +28,7 @@ extern "C" { >> */ >> typedef struct odp_barrier_t { >> int count; >> - odp_atomic_int_t in; >> - odp_atomic_int_t out; >> + odp_atomic_int_t bar; >> } odp_barrier_t; >> diff --git a/platform/linux-generic/source/odp_barrier.c >> b/platform/linux-generic/source/odp_barrier.c >> index 64fbdb9..fc0f13f 100644 >> --- a/platform/linux-generic/source/odp_barrier.c >> +++ b/platform/linux-generic/source/odp_barrier.c >> @@ -11,40 +11,36 @@ >> void odp_barrier_init_count(odp_barrier_t *barrier, int count) >> { >> barrier->count = count; >> - barrier->in = 0; >> - barrier->out = count - 1; >> - odp_sync_stores(); >> + barrier->bar = 0; >> } >> +/* >> + * Efficient barrier_sync - >> + * >> + * Barriers are initialized with a count of the number of callers >> + * that must sync on the barrier before any may proceed. >> + * >> + * To avoid race conditions and to permit the barrier to be fully >> + * reusable, the barrier value cycles between 0..2*count-1. When >> + * synchronizing the wasless variable simply tracks which half of >> + * the cycle the barrier was in upon entry. Exit is when the >> + * barrier crosses to the other half of the cycle. >> + */ >> void odp_barrier_sync(odp_barrier_t *barrier) >> { >> int count; >> + int wasless; >> - odp_sync_stores(); >> - >> - count = odp_atomic_fetch_inc_int(&barrier->in); >> - >> - if (count == barrier->count - 1) { >> - /* If last thread, release others */ >> - barrier->in = 0; >> - odp_sync_stores(); >> - >> - /* Wait for others to exit */ >> - while (barrier->out) >> - odp_spin(); >> - >> - /* Ready, reset out counter */ >> - barrier->out = barrier->count - 1; >> - odp_sync_stores(); >> + wasless = barrier->bar < barrier->count; >> + count = odp_atomic_fetch_inc_int(&barrier->bar); >> + if (count == 2*barrier->count-1) { >> + barrier->bar = 0; >> } else { >> - /* Wait for the last thread*/ >> - while (barrier->in) >> - odp_spin(); >> - >> - /* Ready */ >> - odp_atomic_dec_int(&barrier->out); >> - odp_mem_barrier(); >> + while ((barrier->bar < barrier->count) == wasless) { >> + odp_spin(); >> + } >> } >> + >> } >> > I got some warnings with odp/scripts/checkpatch on the raw patch. And > after patching, running odp/scripts/odp_check on the patched files > themselves also generated warnings. > > WARNING: braces {} are not necessary for single statement blocks > #41: FILE: linux-generic/source/odp_barrier.c:41: > > + while ((barrier->bar < barrier->count) == wasless) { > + odp_spin(); > + } > > CHECK: Blank lines aren't necessary before a close brace '}' > #46: FILE: linux-generic/source/odp_barrier.c:46: > + > +} > > total: 0 errors, 1 warnings, 1 checks, 46 lines checked > > NOTE: Ignored message types: DEPRECATED_VARIABLE NEW_TYPEDEFS > > platform/linux-generic/source/odp_barrier.c has style problems, please > review. > > If any of these errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > > -- > Mike Holmes > Technical Manager > Linaro Networking Group > Email: mike.holmes@linaro.org > IRC: mike-holmes > > -- > 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/5328DA9B.4040800%40linaro.org. > For more options, visit https://groups.google.com/a/linaro.org/d/optout. >
We should update checkpatch if we want to allow them. I also prefer the the braces, but I don't mind living with the current rules. On 18 March 2014 19:56, Bill Fischofer <bill.fischofer@linaro.org> wrote: > I saw those but I thought we decided that we wanted to use unnecessary > braces for singleton blocks. > > I've corrected these and resubmitted the patch. > > Bill > > > On Tue, Mar 18, 2014 at 6:45 PM, Mike Holmes <mike.holmes@linaro.org>wrote: > >> On 03/18/2014 06:58 PM, Bill Fischofer wrote: >> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>> --- >>> include/odp_barrier.h | 3 +- >>> platform/linux-generic/source/odp_barrier.c | 48 >>> +++++++++++++---------------- >>> 2 files changed, 23 insertions(+), 28 deletions(-) >>> >>> diff --git a/include/odp_barrier.h b/include/odp_barrier.h >>> index bb4a6c5..0a1404b 100644 >>> --- a/include/odp_barrier.h >>> +++ b/include/odp_barrier.h >>> @@ -28,8 +28,7 @@ extern "C" { >>> */ >>> typedef struct odp_barrier_t { >>> int count; >>> - odp_atomic_int_t in; >>> - odp_atomic_int_t out; >>> + odp_atomic_int_t bar; >>> } odp_barrier_t; >>> diff --git a/platform/linux-generic/source/odp_barrier.c >>> b/platform/linux-generic/source/odp_barrier.c >>> index 64fbdb9..fc0f13f 100644 >>> --- a/platform/linux-generic/source/odp_barrier.c >>> +++ b/platform/linux-generic/source/odp_barrier.c >>> @@ -11,40 +11,36 @@ >>> void odp_barrier_init_count(odp_barrier_t *barrier, int count) >>> { >>> barrier->count = count; >>> - barrier->in = 0; >>> - barrier->out = count - 1; >>> - odp_sync_stores(); >>> + barrier->bar = 0; >>> } >>> +/* >>> + * Efficient barrier_sync - >>> + * >>> + * Barriers are initialized with a count of the number of callers >>> + * that must sync on the barrier before any may proceed. >>> + * >>> + * To avoid race conditions and to permit the barrier to be fully >>> + * reusable, the barrier value cycles between 0..2*count-1. When >>> + * synchronizing the wasless variable simply tracks which half of >>> + * the cycle the barrier was in upon entry. Exit is when the >>> + * barrier crosses to the other half of the cycle. >>> + */ >>> void odp_barrier_sync(odp_barrier_t *barrier) >>> { >>> int count; >>> + int wasless; >>> - odp_sync_stores(); >>> - >>> - count = odp_atomic_fetch_inc_int(&barrier->in); >>> - >>> - if (count == barrier->count - 1) { >>> - /* If last thread, release others */ >>> - barrier->in = 0; >>> - odp_sync_stores(); >>> - >>> - /* Wait for others to exit */ >>> - while (barrier->out) >>> - odp_spin(); >>> - >>> - /* Ready, reset out counter */ >>> - barrier->out = barrier->count - 1; >>> - odp_sync_stores(); >>> + wasless = barrier->bar < barrier->count; >>> + count = odp_atomic_fetch_inc_int(&barrier->bar); >>> + if (count == 2*barrier->count-1) { >>> + barrier->bar = 0; >>> } else { >>> - /* Wait for the last thread*/ >>> - while (barrier->in) >>> - odp_spin(); >>> - >>> - /* Ready */ >>> - odp_atomic_dec_int(&barrier->out); >>> - odp_mem_barrier(); >>> + while ((barrier->bar < barrier->count) == wasless) { >>> + odp_spin(); >>> + } >>> } >>> + >>> } >>> >> I got some warnings with odp/scripts/checkpatch on the raw patch. And >> after patching, running odp/scripts/odp_check on the patched files >> themselves also generated warnings. >> >> WARNING: braces {} are not necessary for single statement blocks >> #41: FILE: linux-generic/source/odp_barrier.c:41: >> >> + while ((barrier->bar < barrier->count) == wasless) { >> + odp_spin(); >> + } >> >> CHECK: Blank lines aren't necessary before a close brace '}' >> #46: FILE: linux-generic/source/odp_barrier.c:46: >> + >> +} >> >> total: 0 errors, 1 warnings, 1 checks, 46 lines checked >> >> NOTE: Ignored message types: DEPRECATED_VARIABLE NEW_TYPEDEFS >> >> platform/linux-generic/source/odp_barrier.c has style problems, please >> review. >> >> If any of these errors are false positives, please report >> them to the maintainer, see CHECKPATCH in MAINTAINERS. >> >> >> >> -- >> Mike Holmes >> Technical Manager >> Linaro Networking Group >> Email: mike.holmes@linaro.org >> IRC: mike-holmes >> >> -- >> 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/5328DA9B.4040800%40linaro.org. >> >> 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..0a1404b 100644 --- a/include/odp_barrier.h +++ b/include/odp_barrier.h @@ -28,8 +28,7 @@ extern "C" { */ typedef struct odp_barrier_t { int count; - odp_atomic_int_t in; - odp_atomic_int_t out; + odp_atomic_int_t bar; } odp_barrier_t; diff --git a/platform/linux-generic/source/odp_barrier.c b/platform/linux-generic/source/odp_barrier.c index 64fbdb9..fc0f13f 100644 --- a/platform/linux-generic/source/odp_barrier.c +++ b/platform/linux-generic/source/odp_barrier.c @@ -11,40 +11,36 @@ void odp_barrier_init_count(odp_barrier_t *barrier, int count) { barrier->count = count; - barrier->in = 0; - barrier->out = count - 1; - odp_sync_stores(); + barrier->bar = 0; } +/* + * Efficient barrier_sync - + * + * Barriers are initialized with a count of the number of callers + * that must sync on the barrier before any may proceed. + * + * To avoid race conditions and to permit the barrier to be fully + * reusable, the barrier value cycles between 0..2*count-1. When + * synchronizing the wasless variable simply tracks which half of + * the cycle the barrier was in upon entry. Exit is when the + * barrier crosses to the other half of the cycle. + */ void odp_barrier_sync(odp_barrier_t *barrier) { int count; + int wasless; - odp_sync_stores(); - - count = odp_atomic_fetch_inc_int(&barrier->in); - - if (count == barrier->count - 1) { - /* If last thread, release others */ - barrier->in = 0; - odp_sync_stores(); - - /* Wait for others to exit */ - while (barrier->out) - odp_spin(); - - /* Ready, reset out counter */ - barrier->out = barrier->count - 1; - odp_sync_stores(); + wasless = barrier->bar < barrier->count; + count = odp_atomic_fetch_inc_int(&barrier->bar); + if (count == 2*barrier->count-1) { + barrier->bar = 0; } else { - /* Wait for the last thread*/ - while (barrier->in) - odp_spin(); - - /* Ready */ - odp_atomic_dec_int(&barrier->out); - odp_mem_barrier(); + while ((barrier->bar < barrier->count) == wasless) { + odp_spin(); + } } + }
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- include/odp_barrier.h | 3 +- platform/linux-generic/source/odp_barrier.c | 48 +++++++++++++---------------- 2 files changed, 23 insertions(+), 28 deletions(-)