Skip to content

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

Merged
MichelRottleuthner merged 4 commits intoRIOT-OS:masterfrom
JulianHolzwarth:pr/xtimer_mutex_lock_timeout/short_time_fix
Mar 6, 2020
Merged

xtimer/xtimer.c: xtimer_mutex_lock_timeout fix with short timeout#13199
MichelRottleuthner merged 4 commits intoRIOT-OS:masterfrom
JulianHolzwarth:pr/xtimer_mutex_lock_timeout/short_time_fix

Conversation

@JulianHolzwarth
Copy link
Contributor

Tracking: pr #11660

Contribution description

Fixes BUG

This PR implements a new function to test xtimer_mutex_lock_timeout with a short timeout (spinning) and fixes the function xtimer_mutex_lock_timeout because without the fix in this PR the test fails.

This PR also implements a new function _mutex_remove_thread_from_waiting_queue. This function is implemented to remove all code out of xtimer_mutex_lock_timeout and _mutex_timeout that edits the mutex struct. The function _mutex_remove_thread_from_waiting_queue should be moved to mutex.c in the future to removed all code out of xtimer that edits the mutex struct.

Testing procedure

To test the bug fix run the test. To see the bug revert to commit: "tests/xtimer_mutex_lock_timeout: minimal xtimer_mutex_lock_timeout test" and run the test. (the test will fail)

BOARD=native make -C tests/xtimer_mutex_lock_timeout/ flash test

output:

...
> 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

output without fix:

...
> 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
Timeout in expect script at "child.expect("OK")" (tests/xtimer_mutex_lock_timeout/tests/01-run.py:43)

...RIOT/tests/xtimer_mutex_lock_timeout/../../Makefile.include:735: recipe for target 'test' failed
make: *** [test] Error 1

Issues/PRs references

Tracking PR #11660

@JulianHolzwarth
Copy link
Contributor Author

@MichelRottleuthner

}

mutex_lock(mutex);
int ret = _mutex_lock(mutex, mt.blocking);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handing the value of the volatile variable blocking to _mutex_lock will not have the desired effect here. The timeout might deschedule you right before the irq_disable of _mutex_lock is called, leaving you with a blocking call where actually a non-blocking lock should happen. Is there any reason for not just simply doing a non-blocking lock before setting up the timeout timer? This way you would always try to lock the mutex and if the timeout then happens between setting up the timer and locking it, is actually desired behavior to not lock it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the timeout could happen before the mutex_lock but after the first non blocking mutex lock. Meaning there would be the same problem.
The problem with the irq_disable (interupt in function _mutex_lock but before irq_disable) will be fixed in the next PR. Tracking PR #11660
This PR fixes the problems with smaller timeouts.

@MichelRottleuthner MichelRottleuthner added Area: tests Area: tests and testing framework Area: timers Area: timer subsystems Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 7, 2020
This test checks if the function works when the timeout is smaller than XTIMER_BACKOFF and the mutex is already locked.
This means the timer will spin and the timer will shoot before the mutex lock was called.
Then the mutex lock gets called and the timer will not remove the thread from the mutex.
Checking if this case is handled correctly.
Handling timeout smaller than XTIMER_BACKOFF (the timer spins) when the mutex is already locked.
This fixes the test tests/xtimer_mutex_lock_timeout/main.c:mutex_timeout_spin_locked.
@JulianHolzwarth JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/short_time_fix branch from 73bd77f to f3f562b Compare March 5, 2020 18:00
This function tries to remove the thread from a mutex waiting queue.
The value pointed to by unlocked will be set to 1 if the thread was removed from the waiting queue otherwise 0.
@JulianHolzwarth JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/short_time_fix branch from f3f562b to 4d85fa1 Compare March 5, 2020 18:06
Copy link
Contributor

@MichelRottleuthner MichelRottleuthner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and commits look fine, re-tested -> ACK

@MichelRottleuthner MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 5, 2020
@benpicco benpicco added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 5, 2020
@MichelRottleuthner MichelRottleuthner merged commit d8cb8a2 into RIOT-OS:master Mar 6, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020
@JulianHolzwarth JulianHolzwarth deleted the pr/xtimer_mutex_lock_timeout/short_time_fix branch May 8, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants