core/thread_flags: add thread flags group #21254

derMihai · 2025-02-28T12:18:04Z

Contribution description

Adds thread flags group overlay.

From core/include/thread_flags_group.h:

A thread flags group allows setting thread flags for an arbitrary number of
threads (called waiters) at the same time. The waiters can join and leave
the group at any time. An additional advantage is that the signaler (the
"flags setter") is de-coupled from the list of waiters, i.e. it does not
need to know which specific thread must be signaled.

Example (waiter):

static tfg_t group = THREAD_FLAGS_GROUP_INIT;

// ...
 
tfg_entry_t entry;
thread_flags_group_join(&group, &entry);
 
while (!some_condition_is_met()) {
    thread_flags_wait_any(SOME_FLAG);
}

thread_flags_group_leave(&group, &entry);

Example (signaler):

fulfill_some_condition();
thread_flags_group_set(&group, SOME_FLAG);

Testing procedure

Run the test application on:

native
samd20-xpro

Issues/PRs references

This construct was suggested by @kaspar030 as alternative for wait queues in #21123

maribu

Nice one, thx :)

tests/core/thread_flags_group/main.c

core/include/thread_flags_group.h

maribu · 2025-02-28T14:20:33Z

PR adds new feature to core --> 2 ACKs required.

Feel free to squash right away.

kaspar030 · 2025-02-28T14:37:34Z

Why not a bit field for the pids that want to be group members?

derMihai · 2025-03-03T08:25:24Z

Why not a bit field for the pids that want to be group members?

We only expect a handful of waiters. In that case I guess it's probably faster than iterating the bitfield? On the other hand joining and especially leaving would probably be faster with a bitfield...

I'll prepare an alternative impl with bitfields.

derMihai · 2025-03-03T11:15:39Z

Why not a bit field for the pids that want to be group members?

I pushed a bitfield version. I'm explicitly not using the RIOT's bitfield implementation as it forces some bit ordering which I don't care about, and as result doesn't use the atomics in atomic_utils.h, which are a nice optimization.

kaspar030 · 2025-03-05T10:22:21Z

core/include/thread_flags_group.h

