rtmutex: enable deadlock detection in ww_mutex_lock functions

PATCH Applied Gustavo Bittencourt 3 participants 7 messages Jan 20, 2015 – Feb 23, 2015 Locking primitives

Share & tools

Gustavo Bittencourt

author

2015-01-20 20:02 UTC #0 lore raw

PATCH 1/1 rtmutex: enable deadlock detection in ww_mutex_lock functions

The functions ww_mutex_lock_interruptible and ww_mutex_lock should return -EDEADLK when faced with a deadlock. To do so, the paramenter detect_deadlock in rt_mutex_slowlock must be TRUE. This patch corrects potential deadlocks when running PREEMPT_RT with nouveau driver.

Kernel version: 3.14.25-rt22

Signed-off-by Gustavo Bittencourt <[email protected]>

---
 kernel/locking/rtmutex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/locking/rtmutex.c +2 -2

index 6c40660..3f6ef91 100644
@@ -1965,7 +1965,7 @@ __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ww_c
 	might_sleep();
 
 	mutex_acquire(&lock->base.dep_map, 0, 0, _RET_IP_);
-	ret = rt_mutex_slowlock(&lock->base.lock, TASK_INTERRUPTIBLE, NULL, 0, ww_ctx);
+	ret = rt_mutex_slowlock(&lock->base.lock, TASK_INTERRUPTIBLE, NULL, 1, ww_ctx);
 	if (ret)
 		mutex_release(&lock->base.dep_map, 1, _RET_IP_);
 	else if (!ret && ww_ctx->acquired > 1)

@@ -1984,7 +1984,7 @@ __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx)
 
 	mutex_acquire_nest(&lock->base.dep_map, 0, 0, &ww_ctx->dep_map,
 			_RET_IP_);
-	ret = rt_mutex_slowlock(&lock->base.lock, TASK_UNINTERRUPTIBLE, NULL, 0, ww_ctx);
+	ret = rt_mutex_slowlock(&lock->base.lock, TASK_UNINTERRUPTIBLE, NULL, 1, ww_ctx);
 	if (ret)
 		mutex_release(&lock->base.dep_map, 1, _RET_IP_);
 	else if (!ret && ww_ctx->acquired > 1)

Signature · 1 line

1.9.1

Paul Gortmaker

2015-01-25 20:55 UTC #1 lore raw

Quoted 5 lines · Gustavo Bittencourt

The functions ww_mutex_lock_interruptible and ww_mutex_lock should return -EDEADLK when faced with a deadlock. To do so, the paramenter detect_deadlock in rt_mutex_slowlock must be TRUE. This patch corrects potential deadlocks when running PREEMPT_RT with nouveau driver.
Kernel version: 3.14.25-rt22

You might want to have a look at this:

http://www.spinics.net/lists/linux-rt-users/msg12259.html

as it also impacts what the deadlock detection flag does.

Paul.
--

Quoted 35 lines

Signed-off-by Gustavo Bittencourt <[email protected]>

---
 kernel/locking/rtmutex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/locking/rtmutex.c +2 -3

index 6c40660..3f6ef91 100644
@@ -1965,7 +1965,7 @@ __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ww_c
        might_sleep();
 
        mutex_acquire(&lock->base.dep_map, 0, 0, _RET_IP_);
-       ret = rt_mutex_slowlock(&lock->base.lock, TASK_INTERRUPTIBLE, NULL, 0, ww_ctx);
+       ret = rt_mutex_slowlock(&lock->base.lock, TASK_INTERRUPTIBLE, NULL, 1, ww_ctx);
        if (ret)
                mutex_release(&lock->base.dep_map, 1, _RET_IP_);
        else if (!ret && ww_ctx->acquired > 1)

@@ -1984,7 +1984,7 @@ __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx)
 
        mutex_acquire_nest(&lock->base.dep_map, 0, 0, &ww_ctx->dep_map,
                        _RET_IP_);
-       ret = rt_mutex_slowlock(&lock->base.lock, TASK_UNINTERRUPTIBLE, NULL, 0, ww_ctx);
+       ret = rt_mutex_slowlock(&lock->base.lock, TASK_UNINTERRUPTIBLE, NULL, 1, ww_ctx);
        if (ret)
                mutex_release(&lock->base.dep_map, 1, _RET_IP_);
        else if (!ret && ww_ctx->acquired > 1)
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gustavo Bittencourt

author

2015-01-26 15:06 UTC #2 lore raw

I'm not sure if those patches are connected, so I am posting my findings for anyone.

ABOUT NOUVEAU AND WW_MUTEX:
The nouveau driver intensely uses the ww_mutex. According the ww-mutex-design.txt documentation, the ww_mutex makes the following steps when try to lock a resource:

1) It calls the ww_mutex_lock (or ww_mutex_lock_interruptible) function for normal lock acquisition.
2) If it returns -EDEADLK, the wounded task must drop all already acquired locks and call the ww_mutex_lock_slow (or ww_mutex_lock_slow_interruptible) function.

In the nouveau driver, those steps happen in the validate_init function (drivers/gpu/drm/nouveau_gem.c) when it calls the functions ttm_bo_reserve (step 1) and ttm_bo_reserve_slowpath (step 2):

