[2/7] api: odp_barrier.h: signature change, use odp_atomic.h, atomic wrap-around fix

Message ID	1416484428-23849-3-git-send-email-ola.liljedahl@linaro.org
State	New
Headers	show Return-Path: <patchwork-forward+bncBDK5FOHVXUMBB4NMW6RQKGQEBWHGP5Q@linaro.org> Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.45 as permitted sender) client-ip=209.85.215.45; Received-SPF: none (google.com: lng-odp-bounces@lists.linaro.org does not designate permitted sender hosts) client-ip=54.225.227.206; From: Ola Liljedahl <ola.liljedahl@linaro.org> To: lng-odp@lists.linaro.org Date: Thu, 20 Nov 2014 12:53:43 +0100 Message-Id: <1416484428-23849-3-git-send-email-ola.liljedahl@linaro.org> In-Reply-To: <1416484428-23849-1-git-send-email-ola.liljedahl@linaro.org> References: <1416484428-23849-1-git-send-email-ola.liljedahl@linaro.org> Subject: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change, use odp_atomic.h, atomic wrap-around fix Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: lng-odp-bounces@lists.linaro.org Sender: lng-odp-bounces@lists.linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org

Message ID

1416484428-23849-3-git-send-email-ola.liljedahl@linaro.org

State

New

Headers

Received-SPF: pass (google.com: domain of
	patch+caf_=patchwork-forward=linaro.org@linaro.org designates
	209.85.215.45 as permitted sender) client-ip=209.85.215.45; 
Received-SPF: none (google.com: lng-odp-bounces@lists.linaro.org does not
	designate permitted sender hosts) client-ip=54.225.227.206; 
From: Ola Liljedahl <ola.liljedahl@linaro.org>
To: lng-odp@lists.linaro.org
Date: Thu, 20 Nov 2014 12:53:43 +0100
Message-Id: <1416484428-23849-3-git-send-email-ola.liljedahl@linaro.org>
In-Reply-To: <1416484428-23849-1-git-send-email-ola.liljedahl@linaro.org>
References: <1416484428-23849-1-git-send-email-ola.liljedahl@linaro.org>
Subject: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change,
	use odp_atomic.h, atomic wrap-around fix
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: lng-odp-bounces@lists.linaro.org
Sender: lng-odp-bounces@lists.linaro.org
Mailing-list: list patchwork-forward@linaro.org;
	contact patchwork-forward+owners@linaro.org

Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> --- platform/linux-generic/include/api/odp_barrier.h | 2 +- platform/linux-generic/odp_barrier.c | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-)

Comments

Savolainen, Petri (NSN - FI/Espoo) Nov. 21, 2014, 9:11 a.m. UTC | #1

> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
> Sent: Thursday, November 20, 2014 1:54 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change, use
> odp_atomic.h, atomic wrap-around fix
> 
> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
> ---
>  platform/linux-generic/include/api/odp_barrier.h |  2 +-
>  platform/linux-generic/odp_barrier.c             | 17 +++++++++--------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/platform/linux-generic/include/api/odp_barrier.h
> b/platform/linux-generic/include/api/odp_barrier.h
> index 1790ea3..03e889d 100644
> --- a/platform/linux-generic/include/api/odp_barrier.h
> +++ b/platform/linux-generic/include/api/odp_barrier.h
> @@ -42,7 +42,7 @@ typedef struct odp_barrier_t {
>   * @param barrier    Barrier
>   * @param count      Thread count
>   */
> -void odp_barrier_init(odp_barrier_t *barrier, int count);
> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count);
> 
> 
>  /**
> diff --git a/platform/linux-generic/odp_barrier.c b/platform/linux-
> generic/odp_barrier.c
> index 87be2a1..2f14e2f 100644
> --- a/platform/linux-generic/odp_barrier.c
> +++ b/platform/linux-generic/odp_barrier.c
> @@ -8,11 +8,10 @@
>  #include <odp_sync.h>
>  #include <odp_spin_internal.h>
> 
> -void odp_barrier_init(odp_barrier_t *barrier, int count)
> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count)

Signature should stay as int, since it matches e.g. odp_sys_core_count().

-Petri

>  {
>  	barrier->count = count;
> -	barrier->bar   = 0;
> -	odp_sync_stores();
> +	odp_atomic_init_u32(&barrier->bar, 0);
>  }
> 
>  /*
> @@ -33,16 +32,18 @@ void odp_barrier_wait(odp_barrier_t *barrier)
>  	uint32_t count;
>  	int wasless;
> 
> -	odp_sync_stores();
> -	wasless = barrier->bar < barrier->count;
> +	__atomic_thread_fence(__ATOMIC_SEQ_CST);
>  	count   = odp_atomic_fetch_inc_u32(&barrier->bar);
> +	wasless = count < barrier->count;
> 
>  	if (count == 2*barrier->count-1) {
> -		barrier->bar = 0;
> +		/* Wrap around *atomically* */
> +		odp_atomic_sub_u32(&barrier->bar, 2 * barrier->count);
>  	} else {
> -		while ((barrier->bar < barrier->count) == wasless)
> +		while ((odp_atomic_load_u32(&barrier->bar) < barrier->count)
> +				== wasless)
>  			odp_spin();
>  	}
> 
> -	odp_mem_barrier();
> +	__atomic_thread_fence(__ATOMIC_SEQ_CST);
>  }
> --
> 1.9.1
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp

Ola Liljedahl Nov. 21, 2014, 9:57 a.m. UTC | #2

And why does odp_sys_core_count() return int? Is there any case where
we need to return a negative value?

-- Ola


On 21 November 2014 10:11, Savolainen, Petri (NSN - FI/Espoo)
<petri.savolainen@nsn.com> wrote:
>
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
>> Sent: Thursday, November 20, 2014 1:54 PM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change, use
>> odp_atomic.h, atomic wrap-around fix
>>
>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>> ---
>>  platform/linux-generic/include/api/odp_barrier.h |  2 +-
>>  platform/linux-generic/odp_barrier.c             | 17 +++++++++--------
>>  2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/api/odp_barrier.h
>> b/platform/linux-generic/include/api/odp_barrier.h
>> index 1790ea3..03e889d 100644
>> --- a/platform/linux-generic/include/api/odp_barrier.h
>> +++ b/platform/linux-generic/include/api/odp_barrier.h
>> @@ -42,7 +42,7 @@ typedef struct odp_barrier_t {
>>   * @param barrier    Barrier
>>   * @param count      Thread count
>>   */
>> -void odp_barrier_init(odp_barrier_t *barrier, int count);
>> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count);
>>
>>
>>  /**
>> diff --git a/platform/linux-generic/odp_barrier.c b/platform/linux-
>> generic/odp_barrier.c
>> index 87be2a1..2f14e2f 100644
>> --- a/platform/linux-generic/odp_barrier.c
>> +++ b/platform/linux-generic/odp_barrier.c
>> @@ -8,11 +8,10 @@
>>  #include <odp_sync.h>
>>  #include <odp_spin_internal.h>
>>
>> -void odp_barrier_init(odp_barrier_t *barrier, int count)
>> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count)
>
> Signature should stay as int, since it matches e.g. odp_sys_core_count().
>
> -Petri
>
>>  {
>>       barrier->count = count;
>> -     barrier->bar   = 0;
>> -     odp_sync_stores();
>> +     odp_atomic_init_u32(&barrier->bar, 0);
>>  }
>>
>>  /*
>> @@ -33,16 +32,18 @@ void odp_barrier_wait(odp_barrier_t *barrier)
>>       uint32_t count;
>>       int wasless;
>>
>> -     odp_sync_stores();
>> -     wasless = barrier->bar < barrier->count;
>> +     __atomic_thread_fence(__ATOMIC_SEQ_CST);
>>       count   = odp_atomic_fetch_inc_u32(&barrier->bar);
>> +     wasless = count < barrier->count;
>>
>>       if (count == 2*barrier->count-1) {
>> -             barrier->bar = 0;
>> +             /* Wrap around *atomically* */
>> +             odp_atomic_sub_u32(&barrier->bar, 2 * barrier->count);
>>       } else {
>> -             while ((barrier->bar < barrier->count) == wasless)
>> +             while ((odp_atomic_load_u32(&barrier->bar) < barrier->count)
>> +                             == wasless)
>>                       odp_spin();
>>       }
>>
>> -     odp_mem_barrier();
>> +     __atomic_thread_fence(__ATOMIC_SEQ_CST);
>>  }
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp

Ola Liljedahl Nov. 21, 2014, 10:09 a.m. UTC | #3

The 'bar' variable in the barrier that counts the number of threads
currently waiting at the barrier is of type odp_atomic_u32_t, i.e. an
unsigned types. And we will compare 'bar' against 'count'. Should
'count' be unsigned but the function parameter used to initialize
'count' be signed (int)?

Maybe this is just a difference of philosophy or style, some like to
use int as the default type for anything scalar, I favor unsigned or
uint32_t for values that have reason for being negative.

-- Ola


