xtimer: fix mutex unlocking in _mutex_timeout

[vincent-d]

Changes proposed in #6427.

[lebrush]

Really good catch! 👍

I ignored that the MUTEX_LOCK is not always the tip of the list! It is only when the mutex is owned by one thread. If there are more threads waiting, the tip is NULL. I guess this is done to be able to use thread_add_to_list cleanly.

There's one race condition still: if the timer fires between setting the timer and locking the mutex might create inconsistencies in the mutex queue. Just test that list_remove really removed a node

list_node_t* node = list_remove(...)
if (node != NULL && mt->mutex->queue.next == NULL) {
    // we were the last node in the queue but the mutex shall still be owned by someone else
    mt->mutex->queue.next = MUTEX_LOCKED;
}

[miri64]

Proxy-ACK, please squash

[OlegHahm]

Please but each condition into parentheses.

[vincent-d]

addressed

[vincent-d]

@OlegHahm comment addressed and squashed

[lebrush]

Please wait before merging this one

…

On 19 Jan 2017 18:28, "Vincent Dupont" ***@***.***> wrote: @OlegHahm <https://github.com/OlegHahm> comment addressed and squashed — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#6428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABXvYvQ_K4XAXToi9dlAWxFU7aSA4Brxks5rT50hgaJpZM4LoNsW> .

[OlegHahm]

@lebrush requested to wait

[OlegHahm]

@lebrush, waiting for what?

[lebrush]

Sorry for the delay. I wanted to make sure that all bugs here are addressed, since I introduced the feature and the bugs :-)

After reviewing the code once more I made a table with the following corner cases: what happens if the timer fires before calling mutex_lock and what if fires after calling mutex_lock. Since the code within xtimer_mutex_lock_timeout is not atomic we have some possible race conditions.

Mutex was	Before mutex_lock	After mutex_lock
UNLOCKED	mutex: LOCKED (1), return: -1	-
LOCKED (1)	mutex_lock blocks until unlocked; return -1	mutex: LOCKED (1), return -1 (with this patch)
LOCKED (2+)	mutex_lock blocks until unlocked; return -1	mutex: LOCKED (2+); return -1

This patch solves the case LOCKED (1) | After mutex_lock.

Mutex was	Before mutex_lock	After mutex_lock
UNLOCKED	WRONG	-
LOCKED (1)	WRONG	OK
LOCKED (2+)	WRONG	OK

The implementation of xtimer might decide to spin if the timeout is to small. Hence we need to solve the Before column as well. One solution would be to replace the if (timeout != 0) with:

if (timeout > XTIMER_BACKOFF) 

With these two fixes then we are:

Mutex was	Before mutex_lock	After mutex_lock
UNLOCKED	OK	-
LOCKED (1)	OK	OK
LOCKED (2+)	OK	OK

The last consideration I could think of is the case that the timer triggers between the return of mutex_lock (isr safe) and xtimer_remove (isr safe). The mutex would have the correct state but the method would return -1. How probable is that this happens?

I think it is necessary to include a fix for the XTIMER_BACKOFF as well. It's not as likely to happen as this one but still. Maybe a separate PR in order not to block this one any longer?

[lebrush]

TL;DR I would merge this one and contemplate the other cases in i.e. #6441

[OlegHahm]

What was the initial reason not to call mutex_unlock() in the callback?

[lebrush]

Thread A has the mutex, Thread B waits for it. Thread C sets a mutex lock with timeout. If the callback occurs before A has released the mutex and you call mutex_unlock both threads A and B "have" the mutex now, right?

[OlegHahm]

Ah, I think I misunderstood xtimer_mutex_lock_timeout() (and I think the documentation is not very accurate). I thought the function would lock the timeout for a certain time (which doesn't have much use cases, I guess).

[lebrush]

Good point. Will improve the docs in #6441

[OlegHahm]

This has to be MUTEX_LOCKED because we assume that someone still owns the mutex, right?

[lebrush]

Exactly.

When the mutex is unlocked, queue.next is NULL. mutex -> NULL
When the mutex is locked by 1 thread queue.next is MUTEX_LOCKED. mutex -> LOCKED
When the mutex is locked by 2+ threads queue.next is the mt->thread->rq_entry of a thread and the last .next is NULL. mutex -> thread -> (thread -> ...) NULL

If node != NULL we were removed from the list, hence 2+ threads where in the list. At the same time if queue.next is NULL means that we were the last thread in the list and we need to set queue.next to MUTEX_LOCKED as there's now 1 thread still owning the mutex.

[vincent-d]

Yes, otherwise you could have locked it and then the timeout never fire.

[OlegHahm]

ACK