[TRACKING] xtimer/xtimer.c: xtimer_mutex_lock_timeout fix and improvements by JulianHolzwarth · Pull Request #11660 · RIOT-OS/RIOT

JulianHolzwarth · 2019-06-07T09:21:28Z

This pr tracks the fix for xtimer/xtimer.c: xtimer_mutex_lock_timeout
This pr includes two throwaway test commits.

Contribution description

This pr fixes Bugs in xtimer.c: xtimer_mutex_lock_timeout() and improves it. It addresses concurrency issues. The timer can trigger at a different time than expected.
Before there were problems when the timer spins. This happens when the timeout is less than XTIMER_BACKOFF.

This pr also creates a new static function called _mutex_remove_thread_from_waiting_queue. The function removes a thread from a mutex waiting queue, when the thread waiting for the mutex. This was already used inside of the _mutex_timeout function. It is needed for the xtimer_mutex_lock_timeout function.
This function should go into core/mutex in the future because it modifies queue of the mutex_t struct. (in mutex_t struct comment: @brief Mutex structure. Must never be modified by the user. and in comment of queue @brief The process waiting queue of the mutex. **Must never be changed by the user.**)

This pr also fixes core/mutex.c:_mutex_lock() there was a problem when it gets interrupted before disabling interrupt. This is fixed by giving it a pointer to blocking.

This pr also has tests and throwaway commits to show the bug and fix.

The struct for the xtimer_mutex_lock_timeout function called mutex_thread_t was changed to fix the function and name changes for better understanding.

This pull request is split into smaller parts:

tests/xtimer_mutex_lock_timeout: add simple case test tests/xtimer_mutex_lock_timeout: add simple case test #11679
sys/xtimer/xtimer.c: _mutex_timeout() cleanup sys/xtimer/xtimer.c: _mutex_timeout() cleanup #11807
sys/xtimer/xtimer.c: _mutex_timeout() bug fix sys/xtimer/xtimer.c: _mutex_timeout() bug fix #11992
tests/xtimer_mutex_lock_timeout: New test tests/xtimer_mutex_lock_timeout: New test #12008
xtimer/xtimer.c:_mutex_timeout() improved xtimer/xtimer.c:_mutex_timeout() improved #13185
xtimer/xtimer.c: xtimer_mutex_lock_timeout fix with short timeout xtimer/xtimer.c: xtimer_mutex_lock_timeout fix with short timeout #13199

Testing procedure

BOARD=native make -C tests/xtimer_mutex_lock timeout/ flash test
it outputs:

...
> mutex_timeout_long_unlocked
mutex_timeout_long_unlocked
starting test: xtimer mutex lock timeout
OK

> mutex_timeout_long_locked
mutex_timeout_long_locked
starting test: xtimer mutex lock timeout
OK

> mutex_timeout_long_locked_low
mutex_timeout_long_locked_low
starting test: xtimer mutex lock timeout with thread
threads = 2
THREAD low prio: start
MAIN THREAD: calling xtimer_mutex_lock_timeout
OK
threads = 3
MAIN THREAD: waiting for created thread to end
THREAD low prio: exiting low
threads = 2

> mutex_timeout_short_locked
mutex_timeout_short_locked
starting test: xtimer mutex lock timeout with short timeout and locked mutex
OK

> mutex_timeout_short_unlocked
mutex_timeout_short_unlocked
starting test: xtimer mutex lock timeout with short timeout and unlocked mutex
OK

>

Without the fixes this would fail.

The remove me commits show other bugs.
Go to the commit before "REMOVE ME" commit gets reverted.
Test will work.

Now go to the "REMOVE ME" commit.
Test will fail.

To test the bug in mutex.c:_mutex_lock revert to the second "REMOVE ME" commit with the commit message

REMOVE ME

test commit to show bug in _mutex_lock
when interupted before irq_disable

Issues/PRs references