On 21 November 2014 10:57, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
> And why does odp_sys_core_count() return int? Is there any case where
> we need to return a negative value?
>
> -- Ola
>
>
> On 21 November 2014 10:11, Savolainen, Petri (NSN - FI/Espoo)
> <petri.savolainen@nsn.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>>> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
>>> Sent: Thursday, November 20, 2014 1:54 PM
>>> To: lng-odp@lists.linaro.org
>>> Subject: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change, use
>>> odp_atomic.h, atomic wrap-around fix
>>>
>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>>> ---
>>>  platform/linux-generic/include/api/odp_barrier.h |  2 +-
>>>  platform/linux-generic/odp_barrier.c             | 17 +++++++++--------
>>>  2 files changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/platform/linux-generic/include/api/odp_barrier.h
>>> b/platform/linux-generic/include/api/odp_barrier.h
>>> index 1790ea3..03e889d 100644
>>> --- a/platform/linux-generic/include/api/odp_barrier.h
>>> +++ b/platform/linux-generic/include/api/odp_barrier.h
>>> @@ -42,7 +42,7 @@ typedef struct odp_barrier_t {
>>>   * @param barrier    Barrier
>>>   * @param count      Thread count
>>>   */
>>> -void odp_barrier_init(odp_barrier_t *barrier, int count);
>>> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count);
>>>
>>>
>>>  /**
>>> diff --git a/platform/linux-generic/odp_barrier.c b/platform/linux-
>>> generic/odp_barrier.c
>>> index 87be2a1..2f14e2f 100644
>>> --- a/platform/linux-generic/odp_barrier.c
>>> +++ b/platform/linux-generic/odp_barrier.c
>>> @@ -8,11 +8,10 @@
>>>  #include <odp_sync.h>
>>>  #include <odp_spin_internal.h>
>>>
>>> -void odp_barrier_init(odp_barrier_t *barrier, int count)
>>> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count)
>>
>> Signature should stay as int, since it matches e.g. odp_sys_core_count().
>>
>> -Petri
>>
>>>  {
>>>       barrier->count = count;
>>> -     barrier->bar   = 0;
>>> -     odp_sync_stores();
>>> +     odp_atomic_init_u32(&barrier->bar, 0);
>>>  }
>>>
>>>  /*
>>> @@ -33,16 +32,18 @@ void odp_barrier_wait(odp_barrier_t *barrier)
>>>       uint32_t count;
>>>       int wasless;
>>>
>>> -     odp_sync_stores();
>>> -     wasless = barrier->bar < barrier->count;
>>> +     __atomic_thread_fence(__ATOMIC_SEQ_CST);
>>>       count   = odp_atomic_fetch_inc_u32(&barrier->bar);
>>> +     wasless = count < barrier->count;
>>>
>>>       if (count == 2*barrier->count-1) {
>>> -             barrier->bar = 0;
>>> +             /* Wrap around *atomically* */
>>> +             odp_atomic_sub_u32(&barrier->bar, 2 * barrier->count);
>>>       } else {
>>> -             while ((barrier->bar < barrier->count) == wasless)
>>> +             while ((odp_atomic_load_u32(&barrier->bar) < barrier->count)
>>> +                             == wasless)
>>>                       odp_spin();
>>>       }
>>>
>>> -     odp_mem_barrier();
>>> +     __atomic_thread_fence(__ATOMIC_SEQ_CST);
>>>  }
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp

Savolainen, Petri (NSN - FI/Espoo) Nov. 21, 2014, 10:21 a.m. UTC | #4

Maybe not (not even 0), but the point is that the same type should be used for core/thread counts over all the APIs. Now it's int. It's another task to change that if needed.

-Petri  

