core/thread_flags: add thread_flags_wait_any_or_mbox()

[maribu]

Contribution description

This adds thread_flags_wait_any_or_mbox() that blocks until any of a given thread flags is set, or a message is retrieved from an mbox - whatever happens first.

This then was used to implement ztimer_mbox_get_timeout() (which relates to mbox_get() the same way ztimer_mutex_lock_timeout() relates to mutex_lock()).

Finally, ztimer_mbox_get_timeout() is used in GNRC to fix a nasty race condition.

Testing procedure

A test application is provided.

Issues/PRs references

Alternative to #18949 and #19965

[benpicco]

If this saves us from hacks like #19965 I'm all for it!
Can you provide an example how this solves the issue in GNRC?
I have a test setup that provokes the issue quite reliably.

[riot-ci]

Murdock results

❌ FAILED

963d3b3 sys/net/gnrc_sock: use ztimer_mbox_get_timeout()

Success	Failures	Total	Runtime
157	0	7989	40s

Artifacts

[MrKevinWeiss]

Since we have an ACK and it is agreed that this is proper fix I would suggest we merge before the hard feature freeze today? Any objections?

[maribu]

I'm sadly afk, otherwise I would suggest that I add a big warning that the function is super mega experimental. That would give some leverage to revert, if needed be.

[maribu]

⬆️ First a rebase without any changes

[maribu]

Can you provide an example how this solves the issue in GNRC?

Done. Care to test?

[maribu]

A commit of a different PR sneaked in, sorry

[benpicco]

Hm I applied 17e12fc, c8853f1 and 8a2bc1f but I still see

>>> timeout happened 9847 µs early <<<
>>> timeout happened 1166 µs early <<<
>>> timeout happened 9824 µs early <<<
>>> timeout happened 9818 µs early <<<
>>> timeout happened 9794 µs early <<<
>>> timeout happened 9825 µs early <<<
>>> timeout happened 9400 µs early <<<
>>> timeout happened 9730 µs early <<<
>>> timeout happened 1043 µs early <<<
>>> timeout happened 9667 µs early <<<

with

    unsigned now = ztimer_now(ZTIMER_USEC);
    while (1) {
        res = gnrc_sock_recv((gnrc_sock_reg_t *)sock, &pkt, timeout, &tmp, &_aux);
        /* HACK: gnrc_sock_recv() sometimes returnes -ETIMEDOUT too early */
        now = ztimer_now(ZTIMER_USEC) - now;
        if (res == -ETIMEDOUT && now < (timeout - timeout/10))  {
            timeout -= now;
            printf(">>> timeout happened %u µs early <<<\n", timeout);
            continue;
        }
        break;
    }

(from #19965)

[maribu]

Can you provide more details on how to reproduce this? Maybe I can even turn this into a unit test.

[benpicco]

It's unfortunately a bit involved for a unit test, but if you check out the nanocoap_non_block-gnrc_bug branch and run ./test.sh -n 100 init and ./test.sh attach in tests/nanocoap_cli you can cycle through the different native instances in tmux, you'll quickly find some that show the early timeout.

[MrKevinWeiss]

It would be good to have either this or #19965 backported to the release...

[MrKevinWeiss]

ping, I think we would like this in the release

[maribu]

I addressed the whitepsace and spelling issues uncovered by static-tests (sqaushed them right in) and rebased on top of the changes for github merge queues.

I'm almost certain that this does fix a race condition. But the issue at hand in @benpicco's test is then not caused by this. (Nor was it caused by the issue in the periph_timer implementation on native.) However, until that test is indeed passing, we cannot be sure if this fix is just broken, or whether there is indeed a third bug still not found that needs to be addressed as well.

My gut tells me that since the race condition has been there for ages, it might be best to solve this puzzle fully before applying a fix that may not work - even if this means the fix will not get in with this release.

[MrKevinWeiss]

Then I will remove it from the release backport and we can just have a known bug... We are already way over the time.

[benpicco]

#19965 provides a workaround