#11679: first tests
#11807: sys/xtimer/xtimer.c: _mutex_timeout() clean ups
#11992: sys/xtimer/xtimer.c: _mutex_timeout() bug fix
#12008: tests/xtimer_mutex_lock_timeout: New test
#13185: xtimer/xtimer.c:_mutex_timeout() improved
#13199: xtimer/xtimer.c: xtimer_mutex_lock_timeout fix with short timeout

needed for:
- #11977: xtimer/xtimer.c: xtimer_rmutex_lock_timeout
- #11485: [TRACKING] FreeRTOS Adaption Layer

JulianHolzwarth · 2019-06-26T16:06:15Z

rebase because #11679 got merged

cladmi · 2019-06-28T13:36:28Z

sys/xtimer/xtimer.c

+            }
+            sched_set_status(thread, STATUS_PENDING);
+            irq_restore(irqstate);
+            thread_yield_higher();


It looks like it is sched_switch that may need to be used here instead to be working everywhere.
But from the implementation, thread_yield_higher in cpu/atmega_common/thread_arch.c, does the check for is_in_irq…

I will do a dedicated test for this.

Implementation in the CPU is wrong, this code is correct #11759 (comment)

I will change it to sched_switch for now until #11759 is solved/discussed/closed.

done in: sys/xtimer/xtimer.c: _mutex_timeout() clean ups #11807

JulianHolzwarth · 2020-01-24T15:38:36Z

rebased on master

JulianHolzwarth · 2020-03-06T20:13:51Z

The _mutex_remove_thread_from_waiting_queue function will be moved into mutex.c

JulianHolzwarth · 2020-03-06T20:47:26Z

rebased on master

MichelRottleuthner

Tested and was able to reproduce the bug and verify the fix.
Some of the added debug output lines are longer than the absolute maximum of 120 chars and some are outdated (changed variable names).
While the debug messages probably were helpful for working on the test I don't think we should keep (all of) them.
Please squash so we can put Murdock to work.

JulianHolzwarth · 2020-03-26T15:41:30Z

@MichelRottleuthner done. Removed debug msg.

MichelRottleuthner

Sorry I didn't see that before: the commit message is outdated (and contains typos).
Also see below.

MichelRottleuthner · 2020-03-26T16:00:25Z

core/include/mutex.h

 static inline int mutex_trylock(mutex_t *mutex)
 {
-    return _mutex_lock(mutex, 0);
+    uint8_t blocking = 0;


This should be declared as volatile explicitly.

MichelRottleuthner · 2020-03-26T16:00:40Z

core/include/mutex.h

 static inline void mutex_lock(mutex_t *mutex)
 {
-    _mutex_lock(mutex, 1);
+    uint8_t blocking = 1;


MichelRottleuthner · 2020-03-26T16:00:49Z

sys/xtimer/xtimer.c

    unsigned int irqstate = irq_disable();

    mutex_thread_t *mt = (mutex_thread_t *)arg;
+


JulianHolzwarth · 2020-03-26T16:09:47Z

@MichelRottleuthner can I just rebase after the changes? (do you want fixup commits?)

_mutex_lock uses a volatile int pointer for the parameter blocking instead of an int.

MichelRottleuthner · 2020-03-26T17:02:18Z

sure

JulianHolzwarth · 2020-03-26T17:04:53Z

done

MichelRottleuthner

Changes make sense and were tested -> ACK

[11660]: RIOT-OS/RIOT#11660

jcarrano requested a review from cladmi June 7, 2019 11:46

jcarrano added Area: timers Area: timer subsystems Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: tracking The issue tracks and organizes the sub-tasks of a larger effort labels Jun 7, 2019

This was referenced Jun 12, 2019

tests/xtimer_mutex_lock_timeout: add simple case test #11679

Merged

[TRACKING] FreeRTOS Adaption Layer #11485

Open

JulianHolzwarth mentioned this pull request Jun 21, 2019

xtimer: check in xtimer_mutex_lock_timeout timeout callback if a thread blocked on mutex #11543

Closed

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from 3c65bc6 to b9b3cd4 Compare June 26, 2019 16:03

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from b9b3cd4 to a1c5030 Compare June 26, 2019 17:24

cladmi reviewed Jun 28, 2019

View reviewed changes

cladmi mentioned this pull request Jun 28, 2019

tests/isr_thread_yield_higher: test behavior from interrupt #11759

Open

JulianHolzwarth mentioned this pull request Jul 5, 2019

sys/xtimer/xtimer.c: _mutex_timeout() cleanup #11807

Merged

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from f00b6df to 25ffbc4 Compare July 10, 2019 15:55

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from f5b931d to ec2933a Compare July 30, 2019 16:16

JulianHolzwarth mentioned this pull request Aug 7, 2019

xtimer/xtimer.c: xtimer_rmutex_lock_timeout #11977

Merged

tcschmidt assigned MichelRottleuthner Aug 8, 2019

JulianHolzwarth mentioned this pull request Aug 9, 2019

sys/xtimer/xtimer.c: _mutex_timeout() bug fix #11992

Merged

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch 2 times, most recently from 17c6311 to 8ffafdb Compare August 14, 2019 13:58

JulianHolzwarth mentioned this pull request Aug 14, 2019

tests/xtimer_mutex_lock_timeout: New test #12008

Merged

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from 8ffafdb to cbe6a24 Compare August 14, 2019 14:53

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from cbe6a24 to 8853126 Compare January 24, 2020 15:37

JulianHolzwarth mentioned this pull request Jan 24, 2020

xtimer/xtimer.c: xtimer_mutex_lock_timeout fix with short timeout #13199

Merged

haukepetersen assigned kaspar030 Jan 29, 2020

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from 8853126 to 9fc60bd Compare March 6, 2020 20:42

JulianHolzwarth requested review from MichelRottleuthner and kaspar030 as code owners March 6, 2020 20:42

MichelRottleuthner requested changes Mar 25, 2020

View reviewed changes

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from 9fc60bd to 05c88f4 Compare March 26, 2020 15:40

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from 05c88f4 to 40bf85e Compare March 26, 2020 15:43

MichelRottleuthner requested changes Mar 26, 2020

View reviewed changes

core/mutex.c _mutex_lock uses an int pointer

dd6e51b

_mutex_lock uses a volatile int pointer for the parameter blocking instead of an int.

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch from 40bf85e to dd6e51b Compare March 26, 2020 17:03

MichelRottleuthner approved these changes Mar 27, 2020

View reviewed changes

MichelRottleuthner merged commit 80ef241 into RIOT-OS:master Mar 27, 2020

leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020

JulianHolzwarth mentioned this pull request Jun 23, 2020

sys/xtimer: fix xtimer_mutex_lock_timeout corner cases #6441

Closed

chrysn added a commit to RIOT-OS/rust-riot-sys that referenced this pull request May 31, 2022

mutex: Adapt to internal changes after [11660]

aa52953

[11660]: RIOT-OS/RIOT#11660

JulianHolzwarth deleted the pr/xtimer_mutex_lock_timeout/_mutex_remove_thread_from_waiting_queue branch May 8, 2024 19:36

		unsigned int irqstate = irq_disable();

		mutex_thread_t mt = (mutex_thread_t )arg;

Conversation

JulianHolzwarth commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

JulianHolzwarth commented Jun 26, 2019

Uh oh!

cladmi Jun 28, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 28, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 28, 2019

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth Jul 5, 2019 • 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!

JulianHolzwarth commented Jan 24, 2020

Uh oh!

JulianHolzwarth commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JulianHolzwarth commented Mar 6, 2020

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth commented Mar 26, 2020

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichelRottleuthner commented Mar 26, 2020

Uh oh!

JulianHolzwarth commented Mar 26, 2020

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JulianHolzwarth commented Jun 7, 2019 •

edited

Loading

JulianHolzwarth Jul 5, 2019 •

edited

Loading

JulianHolzwarth commented Mar 6, 2020 •

edited

Loading

JulianHolzwarth commented Mar 26, 2020 •

edited

Loading