Skip to content

Commit

Permalink
Stop using atomic_flag_*() for atomic_int in mutex
Browse files Browse the repository at this point in the history
The atomic_flag_* functions are only defined for operands of the type
atomic_flag so the previous implementation relied on undefined behavior.
Because of the need for a read without side effects (in
__metal_mutex_is_acquired), it is not possible to use the atomic_flags
type for the mutex implementation. Because of this, all atomic_flag_*
functions are replaced with the appropriate functions for atomic value
types.

Signed-off-by: Simon Leiner <simon@leiner.me>
  • Loading branch information
sleiner authored and arnopo committed Sep 7, 2020
1 parent 5cae751 commit 9a3162e
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 13 deletions.
26 changes: 19 additions & 7 deletions lib/system/freertos/mutex.h
Expand Up @@ -17,7 +17,6 @@
#define __METAL_FREERTOS_MUTEX__H__

#include <metal/atomic.h>
#include <stdlib.h>

#ifdef __cplusplus
extern "C" {
Expand All @@ -27,11 +26,14 @@ typedef struct {
atomic_int v;
} metal_mutex_t;

#define METAL_MUTEX_UNLOCKED 0
#define METAL_MUTEX_LOCKED 1

/*
* METAL_MUTEX_INIT - used for initializing an mutex elmenet in a static struct
* METAL_MUTEX_INIT - used for initializing an mutex element in a static struct
* or global
*/
#define METAL_MUTEX_INIT(m) { ATOMIC_VAR_INIT(0) }
#define METAL_MUTEX_INIT(m) { ATOMIC_VAR_INIT(METAL_MUTEX_UNLOCKED) }
/*
* METAL_MUTEX_DEFINE - used for defining and initializing a global or
* static singleton mutex
Expand All @@ -40,7 +42,7 @@ typedef struct {

static inline void __metal_mutex_init(metal_mutex_t *mutex)
{
atomic_store(&mutex->v, 0);
atomic_store(&mutex->v, METAL_MUTEX_UNLOCKED);
}

static inline void __metal_mutex_deinit(metal_mutex_t *mutex)
Expand All @@ -50,19 +52,29 @@ static inline void __metal_mutex_deinit(metal_mutex_t *mutex)

static inline int __metal_mutex_try_acquire(metal_mutex_t *mutex)
{
return 1 - atomic_flag_test_and_set(&mutex->v);
int unlocked = METAL_MUTEX_UNLOCKED;

if (atomic_compare_exchange_strong(&mutex->v, &unlocked,
METAL_MUTEX_LOCKED)) {
return 1; /* acquired */
} else {
return 0; /* not acquired */
}
}

static inline void __metal_mutex_acquire(metal_mutex_t *mutex)
{
while (atomic_flag_test_and_set(&mutex->v)) {
int unlocked = METAL_MUTEX_UNLOCKED;

while (!atomic_compare_exchange_weak(&mutex->v, &unlocked,
METAL_MUTEX_LOCKED)) {
;
}
}

static inline void __metal_mutex_release(metal_mutex_t *mutex)
{
atomic_flag_clear(&mutex->v);
atomic_store(&mutex->v, METAL_MUTEX_UNLOCKED);
}

static inline int __metal_mutex_is_acquired(metal_mutex_t *mutex)
Expand Down
25 changes: 19 additions & 6 deletions lib/system/generic/mutex.h
Expand Up @@ -26,11 +26,14 @@ typedef struct {
atomic_int v;
} metal_mutex_t;

#define METAL_MUTEX_UNLOCKED 0
#define METAL_MUTEX_LOCKED 1

/*
* METAL_MUTEX_INIT - used for initializing an mutex elmenet in a static struct
* METAL_MUTEX_INIT - used for initializing an mutex element in a static struct
* or global
*/
#define METAL_MUTEX_INIT(m) { ATOMIC_VAR_INIT(0) }
#define METAL_MUTEX_INIT(m) { ATOMIC_VAR_INIT(METAL_MUTEX_UNLOCKED) }
/*
* METAL_MUTEX_DEFINE - used for defining and initializing a global or
* static singleton mutex
Expand All @@ -39,7 +42,7 @@ typedef struct {

static inline void __metal_mutex_init(metal_mutex_t *mutex)
{
atomic_store(&mutex->v, 0);
atomic_store(&mutex->v, METAL_MUTEX_UNLOCKED);
}

static inline void __metal_mutex_deinit(metal_mutex_t *mutex)
Expand All @@ -49,19 +52,29 @@ static inline void __metal_mutex_deinit(metal_mutex_t *mutex)

static inline int __metal_mutex_try_acquire(metal_mutex_t *mutex)
{
return 1 - atomic_flag_test_and_set(&mutex->v);
int unlocked = METAL_MUTEX_UNLOCKED;

if (atomic_compare_exchange_strong(&mutex->v, &unlocked,
METAL_MUTEX_LOCKED)) {
return 1; /* acquired */
} else {
return 0; /* not acquired */
}
}

static inline void __metal_mutex_acquire(metal_mutex_t *mutex)
{
while (atomic_flag_test_and_set(&mutex->v)) {
int unlocked = METAL_MUTEX_UNLOCKED;

while (!atomic_compare_exchange_weak(&mutex->v, &unlocked,
METAL_MUTEX_LOCKED)) {
;
}
}

static inline void __metal_mutex_release(metal_mutex_t *mutex)
{
atomic_flag_clear(&mutex->v);
atomic_store(&mutex->v, METAL_MUTEX_UNLOCKED);
}

static inline int __metal_mutex_is_acquired(metal_mutex_t *mutex)
Expand Down

2 comments on commit 9a3162e

@vishnu-banavath
Copy link

@vishnu-banavath vishnu-banavath commented on 9a3162e Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
I'm aware that these changes are merged. But, just checking if there is an fallback option for processors that does not support atomic operations. As an example, Cortex-M0 (ArmV6M) does not support atomic operations, this means, atomic_compare_exchange_weak can't be used as the compiler can't generate code for __sync_val_compare_and_swap

@arnopo
Copy link
Contributor

@arnopo arnopo commented on 9a3162e Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vishnu-banavath

Hi,
I'm aware that these changes are merged. But, just checking if there is an fallback option for processors that does not support atomic operations. As an example, Cortex-M0 (ArmV6M) does not support atomic operations, this means, atomic_compare_exchange_weak can't be used as the compiler can't generate code for __sync_val_compare_and_swap

it a concerns or an issue you detected?
Do you have a look to https://github.com/OpenAMP/libmetal/blob/master/lib/compiler/gcc/atomic.h?

If there is an issue don't hesitate to enter a github issue

Please sign in to comment.