Add C11 standard atomic support #1645

2024-02-20T17:18:48-06:00

sgstreet commented

2024-02-20 17:18:48 -06:00

(Migrated from github.com)

This PR Fixes #1642 Runtime for C11 atomics missing.

The file pico_atomic.c implements the required functions for the read/modify/write function of atomic variables with sizes of 1, 2, 4 and 8 bytes. Atomic load and store for 8 bytes quantities is also supported. Support for atomic_flags is also present.

To eliminate interference with existing hardware spinlock usage while reducing multicore contention on unique atomic variables, we use one of the watchdog scratch registers (WATCHDOG_SCRATCH3) to implement 16, 2 bit, multicore locks, via a variation of Dekker's Algorithm. The lock is selected as a function of the variable address and the stripe width which hashes variables addresses to locks numbers and leverages the RP2040 Atomic Register Access functionality of the WATCHDOG scratch registers.

Clearly the current approach (See __atomic_lock) is slower than using the RP2040 Hardware Spinlocks directly, but I was not sure how to assign spin locks from the reserved range (IDs 0-13) and was concerned about multicore lock contention when the number of hash buckets (hardware spinlocks) is minimized. Any recommendations concerning the number of spins lock to allocated to this library along with the stripping width (number of addresses hashed to the same lock) would greatly appreciated? My thoughts are four to eight locks with a stripe width of four bytes. Using the hardware spinlocks is an easy update.

This PR Fixes #1642 Runtime for C11 atomics missing. The file pico_atomic.c implements the required functions for the read/modify/write function of atomic variables with sizes of 1, 2, 4 and 8 bytes. Atomic load and store for 8 bytes quantities is also supported. Support for atomic_flags is also present. To eliminate interference with existing hardware spinlock usage while reducing multicore contention on unique atomic variables, we use one of the watchdog scratch registers (WATCHDOG_SCRATCH3) to implement 16, 2 bit, multicore locks, via a variation of [Dekker's Algorithm](https://en.wikipedia.org/wiki/Dekker%27s_algorithm). The lock is selected as a function of the variable address and the stripe width which hashes variables addresses to locks numbers and leverages the RP2040 Atomic Register Access functionality of the WATCHDOG scratch registers. Clearly the current approach (See __atomic_lock) is slower than using the RP2040 Hardware Spinlocks directly, but I was not sure how to assign spin locks from the reserved range (IDs 0-13) and was concerned about multicore lock contention when the number of hash buckets (hardware spinlocks) is minimized. Any recommendations concerning the number of spins lock to allocated to this library along with the stripping width (number of addresses hashed to the same lock) would greatly appreciated? My thoughts are four to eight locks with a stripe width of four bytes. Using the hardware spinlocks is an easy update.

sgstreet commented

2024-02-28 13:22:07 -06:00

(Migrated from github.com)

As motivation to add this PR, below is a sample ticket based spin lock implementation similar to the Linux kernel. This type of implementation removes the limitation of 32 hardware spin locks at the minimal cost of potential hardware spin lock contention.

typedef atomic_ulong spinlock_t;

void spin_lock(spinlock_t *spinlock)
{
	uint16_t ticket = atomic_fetch_add(spinlock, 1UL << 16) >> 16;
	while ((*spinlock & 0xffff) != ticket);
}

unsigned int spin_lock_irqsave(spinlock_t *spinlock)
{
	uint32_t state = save_and_disable_interrupts();
	spin_lock(spinlock);
	return state;
}

bool spin_try_lock(spinlock_t *spinlock)
{
	uint32_t value = *spinlock;
	if ((value >> 16) != (value & 0xffff))
		return false;

	return atomic_compare_exchange_strong(spinlock, &value, value + (1UL << 16));
}

bool spin_try_lock_irqsave(spinlock_t *spinlock, unsigned int *state)
{
	uint32_t irq_state = save_and_disable_interrupts();
	bool locked = spin_try_lock(spinlock);
	if (!locked) {
		restore_interrupts(irq_state);
		return false;
	}

	*state = irq_state;
	return true;
}