+ * @brief Thread flags group.
+ */
+typedef struct {
+    uint8_t members[MAXTHREADS / 8 + !!(MAXTHREADS % 8)]; /**< members bit field */


hm, does RIOT still want to support >32 threads? ... if yes, this is maybe fine. If not, why not just use uint32_t (atomic) here.
Then, re-use the bitarithm.h functionality (there's e.g., an ffs that would use cpu intrinsics like CLZ under the hood), dropping the need for the loop below.

I really don't know, but I assumed it's possible to have more that 32 threads. There's currently nothing preventing me from setting e.g. MAXTHREADS to 64.

Ok I changed the bitfield array to native integer size (by that I mean whatever unsigned int is). If MAXTHREADS <= 32 and if the arch is 32 bit, the whole bitfield arithmetic is compiled out of existence.

core/include/thread_flags_group.h

derMihai · 2025-04-23T18:02:26Z

Any updates here?

sys/include/atomic_utils.h

maribu · 2025-05-02T09:25:57Z

UINT_WIDTH is not provided by the GCC 5.X series limits.h. GCC 7.X, to which the AVR tool has been updated in the recent Ubuntu package, does provide it.

IMO we should avoid cluttering core with workarounds for shitty historic toolchains that some distros keep shipping. Instead, we should IMO add the workaround to AVR.

I think something along the lines of:

diff --git a/cpu/avr8_common/Makefile.include b/cpu/avr8_common/Makefile.include
index 295bdea2aa..006c103e5b 100644
--- a/cpu/avr8_common/Makefile.include
+++ b/cpu/avr8_common/Makefile.include
@@ -12,3 +12,8 @@ LINKFLAGS += -Wl,--defsym=flash_printf=printf_P
 LINKFLAGS += -Wl,--defsym=flash_fprintf=fprintf_P
 LINKFLAGS += -Wl,--defsym=flash_snprintf=snprintf_P
 include $(RIOTMAKE)/arch/avr8.inc.mk
+
+# work around for shitty Ubuntu/Debian AVR toolchain. This can be dropped
+# when all commonly used versions of Ubuntu/Debian ship less historic
+# versions of GCC.
+CFLAGS += -DUINT_WIDTH=16

should work.

derMihai · 2025-05-02T13:19:21Z

UINT_WIDTH is not provided by the GCC 5.X series limits.h. GCC 7.X, to which the AVR tool has been updated in the recent Ubuntu package, does provide it.

IMO we should avoid cluttering core with workarounds for shitty historic toolchains that some distros keep shipping. Instead, we should IMO add the workaround to AVR.

I think something along the lines of:
diff --git a/cpu/avr8_common/Makefile.include b/cpu/avr8_common/Makefile.include
index 295bdea2aa..006c103e5b 100644
--- a/cpu/avr8_common/Makefile.include
+++ b/cpu/avr8_common/Makefile.include
@@ -12,3 +12,8 @@ LINKFLAGS += -Wl,--defsym=flash_printf=printf_P
 LINKFLAGS += -Wl,--defsym=flash_fprintf=fprintf_P
 LINKFLAGS += -Wl,--defsym=flash_snprintf=snprintf_P
 include $(RIOTMAKE)/arch/avr8.inc.mk
+
+# work around for shitty Ubuntu/Debian AVR toolchain. This can be dropped
+# when all commonly used versions of Ubuntu/Debian ship less historic
+# versions of GCC.
+CFLAGS += -DUINT_WIDTH=16
should work.

Looks like other compilers (including the ARM gcc) don't have this either...

maribu · 2025-05-02T13:31:47Z

Someone (tm) really should fix the CI image...

I wonder why it is different for legacy ARM and Cortex M. On Alpine, we are using the same GCC for all ARM systems.

derMihai · 2025-05-02T13:45:25Z

I added a fallback define, my ARM toolchain on Ubuntu 22.04 doesn't have UINT_WIDTH either.

maribu

Now only a

$ make generate-Makefile.ci -C tests/core/thread_flags_group

is missing to get it past the CI :)

maribu · 2025-05-02T14:20:04Z

core/include/thread_flags_group.h

+/**
+ * @brief Number of bits in unsigned int
+ */
+#define UINT_WIDTH (sizeof(unsigned) * 8)


Suggested change

#define UINT_WIDTH (sizeof(unsigned) * 8)

# define UINT_WIDTH (sizeof(unsigned) * 8)

maribu · 2025-05-02T14:26:03Z

core/include/thread_flags_group.h

+/* Some compilers don't define UINT_WIDTH */
+#ifndef UINT_WIDTH


The defines are actually provided on the GCC 13.2.1 packed in Ubuntu, but guarded with:

#if (defined __STDC_WANT_IEC_60559_BFP_EXT__ \ || (defined (__STDC_VERSION__) && __STDC_VERSION__ > 201710L))

This matches what https://en.cppreference.com/w/c/header/limits says.

Suggested change

/* Some compilers don't define UINT_WIDTH */

#ifndef UINT_WIDTH

/* UINT_WIDTH is only provided starting with -std=c23 or newer. Until RIOT

* requires C23 as C version, we need provide it by hand when missing. */

#ifndef UINT_WIDTH

derMihai · 2025-05-02T15:46:19Z

Now only a
$ make generate-Makefile.ci -C tests/core/thread_flags_group
is missing to get it past the CI :)

I also managed to shrink the RAM usage to widen the coverage.

derMihai · 2025-05-20T07:17:05Z

@kaspar030 I think I addressed your suggestions, can we merge this?

crasbe

I can't really comment on what the code actually does, I don't have much experience with the core and thread internals.

Therefore some general remarks.

core/include/thread_flags_group.h

cpu/avr8_common/Makefile.include

tests/core/thread_flags_group/main.c

core/include/thread_flags_group.h

core/thread_flags_group.c

derMihai · 2025-05-28T18:04:07Z

@crasbe sorry I had a really busy week. I think I addressed your suggestions!

derMihai requested review from leandrolanzieri, aabadie, MichelRottleuthner and kaspar030 as code owners February 28, 2025 12:18

github-actions bot added Area: tests Area: tests and testing framework Area: core Area: RIOT kernel. Handle PRs marked with this with care! labels Feb 28, 2025

derMihai force-pushed the mir/tflags_group branch from 675f752 to dcffa32 Compare February 28, 2025 12:35

benpicco requested a review from maribu February 28, 2025 12:41

derMihai force-pushed the mir/tflags_group branch from dcffa32 to 96840fa Compare February 28, 2025 12:45

maribu reviewed Feb 28, 2025

View reviewed changes

tests/core/thread_flags_group/main.c Outdated Show resolved Hide resolved

tests/core/thread_flags_group/main.c Outdated Show resolved Hide resolved

core/include/thread_flags_group.h Outdated Show resolved Hide resolved

maribu added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Feb 28, 2025

github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Feb 28, 2025

derMihai force-pushed the mir/tflags_group branch from 96840fa to b785d24 Compare March 3, 2025 08:30

derMihai force-pushed the mir/tflags_group branch from 1b1aaf8 to 52bdfdf Compare March 4, 2025 09:38

This was referenced Mar 4, 2025

core: add wait queue wrapper around thread flags group #21265

Draft

core/sync: add wait queues #21123

Closed

kaspar030 reviewed Mar 5, 2025

View reviewed changes

derMihai force-pushed the mir/tflags_group branch from ea26c1d to 34bb960 Compare March 6, 2025 15:13

derMihai commented Mar 6, 2025

View reviewed changes

core/include/thread_flags_group.h Outdated Show resolved Hide resolved

derMihai force-pushed the mir/tflags_group branch from 34bb960 to 42e2442 Compare March 6, 2025 15:29

derMihai force-pushed the mir/tflags_group branch 4 times, most recently from 3c7fe36 to 2e9af62 Compare March 20, 2025 15:19

maribu mentioned this pull request Apr 23, 2025

sys/atomic_utils: add helpers for unsigned int #21429

Merged

maribu reviewed Apr 26, 2025

View reviewed changes

sys/include/atomic_utils.h Outdated Show resolved Hide resolved

derMihai force-pushed the mir/tflags_group branch from 601506f to b7504e4 Compare May 2, 2025 08:16

github-actions bot removed the Area: sys Area: System label May 2, 2025

derMihai force-pushed the mir/tflags_group branch from b7504e4 to b198ca0 Compare May 2, 2025 08:20

maribu approved these changes May 2, 2025

View reviewed changes

derMihai force-pushed the mir/tflags_group branch from b198ca0 to 3ebef94 Compare May 2, 2025 13:12

derMihai requested review from kYc0o and nandojve as code owners May 2, 2025 13:12

github-actions bot added Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: cpu Area: CPU/MCU ports labels May 2, 2025

derMihai force-pushed the mir/tflags_group branch from 3ebef94 to 1309f37 Compare May 2, 2025 13:44

maribu reviewed May 2, 2025

View reviewed changes

derMihai force-pushed the mir/tflags_group branch from ca93d98 to ff87cfe Compare May 2, 2025 15:44

maribu requested review from benpicco, crasbe and mguetschow May 20, 2025 08:17

crasbe reviewed May 20, 2025

View reviewed changes

derMihai force-pushed the mir/tflags_group branch from ff87cfe to f99603d Compare May 28, 2025 18:03

github-actions bot removed Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: cpu Area: CPU/MCU ports labels May 28, 2025

derMihai force-pushed the mir/tflags_group branch from f99603d to 505843b Compare May 28, 2025 18:07

core/thread_flags: add thread flags group

eb3c8c3

derMihai force-pushed the mir/tflags_group branch from 505843b to eb3c8c3 Compare May 28, 2025 19:02

	#define UINT_WIDTH (sizeof(unsigned) * 8)
	# define UINT_WIDTH (sizeof(unsigned) * 8)

		/* Some compilers don't define UINT_WIDTH */
		#ifndef UINT_WIDTH

core/thread_flags: add thread flags group #21254

Are you sure you want to change the base?

core/thread_flags: add thread flags group #21254

Conversation

derMihai commented Feb 28, 2025

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Feb 28, 2025

Uh oh!

kaspar030 commented Feb 28, 2025

Uh oh!

derMihai commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derMihai commented Mar 3, 2025

Uh oh!

kaspar030 Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derMihai Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

derMihai Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

derMihai commented Apr 23, 2025

Uh oh!

Uh oh!

maribu commented May 2, 2025

Uh oh!

derMihai commented May 2, 2025

Uh oh!

maribu commented May 2, 2025

Uh oh!

derMihai commented May 2, 2025

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

maribu May 2, 2025

Choose a reason for hiding this comment

Uh oh!

maribu May 2, 2025

Choose a reason for hiding this comment

Uh oh!

derMihai commented May 2, 2025

Uh oh!

derMihai commented May 20, 2025

Uh oh!

crasbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derMihai commented May 28, 2025

Uh oh!

Uh oh!

derMihai commented Mar 3, 2025 •

edited

Loading

kaspar030 Mar 5, 2025 •

edited

Loading