tests/isr_thread_yield_higher: test behavior from interrupt

[cladmi]

Contribution description

From interrupt context on some architectures, thread_yield_higher yields
immediately, on some others it yields after the end of the interrupt.

However, some cpus implementation like 'atmega_common' have an explicit
'irq_is_in' switch to a different handling.

So I assume either this special handling is not required, or the
architectures that behave differently are wrong.

Question

How should thread_yield_higher behave from interrupt context ?
Note that masking interrupts also let it currently yield, at least on the msba2.

The problem was found as xtimer_mutex_lock_timeout is using thread_yield_higher from interrupts #6158 (when not spinning).
It currently brings back to the main thread with xtimer, _in_handler variable still set to 1, so _timer_callback not being finished.

Testing procedure

Run the test on different cpus/boards.

Boards that have a value of 1, so yield after the interrupt

• arduino-mega2560
• cc2650-launchpad
• frdm-k64f
• frdm-kw41z
• mulle
• nrf52dk
• nucleo-f103rb
• pba-d-01-kw2x
• sltb001a
• stm32f3discovery
• remote-revb (ran manually, it ignores the sent character when automated)
• esp32-wroom-32 (ran manually)
• esp8266-esp-12x (ran manually)
• pic32-wifire (ran without getchar as no input and using pic32-wifire: add support for flashing with pic32prog #9259)
• msba2 (fixed by cpu/arm7_common: Fix thread_yield_higher in ISR #11887)
• ...

Boards that have a value of 0, so yield inside the interrupt

• msba2 cpu/arm7_common: Fix thread_yield_higher in ISR #11887
• wsn430-v1_3b
• ...

FIXED value on `msba2`

#11887

2019-06-28 16:22:13,378 - INFO # Connect to serial port /dev/riot/ttyMSBA2
Welcome to pyterm!
Type '/exit' to exit.
2019-06-28 16:22:14,384 - INFO # main(): This is RIOT! (Version: buildtest)
2019-06-28 16:22:14,385 - INFO # Send character to start
2019-06-28 16:22:16,418 - INFO # Go to sleep
2019-06-28 16:22:17,410 - INFO # Value is 0

RIOT MSP430 hardware initialization complete.
main(): This is RIOT! (Version: 2019.07-devel-847-g9f964-pr/tests/isr_thread_yield_higher)
Send character to start
Go to sleep
Value is 0
Timeout in expect script at "child.expect_exact('Value is 1')" (tests/isr_thread_yield_higher/tests/test.py:16)

Test run on most of the boards, it fails for the msba2.
https://ci-ilab.imp.fu-berlin.de/job/experimental-pull-request-tests/40/consoleText

Issues/PRs references

This different behavior was found when testing #11660
The original implementation was using thread_yield_higher already, but not setting a value after.

[cladmi]

Not sure who to ask more about this issue…

[kaspar030]

How should thread_yield_higher behave from interrupt context ?

Run scheduler after interrupts have finished. The equivalent of sched_context_switch_request = 1;.

[cladmi]

How should thread_yield_higher behave from interrupt context ?

Run scheduler after interrupts have finished. The equivalent of set sched_context_switch_request = 1;.

So the test is indeed showing the right behavior, and msp430 and msba2 are wrong.
I can rewrite the test documentation in that direction and update the API documentation.

If we do not mind loosing the debug messages, this should also be updated to directly use thread_yield_higher right ?

RIOT/core/sched.c

Lines 186 to 209 in c9bf22b

	void sched_switch(uint16_t other_prio)
	{
	thread_t *active_thread = (thread_t *) sched_active_thread;
	uint16_t current_prio = active_thread->priority;
	int on_runqueue = (active_thread->status >= STATUS_ON_RUNQUEUE);
	DEBUG("sched_switch: active pid=%" PRIkernel_pid" prio=%" PRIu16 " on_runqueue=%i "
	", other_prio=%" PRIu16 "\n",
	active_thread->pid, current_prio, on_runqueue, other_prio);
	if (!on_runqueue || (current_prio > other_prio)) {
	if (irq_is_in()) {
	DEBUG("sched_switch: setting sched_context_switch_request.\n");
	sched_context_switch_request = 1;
	}
	else {
	DEBUG("sched_switch: yielding immediately.\n");
	thread_yield_higher();
	}
	}
	else {
	DEBUG("sched_switch: continuing without yield.\n");
	}
	}

[kaspar030]

this should also be updated to directly use thread_yield_higher right?

Hmm. Let's think this through.

It is obviously a common pattern to check if in isr, maybe yield, maybe set "sched_yield_higher". But the isr check is not necessary in all cases. And we should avoid wasting cycles at this level, or at least establish if always checking for isr_is_in() has a measurable impact on context switches.
Maybe a static inline wrapper for if isr_is_in() sched_context_swittch_request = 1; else; thread_yield_higher(); makes sense. Could be overkill.

[cladmi]

It is obviously a common pattern to check if in isr, maybe yield, maybe set "sched_yield_higher". But the isr check is not necessary in all cases. And we should avoid wasting cycles at this level

It is currently checked everytime for atmega_common, esp32, esp8266.
native also has a special handling, but native is special.

RIOT/cpu/atmega_common/thread_arch.c

Lines 238 to 249 in 50ea0d8

	void thread_yield_higher(void)
	{
	if (irq_is_in() == 0) {
	__context_save();
	sched_run();
	__context_restore();
	__asm__ volatile ("ret");
	}
	else {
	sched_context_switch_request = 1;
	}
	}

RIOT/cpu/esp32/thread_arch.c

Lines 212 to 258 in 50ea0d8

	/**
	* If we are already in an interrupt handler, the function simply sets the
	* context switch flag, which indicates that the context has to be switched
	* in the _frxt_int_exit function when exiting the interrupt. Otherwise, we
	* will generate a software interrupt to force the context switch when
	* terminating the software interrupt (see thread_yield_isr).
	*/
	void thread_yield_higher(void)
	{
	/* reset hardware watchdog */
	system_wdt_feed();
	/* yield next task */
	#if defined(ENABLE_DEBUG) && defined(DEVELHELP)
	if (sched_active_thread) {
	DEBUG("%u old task %u %s %u\n", system_get_time(),
	sched_active_thread->pid, sched_active_thread->name,
	sched_active_thread->sp - sched_active_thread-> stack_start);
	}
	#endif
	if (!irq_is_in()) {
	/* generate the software interrupt to switch the context */
	DPORT_WRITE_PERI_REG(DPORT_CPU_INTR_FROM_CPU_0_REG, DPORT_CPU_INTR_FROM_CPU_0);
	}
	else {
	/* set the context switch flag */
	_frxt_setup_switch();
	}
	#if defined(ENABLE_DEBUG) && defined(DEVELHELP)
	if (sched_active_thread) {
	DEBUG("%u new task %u %s %u\n", system_get_time(),
	sched_active_thread->pid, sched_active_thread->name,
	sched_active_thread->sp - sched_active_thread-> stack_start);
	}
	#endif
	/*
	* Instruction fetch synchronize: Waits for all previously fetched load,
	* store, cache, and special register write instructions that affect
	* instruction fetch to be performed before fetching the next instruction.
	*/
	__asm__("isync");
	return;
	}

RIOT/cpu/esp8266/thread_arch.c

Lines 206 to 240 in 50ea0d8

	void thread_yield_higher(void)
	{
	/* reset hardware watchdog */
	system_soft_wdt_feed();
	/* yield next task */
	#if defined(ENABLE_DEBUG) && defined(DEVELHELP)
	if (sched_active_thread) {
	DEBUG("%u old task %u %s %u\n", phy_get_mactime(),
	sched_active_thread->pid, sched_active_thread->name,
	sched_active_thread->sp - sched_active_thread-> stack_start);
	}
	#endif
	if (!irq_is_in()) {
	#ifdef CONTEXT_SWITCH_BY_INT
	WSR(BIT(ETS_SOFT_INUM), interrupt);
	#else
	vPortYield();
	#endif
	}
	else {
	_frxt_setup_switch();
	}
	#if defined(ENABLE_DEBUG) && defined(DEVELHELP)
	if (sched_active_thread) {
	DEBUG("%u new task %u %s %u\n", phy_get_mactime(),
	sched_active_thread->pid, sched_active_thread->name,
	sched_active_thread->sp - sched_active_thread-> stack_start);
	}
	#endif
	return;
	}

[cladmi]

native supposes the function can be called from interrupt context

RIOT/cpu/native/native_cpu.c

Lines 209 to 229 in ecdccdd

	void thread_yield_higher(void)
	{
	sched_context_switch_request = 1;
	if (_native_in_isr == 0) {
	ucontext_t *ctx = (ucontext_t *)(sched_active_thread->sp);
	_native_in_isr = 1;
	if (!native_interrupts_enabled) {
	warnx("thread_yield_higher: interrupts are disabled - this should not be");
	}
	irq_disable();
	native_isr_context.uc_stack.ss_sp = __isr_stack;
	native_isr_context.uc_stack.ss_size = SIGSTKSZ;
	native_isr_context.uc_stack.ss_flags = 0;
	makecontext(&native_isr_context, isr_thread_yield, 0);
	if (swapcontext(ctx, &native_isr_context) == -1) {
	err(EXIT_FAILURE, "thread_yield_higher: swapcontext");
	}
	irq_enable();
	}
	}

[cladmi]

Reference as it was how it was found, xtimer_mutex_lock_timeout is using thread_yield_higher from interrupts #6158 (when not spinning).

[cladmi]

@MrKevinWeiss This is still there, show we do something about it for the release.
@JulianHolzwarth has a fix in https://github.com/RIOT-OS/RIOT/pull/11807/files for at least not use thread_yield_higher which led to getting out oft he interrupt before it was finished.

[MrKevinWeiss]

Something tells me the fix won't be ready that fast. The challenge of doing a summer release is many people are not available. As a worst case I will put it in the release notes.

[cladmi]

I noticed that also thread_yield is called from interrupt context in tests/thread_float which breaks execution on the msba2.

[maribu]

See inline comments

[maribu]

IMO we should also just implement thread_yield_higher as:

static inline void thread_yield_higher(void)
{
    if (irq_is_in()) {
        sched_context_switch_request = 1;
    }
    else {
        thread_yield_higher_arch();
    }
}

And thread_yield_higher_arch() is guaranteed to never be called form ISR and all special handling can be dropped.

[maribu]

LGTM. Feel free to squash!

[fjmolinas]

Another Travis comment popped up, amended and pushed right away. (Only changed the type for value)

[maribu]

Sorry for the late comments. Feel free to squash right in.

[fjmolinas]

@maribu I also noticed that this PR was before test_utils_interactive_sync was included so I removed the extra send line. Can you check the comment on thread_sleep as well?

[maribu]

Is this needed?

[maribu]

I'm referring the stdin module. (Sorry for the confusion.)

[fjmolinas]

Is this one still relevant @maribu?

[maribu]

I think this should get in. It will need a rebase to be passing the CI, though.

[kfessel]

nice test for the expected behavior
needs rebase and optional ztimer