sys/xtimer: fix xtimer_mutex_lock_timeout corner cases by lebrush · Pull Request #6441 · RIOT-OS/RIOT

lebrush · 2017-01-20T13:56:06Z

As discussed in #6428 (comment) if the timeout given is lower than XTIMER_BACKOFF the timer might spin instead of setting an interrupt and the mutex might lock until it's released rather than applying the timeout. This tries to solve this cases (and improves the documentation)

miri64 · 2017-01-20T13:59:25Z

Can you provide a test application testing for the cases you drew out in #6428?

lebrush · 2017-01-20T14:01:35Z

Will do. Please, note that #6428 is still required.

miri64 · 2017-01-20T14:02:33Z

Please, note that #6428 is still required.

Understood it as such ;-)

OlegHahm · 2017-01-20T15:44:14Z

Rebase is required.

OlegHahm · 2017-01-20T15:52:58Z

sys/include/xtimer.h

 * @return       0, when returned after mutex was locked
- * @return       -1, when the timeout occcured
+ * @return       -1, mutex can't be locked right now (timeout <= XTIMER_BACKOFF)
+ * @return       -2, when the timeout occcured


Can you introduce an enum for the error values? This would make it easier to change this later on.

Alternative use errno.h. -EINVAL and -ETIMEDOUT seem fitting.

OlegHahm · 2017-01-20T15:55:37Z

sys/xtimer/xtimer.c