void spin_unlock(spinlock_t *spinlock)
{
	atomic_fetch_add((uint16_t *)spinlock, 1);
}

void spin_unlock_irqrestore(spinlock_t *spinlock, unsigned int state)
{
	spin_unlock(spinlock);
	restore_interrupts(state);
}

As motivation to add this PR, below is a sample ticket based spin lock implementation similar to the Linux kernel. This type of implementation removes the limitation of 32 hardware spin locks at the minimal cost of potential hardware spin lock contention. ``` typedef atomic_ulong spinlock_t; void spin_lock(spinlock_t *spinlock) { uint16_t ticket = atomic_fetch_add(spinlock, 1UL << 16) >> 16; while ((*spinlock & 0xffff) != ticket); } unsigned int spin_lock_irqsave(spinlock_t *spinlock) { uint32_t state = save_and_disable_interrupts(); spin_lock(spinlock); return state; } bool spin_try_lock(spinlock_t *spinlock) { uint32_t value = *spinlock; if ((value >> 16) != (value & 0xffff)) return false; return atomic_compare_exchange_strong(spinlock, &value, value + (1UL << 16)); } bool spin_try_lock_irqsave(spinlock_t *spinlock, unsigned int *state) { uint32_t irq_state = save_and_disable_interrupts(); bool locked = spin_try_lock(spinlock); if (!locked) { restore_interrupts(irq_state); return false; } *state = irq_state; return true; } void spin_unlock(spinlock_t *spinlock) { atomic_fetch_add((uint16_t *)spinlock, 1); } void spin_unlock_irqrestore(spinlock_t *spinlock, unsigned int state) { spin_unlock(spinlock); restore_interrupts(state); } ```

kilograham commented

2024-07-03 08:01:40 -05:00

(Migrated from github.com)

@sgstreet ; I'm guessing you may have closed this because we hadn't gotten to it yet. It is a useful PR, if you are still OK with us merging it; we generally add our own copyright to yours

sgstreet commented

2024-07-03 09:08:10 -05:00

(Migrated from github.com)

Sure, let me update the PR there was a bug that needs to be fixed.

kilograham (Migrated from github.com) reviewed

2024-07-08 18:19:10 -05:00

src/rp2_common/pico_atomic/include/stdatomic.h

					
				@ -0,0 +8,4 @@

				#ifndef __STDATOMIC_H

				#define __STDATOMIC_H

				#include_next <stdatomic.h>

phew; i was wondering what versions of compilers supported this, but GCC 6 is happy, so OK i guess!

kilograham (Migrated from github.com) reviewed

2024-07-08 18:22:19 -05:00

src/rp2_common/pico_atomic/pico_atomic.c

					
				@ -0,0 +15,4 @@

				#include "pico/config.h"

				#ifndef __optimize

				#define __optimize __attribute__((optimize("-Os")))

is there a reason for Os by default? note i am merging this now as I want to test it with something else, but may fixup later

kilograham (Migrated from github.com) reviewed

2024-07-08 18:25:15 -05:00

kilograham (Migrated from github.com) left a comment

No description provided.

Thanks very much!

sgstreet (Migrated from github.com) reviewed

2024-07-08 18:27:09 -05:00

src/rp2_common/pico_atomic/include/stdatomic.h

					
				@ -0,0 +8,4 @@

				#ifndef __STDATOMIC_H

				#define __STDATOMIC_H

				#include_next <stdatomic.h>

I could drop a complete replacement for stdatomic.h as different approach? Seem like that would be worse in the long term.

kilograham (Migrated from github.com) reviewed

2024-07-08 18:30:00 -05:00

src/rp2_common/pico_atomic/include/stdatomic.h

					
				@ -0,0 +8,4 @@

				#ifndef __STDATOMIC_H

				#define __STDATOMIC_H

				#include_next <stdatomic.h>

