sys: add mutex lock with timeout function by lebrush · Pull Request #6158 · RIOT-OS/RIOT

lebrush · 2016-11-24T07:38:40Z

This implements mutex locking with a timeout, which is required for the semaphore implementation without IPC (#6155).

It's been split into a separate PR as suggested by @miri64

Depends on: #6211
Required by: #6155

miri64 · 2016-11-24T09:31:59Z

core/include/thread.h

+ * @brief Remove thread from list (internal)
+ *
+ * This will remove @p thread from @p list. It reuses the thread's rq_entry
+ * field. Used internally timeout mutex implementation.


Can you directly reference the function here?

Used internally by @ref xtimer_mutex_lock_timeout() "timeout mutex implementation".

OlegHahm · 2016-11-24T12:13:18Z

core/include/thread.h

 void thread_add_to_list(list_node_t *list, thread_t *thread);

+/**
+ * @brief Remove thread from list (internal)


Maybe @internal?

thread_add_to_list is not @internal. Should I update both?

OlegHahm · 2016-11-24T12:15:07Z

@BytesGalore, didn't we have a discussion about this feature some years before?

lebrush · 2016-11-25T17:00:33Z

What was the outcome of the discussion @OlegHahm ? :-)

BytesGalore · 2016-11-30T08:53:49Z

@OlegHahm, @lebrush I really can't remember, but I guess it was related to the requirement of have condition variables. Since solely using mutex could result in a race and in some cases in a deadlock.

lebrush · 2016-11-30T09:40:24Z

I see... I don't have such a deep overview of the sched and thread modules as @kaspar030 and @BytesGalore, can you guys think of any race/deadlock condition with the proposed implementation?

Also, I've to see if I've to rebase to accommodate the latest changes in the xtimer.

kaspar030 · 2016-11-30T09:44:27Z

@kaspar030 and @BytesGalore, can you guys think of any race/deadlock condition with the proposed implementation?

No, @BytesGalore, can you elaborate?

lebrush · 2016-12-13T10:02:47Z

What is holding this back? I'd be nice to have this and #6155 merged for the next release

kaspar030 · 2016-12-13T12:53:53Z

core/thread.c

    list->next = new_node;
 }

+void thread_remove_from_list(list_node_t *list, thread_t *thread)


Why not use clist_remove()?

I didn't consider it because: the list is not a circular list and because when added is using a thread_add_to_list which is in the thread module (I assume that this is done this way to lower inter-dependencies).

Alternatively: why not add a list_remove() implementation? Seems a lot cleaner.

thread_add_to_list() is taking the thread's priority into account and adds the thread so afterwards the list is sorted by priority.

+1 for adding "list_remove()" to list.h! and using that!

adding "list_remove()" to list.h

It's probably best to do that in a seperate PR.

Agree! Will do :-) now we'll have a PR which depends in this PR which depends in another PR, yeeey 🎉

Now this will depend on #6211

Agree! Will do :-) now we'll have a PR which depends in this PR which depends in another PR, yeeey 🎉

Welcome to RIOT ;-P

kaspar030 · 2016-12-13T12:56:04Z

sys/xtimer/xtimer.c

+
+static void _mutex_timeout(void *arg)
+{
+    unsigned irqstate = irq_disable();


This is run in ISR context, so no need for disabling IRQs.

You do need, otherwise you might run in a race condition in case an interrupt occurs while serving this interrupt. Imagine: an that the other interrupt occurs while removing the mutex from the queue and that interrupt does the same operation (i.e. by calling mutex_unlock).

@lebrush RIOT currently doesn't allow nested interrupts, so while one ISR is running, no other will interrupt.

Again what learnt ;-) Will fix

kaspar030 · 2016-12-13T12:56:29Z

sys/xtimer/xtimer.c

 {
    msg_t tmsg;
    xtimer_t t;
+


unrelated change

kaspar030 · 2016-12-13T12:57:07Z

sys/xtimer/xtimer.c

+typedef struct {
+    mutex_t *mutex;
+    thread_t *thread;
+    int timeout;


what is this used for?

When the mutex times out, this flag is set in order to indicate whether it timed out or not (see L257).

(after rebase, is line 255)

lebrush · 2016-12-14T08:01:31Z

@kaspar030 rebased onto latest master and squashed changes

miri64 · 2017-01-10T14:04:39Z

@kaspar030 ping?

miri64 · 2017-01-12T12:21:04Z

#6211 got merged

lebrush · 2017-01-12T12:21:48Z

Rebased. @kaspar030 any other change requested?

kaspar030 · 2017-01-12T13:11:43Z

ACK.

lebrush · 2017-01-12T15:54:13Z

Can anyone trigger Murdock again, please?

Running './dist/tools/pr_check/pr_check.sh master' x
Command output:

	Pull request is waiting for another pull request according to its labels set on GitHub

lebrush force-pushed the mutex-timeout branch from baaaf55 to 94c15e1 Compare November 24, 2016 07:44

lebrush mentioned this pull request Nov 24, 2016

Semaphore implementation without IPC #6155

Merged

miri64 added this to the Release 2017.01 milestone Nov 24, 2016

miri64 assigned kaspar030 Nov 24, 2016

miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: timers Area: timer subsystems labels Nov 24, 2016

miri64 reviewed Nov 24, 2016

View reviewed changes

OlegHahm reviewed Nov 24, 2016

View reviewed changes

OlegHahm assigned BytesGalore Nov 24, 2016

lebrush mentioned this pull request Dec 13, 2016

xtimer: timer already in the list #4841

Closed

kaspar030 requested changes Dec 13, 2016

View reviewed changes

kaspar030 changed the title ~~Mutex timeout~~ sys: add mutex lock with timeout function Dec 13, 2016

lebrush force-pushed the mutex-timeout branch from 561de3f to f6b9cb1 Compare December 14, 2016 08:00

lebrush force-pushed the mutex-timeout branch 2 times, most recently from bc5cfd0 to 6e33b59 Compare December 14, 2016 09:46

lebrush mentioned this pull request Dec 14, 2016

core/list: add remove method #6211

Merged

miri64 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 Dec 14, 2016

miri64 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 10, 2017

OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 10, 2017

miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2017

sys/xtimer: implement mutex lock with timeout

17d6d2a

lebrush force-pushed the mutex-timeout branch from 6e33b59 to 17d6d2a Compare January 12, 2017 12:20

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jan 12, 2017

kaspar030 approved these changes Jan 12, 2017

View reviewed changes

kaspar030 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 12, 2017

miri64 merged commit 3ba1218 into RIOT-OS:master Jan 12, 2017

lebrush deleted the mutex-timeout branch January 13, 2017 08:26

cladmi mentioned this pull request Jul 8, 2019

tests/isr_thread_yield_higher: test behavior from interrupt #11759

Open

cladmi mentioned this pull request Jul 23, 2019

cpu/arm7_common: Fix thread_yield_higher in ISR #11887

Merged

Conversation

lebrush commented Nov 24, 2016 • edited by haukepetersen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Nov 24, 2016

Uh oh!

lebrush commented Nov 25, 2016

Uh oh!

BytesGalore commented Nov 30, 2016

Uh oh!

lebrush commented Nov 30, 2016

Uh oh!

kaspar030 commented Nov 30, 2016

Uh oh!

lebrush commented Dec 13, 2016

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lebrush commented Dec 14, 2016

Uh oh!

miri64 commented Jan 10, 2017

Uh oh!

miri64 commented Jan 12, 2017

Uh oh!

lebrush commented Jan 12, 2017

Uh oh!

kaspar030 commented Jan 12, 2017

Uh oh!

lebrush commented Jan 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lebrush commented Nov 24, 2016 •

edited by haukepetersen

Loading