Messages in this thread | | | Date | Thu, 2 Oct 2014 23:34:25 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 10/11] sched: Debug nested sleeps |
| |
On 10/02, Peter Zijlstra wrote: > > On Wed, Oct 01, 2014 at 08:35:49PM +0200, Oleg Nesterov wrote: > > > > 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. > > Right, but we should not use fixup_sleep() in this case,
(well, I am not really sure but this is off-topic and I agree this needs another discussion)
> because its an > actual proper bug, we should fix it, not paper over it.
Exactly! this is what I meant: CONFIG_DEBUG_ATOMIC_SLEEP will hide the bug we need to fix.
> > 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, > > > > ? > > So I'm hesitant to go that way because it adds extra state dependency.
OK. We can always reconsider this later. I spammed you only because I wanted to understand what did me/you/both missed in this discussion.
> What if someone 'forgets' to use the *set*state() helpers.
Yes, this is true. Although we want to fix them anyway, if nothing else for this warning in might_sleep().
Oleg.
| |