+    /* timeout lower than XTIMER_BACKOFF might cause the code to spin rather
+     * than set it up for interrupt */
+    if (timeout <= XTIMER_BACKOFF) {
+        return (mutex_trylock(mutex) - 1);


Let's say A has the mutex and B calls xtimer_mutex_lock_timeout() with timeout <= XTIMER_BACKOFF. Now A receives an interrupt that causes it to release the mutex. B could do its spin now and lock the mutex, right?

Yes, I've a sketch of that locally, but still not fully happy with it... Will update it asap

lebrush · 2017-01-23T08:58:35Z

I still have to provide some tests for it but [I hope] all corner cases are addressed in this implementation. I removed the two error return values (-1 and -2) and now returns only 0 (success) and -ETIMEDOUT (timed out). Besides if the timeout < XTIMER_BACKOFF it just spins (blocking) and tries to lock the mutex again afterwards ( @OlegHahm I guess that's what you meant). Documentation has been updated accordingly.

@miri64 using ETIMEDOUT requires 4 more bytes (in stm32) than returning just -1 due to the multiplication operation (L253).

sizediffs: http://pastebin.com/bn6et69K

jnohlgard

One comment on premature optimization.

jnohlgard · 2017-01-23T10:10:52Z

sys/xtimer/xtimer.c

-        t.arg = (void *)((mutex_thread_t *)&mt);
-        _xtimer_set64(&t, timeout, timeout >> 32);
+    if (locked || (timeout == 0)) {
+        return (locked - 1) * ETIMEDOUT;


This is an unnecessary optimization in my eyes. Make it a simple if branch and return -ETIMEDOUT if it is locked, otherwise 0.
The current code hurts readability.

I know it's ugly... however with an if branch it generates larger code for most boards:

if (locked) { return 0; } else if (timeout == 0) { return -ETIMEDOUT; }

here is the size diff: http://pastebin.com/VVPjZQfK (+4 +8 bytes larger for most boards but -10 for avr).

Removing the -ETIMEDOUT and returning -1 instead reduces the code size at least by 4 bytes for most boards (many 10 and even 16). Size diff here: http://pastebin.com/dnG7YCJ5)

if (locked || (timeout == 0)) { return (locked - 1); }

I would prefer this solution. What do you think?

lebrush · 2017-01-24T08:00:46Z

Example output of the test

main: tests with unlocked mutex:
thread1 timeout: 0, mutex: unlocked --> OK
thread1 timeout: 0, mutex: locked (1) --> OK
main: locking mutex
main: tests with mutex locked once:
thread1 timeout: 0, mutex: locked (1) --> OK
thread1 timeout: 4, mutex: locked (1) --> OK
thread1 timeout: 5, mutex: locked (1) --> OK
thread1 timeout: 6, mutex: locked (1) --> OK
thread1 timeout: 9, mutex: locked (1) --> OK
thread1 timeout: 10, mutex: locked (1) --> OK
thread1 timeout: 1000000, mutex: locked (1) --> OK
main: test unlocking while waiting
thread1: waiting for mutex
main: unlocking...
thread1: got mutex! --> OK 
main: starting thread2
main: locking mutex
thread2: running
thread2 timeout: 0, mutex: locked (2+) --> OK
thread2 timeout: 4, mutex: locked (2+) --> OK
thread2 timeout: 5, mutex: locked (2+) --> OK
thread2 timeout: 6, mutex: locked (2+) --> OK
thread2 timeout: 9, mutex: locked (2+) --> OK
thread2 timeout: 10, mutex: locked (2+) --> OK
thread2 timeout: 1000000, mutex: locked (2+) --> OK
thread2: waiting for mutex
thread1: unlocking
main: got mutex, unlocking
main: donethread1: done
thread2: got mutex! --> OK 
thread2: done

lebrush · 2017-01-24T08:03:11Z

For some reason when running on native once an xtimer is set and fired I can't set any xtimer anymore. Might be related to #6442 or be the same issue... but the code runs fine on nucleo-f103.

lebrush · 2017-01-30T09:01:08Z

did anyone have a look at this? @OlegHahm are all your comments addressed?

OlegHahm · 2017-01-31T18:36:29Z

On it now.

OlegHahm

Please also provide a pexpect script for the test.

OlegHahm · 2017-01-31T18:40:24Z

sys/include/xtimer.h

 *
- * @note this requires core_thread_flags to be enabled
+ * This will try to lock a mutex. If the mutex is not available immediately or
+ * until a certain amount of time (timeout) the method will return -1


I would rephrase to:

Tries to lock the mutex for a maximum timespan of @p timeout microseconds.

I think your sentence is in this case misleading, it's not clear if the mutex will be locked only during timeout or if it will try to lock it during the timeout. I will use the @p in any case :-)

OlegHahm · 2017-01-31T18:43:15Z

sys/include/xtimer.h

- * @return       0, when returned after mutex was locked
- * @return       -1, when the timeout occcured
+ * @param[in]    mutex    mutex to lock
+ * @param[in]    timeout  timeout in microseconds relative


s/relative//

It reads a bit weird and I think it's common sense that a timeout is specified in relative numbers.

OlegHahm

The test seems to fail.

miri64 · 2017-01-31T19:42:34Z

Test application hangs for me on native, when I tried it first at thread1 timeout: 201, mutex: locked (1) --> , later at thread1 timeout: 1000000, mutex: locked (1) --> :-/

lebrush · 2017-09-15T11:27:39Z

I guess that according to the last mail by @kaspar030 we can dismiss the review by @OlegHahm. @miri64 do you approve the changes?

lebrush · 2017-10-06T08:42:26Z

After 5/6 pings in 8 months I dismiss the review of @OlegHahm . Can someone else please have a look? @miri64 @gebart @vincent-d ?

Not responding to pings on update

smlng · 2018-01-18T16:00:35Z

postponed

miri64 · 2018-04-13T16:05:26Z

@kaspar030 @gebart can you maybe have a look?

miri64 · 2018-06-05T09:41:42Z

Ping @kaspar030 @gebart?

vincent-d · 2019-01-28T10:23:18Z

@lebrush I'm really sorry for what happened here. I was not really confident in reviewing this one at the time you opened it, and this stalled for no good reason.

We encountered one of the issues which are solved here and a colleague opened #10872. I've run a couple of tests and when trying to reproduced I encountered another issue which is also fixed by your PR.

Would you mind rebasing this then we could merge? If you don't have time, could someone else take this over?

vincent-d

Tested on nucleo-f207zg. Please rebase.

ACK

miri64 · 2019-02-18T13:50:32Z

@lebrush ping?

cladmi · 2019-05-17T14:28:38Z

sys/xtimer/xtimer.c

+    /* a timeout lower than XTIMER_BACKOFF causes the xtimer to spin rather
+     * than to set a timer for interrupt. Hence, we shall make the mutex_lock
+     * call blocking only when the interrupt didn't occur yet. */
+    locked = _mutex_lock(mutex, (mt.timeout == NO_TIMEOUT));


While looking at the original code @JulianHolzwarth we noticed the issue with this one, and looked for existing references. But actually this does not solve the concurrency completely.
Between the evaluation of == and the actual irq_disable in the function, the callback can be triggered.

He will provide a PR for the needed change in core to evaluate a volatile condition when the interrupt are disabled.
This would however maybe make using 'mt.timeout' not possible and may require another variable.

miri64 · 2020-06-23T11:46:51Z

Is this still being worked on? Is this fixed? @JulianHolzwarth @kaspar030 maybe you have some insight on this.

JulianHolzwarth · 2020-06-23T12:23:41Z

@miri64 this should be fixed. #11660

miri64 · 2020-06-23T12:47:31Z

Then let's close this.

lebrush mentioned this pull request Jan 20, 2017

xtimer: fix mutex unlocking in _mutex_timeout #6428

Merged

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: timers Area: timer subsystems labels Jan 20, 2017

miri64 added this to the Release 2017.01 milestone Jan 20, 2017

miri64 assigned vincent-d and kaspar030 Jan 20, 2017

OlegHahm requested changes Jan 20, 2017

View reviewed changes

lebrush force-pushed the fix/xtimer-mutex branch from 92f5932 to 2d8a05b Compare January 23, 2017 08:50

jnohlgard reviewed Jan 23, 2017

View reviewed changes

lebrush force-pushed the fix/xtimer-mutex branch 2 times, most recently from ad9ae47 to 493c6f6 Compare January 24, 2017 07:58

lebrush mentioned this pull request Jan 24, 2017

cpu/native: timer interrupt issue #6442

Closed

lebrush force-pushed the fix/xtimer-mutex branch from 493c6f6 to 8501eee Compare January 24, 2017 08:26

lebrush changed the title ~~sys/xtimer: fix mutex locking "before" corner case~~ sys/xtimer: fix xtimer_mutex_lock_timeout corner cases Jan 24, 2017

lebrush force-pushed the fix/xtimer-mutex branch from 8501eee to 6c5b42d Compare January 24, 2017 16:34

PeterKietzmann added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 26, 2017

OlegHahm previously requested changes Jan 31, 2017

View reviewed changes

OlegHahm reviewed Jan 31, 2017

View reviewed changes

PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Feb 1, 2017

lebrush added 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 Oct 6, 2017

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng modified the milestones: Release 2018.01, Release 2018.04 Jan 18, 2018

smlng added 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 Jan 18, 2018

kaspar030 removed this from the Release 2018.04 milestone Apr 22, 2018

vincent-d mentioned this pull request Jan 28, 2019

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

Closed

vincent-d approved these changes Jan 28, 2019

View reviewed changes

vincent-d mentioned this pull request Mar 21, 2019

sys/xtimer: fix xtimer_mutex_lock_timeout corner cases (#2) #11225

Closed

cladmi reviewed May 17, 2019

View reviewed changes

miri64 added the State: stale State: The issue / PR has no activity for >185 days label Jun 23, 2020

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jun 23, 2020

miri64 closed this Jun 23, 2020

Conversation

lebrush commented Jan 20, 2017

Uh oh!

miri64 commented Jan 20, 2017

Uh oh!

lebrush commented Jan 20, 2017

Uh oh!

miri64 commented Jan 20, 2017

Uh oh!

OlegHahm commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lebrush commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lebrush commented Jan 24, 2017

Uh oh!

lebrush commented Jan 24, 2017

Uh oh!

lebrush commented Jan 30, 2017

Uh oh!

OlegHahm commented Jan 31, 2017

Uh oh!

OlegHahm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 31, 2017

Uh oh!

lebrush commented Sep 15, 2017

Uh oh!

lebrush commented Oct 6, 2017

Uh oh!

smlng commented Jan 18, 2018

Uh oh!

miri64 commented Apr 13, 2018

Uh oh!

miri64 commented Jun 5, 2018

Uh oh!

vincent-d commented Jan 28, 2019

Uh oh!

vincent-d left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

miri64 commented Jun 23, 2020

lebrush commented Jan 23, 2017 •

edited

Loading

JulianHolzwarth commented Jun 23, 2020 •

edited

Loading