Re: [Minios-devel] [UNIKRAFT/PTHREAD-EMBEDDED PATCH 5/8] patches: Fix atomic operations

[Costin Lupu <costin.lup@xxxxxxxxx>]

On 6/3/19 10:31 AM, Florian Schmidt wrote:
> Hi Costin,
> 
> On 4/15/19 1:43 PM, Costin Lupu wrote:
>> We use atomic operations as macros because pte uses them for both int
>> and long types.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>   patches/0007-Use-atomic-operations-as-macros.patch | 62
>> +++++++++++++++++++++
>>   ...bugfix-Fix-atomic-operations-on-semaphore.patch | 63
>> ++++++++++++++++++++++
>>   2 files changed, 125 insertions(+)
>>   create mode 100644 patches/0007-Use-atomic-operations-as-macros.patch
>>   create mode 100644
>> patches/0008-bugfix-Fix-atomic-operations-on-semaphore.patch
>>
>> diff --git a/patches/0007-Use-atomic-operations-as-macros.patch
>> b/patches/0007-Use-atomic-operations-as-macros.patch
>> new file mode 100644
>> index 0000000..63313b2
>> --- /dev/null
>> +++ b/patches/0007-Use-atomic-operations-as-macros.patch
>> @@ -0,0 +1,62 @@
>> +From e0cf6be2cec87542be01bb127413f31a05ec161b Mon Sep 17 00:00:00 2001
>> +From: Costin Lupu <costin.lup@xxxxxxxxx>
>> +Date: Wed, 3 Apr 2019 18:54:21 +0300
>> +Subject: [PATCH 1/2] Use atomic operations as macros
>> +
>> +We use atomic operations as macros because pte uses them for both int
>> +and long types (see next patch).
>> +
>> +Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> +---
>> + pte_generic_osal.h | 28 ++++++++++++++++++++++++++++
>> + 1 file changed, 28 insertions(+)
>> +
>> +diff --git a/pte_generic_osal.h b/pte_generic_osal.h
>> +index de1ea5f..6b1f439 100644
>> +--- a/pte_generic_osal.h
>> ++++ b/pte_generic_osal.h
>> +@@ -378,6 +378,7 @@ pte_osResult pte_osTlsFree(unsigned int key);
>> + //@}
>> +
>> + /** @name Atomic operations */
>> ++#if 0
> 
> I wonder whether you could instead remove all the code that you
> commented out with the #if 0? Though that's probably a question of
> preference.
> 

We'll keep this as we agreed for the next patch as well.

>> + //@{
>> +
>> + /**
>> +@@ -455,6 +456,33 @@ int pte_osAtomicDecrement(int *pdest);
>> +  * return origVal;
>> +  */
>> + int pte_osAtomicIncrement(int *pdest);
>> ++#else
>> ++
>> ++#include <uk/arch/atomic.h>
>> ++
>> ++#define pte_osAtomicExchange(ptarg, val) \
>> ++    ukarch_exchange_n(ptarg, val)
>> ++
>> ++#define pte_osAtomicCompareExchange(pdest, exchange, comp) \
>> ++({ \
>> ++    __typeof__(*pdest) __orig = *pdest; \
>> ++    ukarch_compare_exchange_sync(pdest, comp, exchange); \
>> ++    __orig; \
>> ++})
>> ++
>> ++#define pte_osAtomicExchangeAdd(paddend, value) \
>> ++    ukarch_fetch_add(paddend, value)
>> ++
>> ++#define atomic_add(ptarg, val) \
>> ++    __atomic_add_fetch(ptarg, val, __ATOMIC_SEQ_CST)
> 
> Shouldn't this be __atomic_fetch_add? The pthread_embedded source code
> says that pte_osAtomic{De,In}crement return the original value, but this
> returns the modified value.
> 

Although the comments in 'pte_generic_osal.h' state that the functions
should return the original value, if we do that and use
__atomic_fetch_add instead, the pthread tests will block on 'TSD test
#1'. If we go further we check the implementation of
'pthread_barrier_wait' which is used by the given test, we'll find the
following code:

  if (0 == PTE_ATOMIC_DECREMENT ((int *) &(b->nCurrentBarrierHeight)))
    {
       /* post the semaphore, ie let the threads run */

which implies that PTE_ATOMIC_DECREMENT has to return the new value, not
the original one. So I guess the comments in 'pte_generic_osal.h' are
actually wrong.

As we agreed offline we'll keep this as it is.

> 
>> ++
>> ++#define pte_osAtomicDecrement(pdest) \
>> ++    atomic_add(pdest, -1)
>> ++
>> ++#define pte_osAtomicIncrement(pdest) \
>> ++    atomic_add(pdest, 1)
>> ++
>> ++#endif
>> + //@}
>> +
>> + struct timeb;
>> +--
>> +2.11.0
>> +
>> diff --git
>> a/patches/0008-bugfix-Fix-atomic-operations-on-semaphore.patch
>> b/patches/0008-bugfix-Fix-atomic-operations-on-semaphore.patch
>> new file mode 100644
>> index 0000000..6ca339d
>> --- /dev/null
>> +++ b/patches/0008-bugfix-Fix-atomic-operations-on-semaphore.patch
>> @@ -0,0 +1,63 @@
>> +From 09a21187f7e426147d4d5fb91451ea55cf7ac274 Mon Sep 17 00:00:00 2001
>> +From: Costin Lupu <costin.lup@xxxxxxxxx>
>> +Date: Wed, 3 Apr 2019 18:56:31 +0300
>> +Subject: [PATCH 2/2] bugfix Fix atomic operations on semaphore
>> +
>> +Field 'semaphore' of struct pthread_once_t_ is of type 'void *'.
>> +Therefore atomic operations should use long type instead of int.
>> +
>> +Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> +---
>> + pthread_once.c | 12 ++++++------
>> + 1 file changed, 6 insertions(+), 6 deletions(-)
>> +
>> +diff --git a/pthread_once.c b/pthread_once.c
>> +index a8166f5..2b6050f 100644
>> +--- a/pthread_once.c
>> ++++ b/pthread_once.c
>> +@@ -55,7 +55,7 @@ pte_once_init_routine_cleanup(void * arg)
>> +
>> +   (void) PTE_ATOMIC_EXCHANGE(&once_control->state,PTE_ONCE_INIT);
>> +
>> +-  if (PTE_ATOMIC_EXCHANGE_ADD((int*)&once_control->semaphore, 0L))
>> /* MBR fence */
>> ++  if (PTE_ATOMIC_EXCHANGE_ADD((long*)&once_control->semaphore, 0L))
>> /* MBR fence */
>> +     {
>> +       pte_osSemaphorePost((pte_osSemaphoreHandle)
>> once_control->semaphore, 1);
>> +     }
>> +@@ -134,7 +134,7 @@ pthread_once (pthread_once_t * once_control, void
>> (*init_routine) (void))
>> +            * we didn't create the semaphore.
>> +            * it is only there if there is someone waiting.
>> +            */
>> +-          if
>> (PTE_ATOMIC_EXCHANGE_ADD((int*)&once_control->semaphore, 0L)) /* MBR
>> fence */
>> ++          if
>> (PTE_ATOMIC_EXCHANGE_ADD((long*)&once_control->semaphore, 0L)) /* MBR
>> fence */
>> +             {
>> +               pte_osSemaphorePost((pte_osSemaphoreHandle)
>> once_control->semaphore,once_control->numSemaphoreUsers);
>> +             }
>> +@@ -143,12 +143,12 @@ pthread_once (pthread_once_t * once_control,
>> void (*init_routine) (void))
>> +         {
>> +           PTE_ATOMIC_INCREMENT(&once_control->numSemaphoreUsers);
>> +
>> +-          if
>> (!PTE_ATOMIC_EXCHANGE_ADD((int*)&once_control->semaphore, 0L)) /* MBR
>> fence */
>> ++          if
>> (!PTE_ATOMIC_EXCHANGE_ADD((long*)&once_control->semaphore, 0L)) /* MBR
>> fence */
>> +             {
>> +               pte_osSemaphoreCreate(0, (pte_osSemaphoreHandle*) &sema);
>> +
>> +-              if (PTE_ATOMIC_COMPARE_EXCHANGE((int *)
>> &once_control->semaphore,
>> +-                                              (int) sema,
>> ++              if (PTE_ATOMIC_COMPARE_EXCHANGE((long *)
>> &once_control->semaphore,
>> ++                                              (long) sema,
>> +                                               0))
>> +                 {
>> +                   pte_osSemaphoreDelete((pte_osSemaphoreHandle)sema);
>> +@@ -168,7 +168,7 @@ pthread_once (pthread_once_t * once_control, void
>> (*init_routine) (void))
>> +             {
>> +               /* we were last */
>> +               if ((sema =
>> +-                     (pte_osSemaphoreHandle)
>> PTE_ATOMIC_EXCHANGE((int *) &once_control->semaphore,0)))
>> ++                     (pte_osSemaphoreHandle)
>> PTE_ATOMIC_EXCHANGE((long *) &once_control->semaphore,0)))
>> +                 {
>> +                   pte_osSemaphoreDelete(sema);
>> +                 }
>> +--
>> +2.11.0
>> +
>>
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel