core/thread: add thread states for FreeRTOS queue implementation

[gschorcht]

Contribution description

This PR adds the thread states STATE_QUEUE_FULL_BLOCKED and STATE_QUEUE_EMPTY_BLOCKED which are compiled in depending on whether the FreeRTOS compatibility layer for ESP SoCs is used.

This change fixes a problem that was figured out when investigating a crash in tests/nimble_rpble_gnrc in PR #18439.

The FreeRTOS queue and semaphore implementation of the FreeRTOS compatibility layer previously used STATUS_RECEIVE_BLOCKED and STATE_SEND_BLOCKED to block a thread when the corresponding queue was empty and full, respectively. This was compatible to the blocking mechanism used for msg by gnrc_netif. However, since PR #16748, gnrc_netif no longer uses the msg blocking mechanism, but uses thread flags.

As a consequence, RIOT may crash when using FreeRTOS queues and semaphores together with gnrc_netif for the ESP network interfaces if STATUS_RECEIVE_BLOCKED in same thread and STATUS_RECEIVE_BLOCKED is used by FreeRTOS compatibility layer. If the gnrc_netif thread is blocked because of a FreeRTOS semaphore and is therefore in STATUS_RECEIVE_BLOCKED state, the _msg_send function will cause a crash because it then assumes that target->wait_data contains a pointer to a message of type msg_t, but by using thread flags it contains just the flag mask. This situation can easily occur when the ESP hardware is used by the gnrc_netif thread while another thread is sending something timer controlled to the gnrc_netif thread. See also #18439 (comment).

To solve this problem STATE_QUEUE_FULL_BLOCKED and STATE_QUEUE_EMPTY_BLOCKED are introduced.

Testing procedure

Use command

USEMODULE=esp_wifi CFLAGS='-DESP_WIFI_SSID=\"myssid\" -DESP_WIFI_PASS=\"mypass\" -DDEBUG_ASSERT_VERBOSE' BOARD=esp32-wrover-kit make -j8 -C examples/gnrc_networking flash term

and check that the WiFi network interface still works.

Use command ps, the output should look like

	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
	  1 | sys_evt              | bl qempt _ |   4 |   2616 ( 1188) ( 1428) | 0x3ffbd3cc | 0x3ffbdba0 
	  2 | esp_timer            | sleeping _ |   2 |   3640 (  464) ( 3176) | 0x3ffbde08 | 0x3ffbea70 
	  3 | idle                 | pending  Q |  31 |   2048 (  448) ( 1600) | 0x3ffb2390 | 0x3ffb29d0 
	  4 | main                 | running  Q |  15 |   3584 ( 1424) ( 2160) | 0x3ffb2b90 | 0x3ffb3430 
	  5 | pktdump              | bl rx    _ |  14 |   3584 (  560) ( 3024) | 0x3ffb84f0 | 0x3ffb90c0 
	  6 | ipv6                 | bl rx    _ |  12 |   2048 (  784) ( 1264) | 0x3ffb6110 | 0x3ffb66c0 
	  7 | udp                  | bl rx    _ |  13 |   2048 (  572) ( 1476) | 0x3ffb9d3c | 0x3ffba300 
	  8 | wifi                 | bl qempt _ |   1 |   6200 ( 2172) ( 4028) | 0x3ffbec44 | 0x3ffc0240 
	  9 | netif-esp-wifi       | bl anyfl _ |  10 |   2048 (  844) ( 1204) | 0x3ffb45bc | 0x3ffb4b50 
	 10 | RPL                  | bl rx    _ |  13 |   2048 (  504) ( 1544) | 0x3ffb9538 | 0x3ffb9b40 
	    | SUM                  |            |     |  29864 ( 8960) (20904)

The sys_evt thread should be in state bl qemt.

Issues/PRs references

Prerequisite for PR #18439

[maribu]

ACK. Code looks good and test result is provided.

Since this is affecting core, we need a 2nd ACK, though.

[benpicco]

If you add those at the end it will not mess up the thread state mapping in OpenOCD.

The use of this enum in OpenOCD also would speak for adding this unconditionally.
We already have STATUS_ZOMBIE which is only used by mynewt-core

[gschorcht]

If you add those at the end it will not mess up the thread state mapping in OpenOCD.

The use of this enum in OpenOCD also would speak for adding this unconditionally. We already have STATUS_ZOMBIE which is only used by mynewt-core

Hm, I got an assert here

RIOT/core/thread.c

Line 156 in 25a5269

assert(thread->status < STATUS_ON_RUNQUEUE);

when I had them at the end.

[gschorcht]

We already have STATUS_ZOMBIE which is only used by mynewt-core

I couldn't find that.

[benpicco]

It's set by thread_zombify() which is only called by that package.

I guess the assert could be replaced by

assert(thread->status == STATUS_RUNNING || thread->status == STATUS_PENDING);

but I'm not sure if this assumption is backed in elsewhere too 😕

[gschorcht]

According to the documentation, thread_add_to_list may only be called for threads that are not in a runqueue. This is ensured by the assert.

[gschorcht]

It's set by thread_zombify() which is only called by that package.

Ah ok, it is a permanent state.

[gschorcht]

I guess the assert could be replaced by

assert(thread->status == STATUS_RUNNING || thread->status == STATUS_PENDING);

but I'm not sure if this assumption is backed in elsewhere too confused

Hm, >= STATUS_ON_RUNQUEUE is also used several times:

263:        if (!(process->status >= STATUS_ON_RUNQUEUE)) {
268:        if (process->status >= STATUS_ON_RUNQUEUE) {
280:    int on_runqueue = (active_thread->status >= STATUS_ON_RUNQUEUE);
./core/sched.c
146:    if (me->status >= STATUS_ON_RUNQUEUE) {
./core/thread.c

[gschorcht]

If you add those at the end it will not mess up the thread state mapping in OpenOCD.

The use of this enum in OpenOCD also would speak for adding this unconditionally.

I see, it seems not to be a short-term solution. Maybe, we should continue with PR #18439 with state STATUS_MUTEX_BLOCKED for now which also solves the problem.

[gschorcht]

Defining them unconditionally would cost 18 additional bytes in ROM.

[benpicco]

Is this still needed?

[gschorcht]

Is this still needed?

I would say yes. In the FreeRTOS customization layer, STATUS_MUTEX_BLOCKED is being misused for FreeRTOS queue states. However, the PR definitely requires a revision, depending on the progress of PR #18460.