> -----Original Message-----
> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
> Sent: Friday, November 21, 2014 11:58 AM
> To: Savolainen, Petri (NSN - FI/Espoo)
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change,
> use odp_atomic.h, atomic wrap-around fix
> 
> And why does odp_sys_core_count() return int? Is there any case where
> we need to return a negative value?
> 
> -- Ola
> 
> 
> On 21 November 2014 10:11, Savolainen, Petri (NSN - FI/Espoo)
> <petri.savolainen@nsn.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> >> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
> >> Sent: Thursday, November 20, 2014 1:54 PM
> >> To: lng-odp@lists.linaro.org
> >> Subject: [lng-odp] [PATCH 2/7] api: odp_barrier.h: signature change,
> use
> >> odp_atomic.h, atomic wrap-around fix
> >>
> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
> >> ---
> >>  platform/linux-generic/include/api/odp_barrier.h |  2 +-
> >>  platform/linux-generic/odp_barrier.c             | 17 +++++++++-------
> -
> >>  2 files changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/platform/linux-generic/include/api/odp_barrier.h
> >> b/platform/linux-generic/include/api/odp_barrier.h
> >> index 1790ea3..03e889d 100644
> >> --- a/platform/linux-generic/include/api/odp_barrier.h
> >> +++ b/platform/linux-generic/include/api/odp_barrier.h
> >> @@ -42,7 +42,7 @@ typedef struct odp_barrier_t {
> >>   * @param barrier    Barrier
> >>   * @param count      Thread count
> >>   */
> >> -void odp_barrier_init(odp_barrier_t *barrier, int count);
> >> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count);
> >>
> >>
> >>  /**
> >> diff --git a/platform/linux-generic/odp_barrier.c b/platform/linux-
> >> generic/odp_barrier.c
> >> index 87be2a1..2f14e2f 100644
> >> --- a/platform/linux-generic/odp_barrier.c
> >> +++ b/platform/linux-generic/odp_barrier.c
> >> @@ -8,11 +8,10 @@
> >>  #include <odp_sync.h>
> >>  #include <odp_spin_internal.h>
> >>
> >> -void odp_barrier_init(odp_barrier_t *barrier, int count)
> >> +void odp_barrier_init(odp_barrier_t *barrier, uint32_t count)
> >
> > Signature should stay as int, since it matches e.g.
> odp_sys_core_count().
> >
> > -Petri
> >
> >>  {
> >>       barrier->count = count;
> >> -     barrier->bar   = 0;
> >> -     odp_sync_stores();
> >> +     odp_atomic_init_u32(&barrier->bar, 0);
> >>  }
> >>
> >>  /*
> >> @@ -33,16 +32,18 @@ void odp_barrier_wait(odp_barrier_t *barrier)
> >>       uint32_t count;
> >>       int wasless;
> >>
> >> -     odp_sync_stores();
> >> -     wasless = barrier->bar < barrier->count;
> >> +     __atomic_thread_fence(__ATOMIC_SEQ_CST);
> >>       count   = odp_atomic_fetch_inc_u32(&barrier->bar);
> >> +     wasless = count < barrier->count;
> >>
> >>       if (count == 2*barrier->count-1) {
> >> -             barrier->bar = 0;
> >> +             /* Wrap around *atomically* */
> >> +             odp_atomic_sub_u32(&barrier->bar, 2 * barrier->count);
> >>       } else {
> >> -             while ((barrier->bar < barrier->count) == wasless)
> >> +             while ((odp_atomic_load_u32(&barrier->bar) < barrier-
> >count)
> >> +                             == wasless)
> >>                       odp_spin();
> >>       }
> >>
> >> -     odp_mem_barrier();
> >> +     __atomic_thread_fence(__ATOMIC_SEQ_CST);
> >>  }
> >> --
> >> 1.9.1
> >>
> >>
> >> _______________________________________________
> >> lng-odp mailing list
> >> lng-odp@lists.linaro.org
> >> http://lists.linaro.org/mailman/listinfo/lng-odp

diff --git a/platform/linux-generic/include/api/odp_barrier.h b/platform/linux-generic/include/api/odp_barrier.h
index 1790ea3..03e889d 100644
--- a/platform/linux-generic/include/api/odp_barrier.h
+++ b/platform/linux-generic/include/api/odp_barrier.h
@@ -42,7 +42,7 @@  typedef struct odp_barrier_t {
  * @param barrier    Barrier
  * @param count      Thread count
  */
-void odp_barrier_init(odp_barrier_t *barrier, int count);
+void odp_barrier_init(odp_barrier_t *barrier, uint32_t count);
 
 
 /**
diff --git a/platform/linux-generic/odp_barrier.c b/platform/linux-generic/odp_barrier.c
index 87be2a1..2f14e2f 100644
--- a/platform/linux-generic/odp_barrier.c
+++ b/platform/linux-generic/odp_barrier.c
@@ -8,11 +8,10 @@ 
 #include <odp_sync.h>
 #include <odp_spin_internal.h>
 
-void odp_barrier_init(odp_barrier_t *barrier, int count)
+void odp_barrier_init(odp_barrier_t *barrier, uint32_t count)
 {
 	barrier->count = count;
-	barrier->bar   = 0;
-	odp_sync_stores();
+	odp_atomic_init_u32(&barrier->bar, 0);
 }
 
 /*
@@ -33,16 +32,18 @@  void odp_barrier_wait(odp_barrier_t *barrier)
 	uint32_t count;
 	int wasless;
 
-	odp_sync_stores();
-	wasless = barrier->bar < barrier->count;
+	__atomic_thread_fence(__ATOMIC_SEQ_CST);
 	count   = odp_atomic_fetch_inc_u32(&barrier->bar);
+	wasless = count < barrier->count;
 
 	if (count == 2*barrier->count-1) {
-		barrier->bar = 0;
+		/* Wrap around *atomically* */
+		odp_atomic_sub_u32(&barrier->bar, 2 * barrier->count);
 	} else {
-		while ((barrier->bar < barrier->count) == wasless)
+		while ((odp_atomic_load_u32(&barrier->bar) < barrier->count)
+				== wasless)
 			odp_spin();
 	}
 
-	odp_mem_barrier();
+	__atomic_thread_fence(__ATOMIC_SEQ_CST);
 }

[2/7] api: odp_barrier.h: signature change, use odp_atomic.h, atomic wrap-around fix

Commit Message

Comments

Patch