validate_init(...)
{
...

		ret = ttm_bo_reserve(&nvbo->bo, true, false, true, &op->ticket);
		if (ret) {
			validate_fini_no_ticket(op, NULL);
			if (unlikely(ret == -EDEADLK)) {
				ret = ttm_bo_reserve_slowpath(&nvbo->bo, true,
							      &op->ticket);

...
}

THE PROBLEM:
When I run the PREEMPT_RT in my computer, it freezes as soon I start some big graphical application (e.g. Firefox or Chrome). The following execution path happens until reaching an infinite loop:

a) nouveau_gem_ioctl_pushbuf()
b) nouveau_gem_pushbuf_validate()
c) validate_init()
d) ttm_bo_reserve()
e) ttm_bo_reserve_nolru()
f) ww_mutex_lock_interruptible()
g) __ww_mutex_lock_interruptible()
h) rt_mutex_slowlock()
i) rt_mutex_handle_deadlock() -> infinite loop

The infinite loop that I mentioned above is the while(1){...} inside the rt_mutex_handle_deadlock function:

static void rt_mutex_handle_deadlock(int res, int detect_deadlock,
struct rt_mutex_waiter *w)

{
    /*
     * If the result is not -EDEADLOCK or the caller requested
     * deadlock detection, nothing to do here.
     */
    if (res != -EDEADLOCK || detect_deadlock)
        return;

    /*
     * Yell lowdly and stop the task right here.
     */
    rt_mutex_print_deadlock(w);
    while (1) {
        set_current_state(TASK_INTERRUPTIBLE);
        schedule();
    }
}

CONCLUSION:
My conclusion was the detect_deadlock should be enabled to the rt_mutex_handle_deadlock function returns -EDEADLK and hence the ww_mutex_lock_interruptible returns -EDEADLK too. BTW, since I have applied my patch, I've not had anymore freezing issue.

PS: I am a kernelnewbie so it is possible that I am writing some BS here.

Quoted 51 lines · Paul Gortmaker

Quoted 5 lines · Gustavo Bittencourt
The functions ww_mutex_lock_interruptible and ww_mutex_lock should return -EDEADLK when faced with a deadlock. To do so, the paramenter detect_deadlock in rt_mutex_slowlock must be TRUE. This patch corrects potential deadlocks when running PREEMPT_RT with nouveau driver.
Kernel version: 3.14.25-rt22
You might want to have a look at this:
http://www.spinics.net/lists/linux-rt-users/msg12259.html
as it also impacts what the deadlock detection flag does.
Paul.
--
Quoted 35 lines
Signed-off-by Gustavo Bittencourt <[email protected]>
---
  kernel/locking/rtmutex.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
kernel/locking/rtmutex.c +2 -3
index 6c40660..3f6ef91 100644
@@ -1965,7 +1965,7 @@ __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ww_c
         might_sleep();
 
         mutex_acquire(&lock->base.dep_map, 0, 0, _RET_IP_);
-       ret = rt_mutex_slowlock(&lock->base.lock, TASK_INTERRUPTIBLE, NULL, 0, ww_ctx);
+       ret = rt_mutex_slowlock(&lock->base.lock, TASK_INTERRUPTIBLE, NULL, 1, ww_ctx);
         if (ret)
                 mutex_release(&lock->base.dep_map, 1, _RET_IP_);
         else if (!ret && ww_ctx->acquired > 1)
@@ -1984,7 +1984,7 @@ __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx)
 
         mutex_acquire_nest(&lock->base.dep_map, 0, 0, &ww_ctx->dep_map,
                         _RET_IP_);
-       ret = rt_mutex_slowlock(&lock->base.lock, TASK_UNINTERRUPTIBLE, NULL, 0, ww_ctx);
+       ret = rt_mutex_slowlock(&lock->base.lock, TASK_UNINTERRUPTIBLE, NULL, 1, ww_ctx);
         if (ret)
                 mutex_release(&lock->base.dep_map, 1, _RET_IP_);
         else if (!ret && ww_ctx->acquired > 1)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sebastian Andrzej Siewior

2015-02-17 17:46 UTC #3 lore raw

Quoted 2 lines · * Gustavo Bittencourt | 2015-01-26 13:06:35 [-0200]:

I'm not sure if those patches are connected, so I am posting my findings for anyone.

could please re-test again v3.18-RT and tell if this patch is still required or not?

Sebastian

Gustavo Bittencourt

author

2015-02-18 00:18 UTC #4 lore raw

Quoted 8 lines · Sebastian Andrzej Siewior

Quoted 2 lines · * Gustavo Bittencourt | 2015-01-26 13:06:35 [-0200]:
I'm not sure if those patches are connected, so I am posting my findings for anyone.
could please re-test again v3.18-RT and tell if this patch is still required or not?
Sebastian

It is still required.

Sebastian Andrzej Siewior

Applied

2015-02-18 09:43 UTC #5 lore raw

Quoted 1 line · * Gustavo Bittencourt | 2015-02-17 22:18:53 [-0200]:

It is still required.

Applied. Please let me know how that nouveau driver behaves. Last thing I know is that it randomly explodes.

Sebastian

Gustavo Bittencourt

author

2015-02-23 16:31 UTC #6 lore raw

Quoted 6 lines · Sebastian Andrzej Siewior

Quoted 1 line · * Gustavo Bittencourt | 2015-02-17 22:18:53 [-0200]:
It is still required.
Applied. Please let me know how that nouveau driver behaves. Last thing I know is that it randomly explodes.

3.18.7-rt2 is working smoothly.