Messages in this thread | | | Date | Wed, 1 Oct 2014 20:35:49 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 10/11] sched: Debug nested sleeps |
| |
On 10/01, Peter Zijlstra wrote: > > On Tue, Sep 30, 2014 at 11:47:32PM +0200, Oleg Nesterov wrote: > > > > > This is minor, but this way CONFIG_DEBUG_ATOMIC_SLEEP will not imply > > > > a subtle behavioural change. > > > > > > You mean the __set_current_state() that's extra? > > > > Yes, and note that it only does __set_current_state(RUNNING) if > > CONFIG_DEBUG_ATOMIC_SLEEP. This means that disabling/enabling this > > option can, silently hide/uncover a bug. > > > > > I would actually argue > > > to keep that since it makes the 'problem' much worse. > > > > OK, I won't insist, but could you explain why the suggested change can > > make the problem (and which problem ;) worse? > > Sure, so the trivial problem is not actually going to sleep in the outer > wait primitive because the inner wait primitive reset ->state to > TASK_RUNNING.
But this means that fixup_sleep() must not be used?
> So by always setting the ->state to TASK_RUNNING it never goes to sleep > and it'll revert to spinning,
But I tried to suggest to not set TASK_RUNNING?
Peter, I am sorry for wasting your time, this is really minor, but still I'd like to understand.
Let me try again. With this series we have
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP #define fixup_sleep() __set_current_state(TASK_RUNNING) #else #define fixup_sleep() do { } while (0) #endif
and this means that we do not need __set_current_state(RUNNING) for correctness, just we want to shut up the warning in __might_sleep(). This is fine (and the self-documenting helper is nice), but this means that CONFIG_DEBUG_ATOMIC_SLEEP adds a subtle difference.
For example, let's suppose that we do not have 01/11 which fixes mutex_lock(). Then this code
set_current_state(TASK_UNINTERRUPTIBLE); ... fixup_sleep(); ... mutex_lock(some_mutex);
can hang, but only if !CONFIG_DEBUG_ATOMIC_SLEEP.
So perhaps it makes sense to redefine it
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP #define fixup_sleep() (current->task_state_change = 0) #else #define fixup_sleep() do { } while (0) #endif
and change __might_sleep()
- if (WARN(current->state != TASK_RUNNING, + if (WARN(current->state != TASK_RUNNING && current->task_state_change != 0,
?
Oleg.
| |