yeah; i am happy with include_next i just thought it was a recent add

yeah; i am happy with `include_next` i just thought it was a recent add

sgstreet (Migrated from github.com) reviewed

2024-07-08 18:32:42 -05:00

src/rp2_common/pico_atomic/pico_atomic.c

					
				@ -0,0 +15,4 @@

				#include "pico/config.h"

				#ifndef __optimize

				#define __optimize __attribute__((optimize("-Os")))

From my experience, you get better code from -Os on the cortex parts. Less and generally faster code, but our mileage may vary. I can remove if you wish.
Sort of a adjacent question is if the atomic code should be forced in to SRAM?

From my experience, you get better code from -Os on the cortex parts. Less and generally faster code, but our mileage may vary. I can remove if you wish. Sort of a adjacent question is if the atomic code should be forced in to SRAM?

kilograham (Migrated from github.com) reviewed

2024-07-08 18:37:10 -05:00

src/rp2_common/pico_atomic/pico_atomic.c

					
				@ -0,0 +15,4 @@

				#include "pico/config.h"

				#ifndef __optimize

				#define __optimize __attribute__((optimize("-Os")))

good question; in similar bits of code we have a #define to pick

No, i think -Os is fine, and probably as you say better in many circumstances (our default being -O3 is a historic cmake thing, and i am worried about knock on effects on existing stuff if i change it)

good question; in similar bits of code we have a `#define` to pick No, i think `-Os` is fine, and probably as you say better in many circumstances (our default being -O3 is a historic cmake thing, and i am worried about knock on effects on existing stuff if i change it)

kilograham commented

2024-07-08 18:39:02 -05:00

(Migrated from github.com)

Thanks, I have merged this for now somewhat rapidly as we have something internal dependent on it...

sgstreet commented

2024-07-08 18:43:34 -05:00

(Migrated from github.com)

Glad you and the team found a use for this! Let me know if there is a problem or something else the team would like changed.

kilograham commented

2024-07-08 18:55:50 -05:00

(Migrated from github.com)

cool; i have actually backed it out again temporarily, as it seems certain versions of GCC complain about a few things

sgstreet commented

2024-07-08 19:04:34 -05:00

(Migrated from github.com)

If I could know which compiler versions and what problems cropped up I can see if there is an easy fix?

petrhosek (Migrated from github.com) reviewed

2024-07-09 00:06:53 -05:00

src/rp2_common/pico_atomic/pico_atomic.c

					
				@ -0,0 +15,4 @@

				#include "pico/config.h"

				#ifndef __optimize

				#define __optimize __attribute__((optimize("-Os")))

Note that this makes the code incompatible with Clang which doesn't support the optimize attribute which is GCC specific. A correct way to achieve this without relying on non-portable behavior is to set the optimization level for this specific target through the build system.

Note that this makes the code incompatible with Clang which doesn't support the `optimize` attribute which is GCC specific. A correct way to achieve this without relying on non-portable behavior is to set the optimization level for this specific target through the build system.

sgstreet (Migrated from github.com) reviewed

2024-07-09 09:02:47 -05:00

src/rp2_common/pico_atomic/pico_atomic.c

					
				@ -0,0 +15,4 @@

				#include "pico/config.h"

				#ifndef __optimize

				#define __optimize __attribute__((optimize("-Os")))

I tend not to use clang on my projects. I will remove this momentarily.

sgstreet (Migrated from github.com) reviewed

2024-07-09 12:56:30 -05:00

src/rp2_common/pico_atomic/pico_atomic.c

					
				@ -0,0 +15,4 @@

				#include "pico/config.h"

				#ifndef __optimize

				#define __optimize __attribute__((optimize("-Os")))

I updated the branching underlying this PR, but since the PR has been merged, closed and reverted I'm not sure how to get github to see the PR updates. @kilograham @petrhosek I think we will need a new PR. See PR #1763