Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events

[Mel Gorman]

On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote:
> When enabling sched trace events via:
> 
>   echo 1 > /sys/kernel/debug/tracing/events/sched/enable
> 
> I see a hang, with the following BUG in the printk buffer:
> 

I was certain I had tested this case because I was monitoring to make
sure the stats were not updating until the tuning knob was used. It was
because the context was unsafe that any printing happened because at the
time trying to enable stats in that context blew up.

> In addition to the deadlock, I think this code has other issues:
> 
> 1. Instead of just warning and allowing the tracepoints to be broken,
>    I'd argue that it would be better to make them work by forcing
>    schedstats enabled and printing a warning about that, which is what's
>    already being done in other similar cases (latencytop and profiling).
>    Otherwise things like "perf sched record" won't have the intended
>    effect.  In fact the comment in the above code snippet seems to agree
>    with me, so maybe that was the original intent.
> 
> 2. It's called in the scheduler hot path from enqueue_entity().
> 
> So change the warning to a call to force_schedstat_enabled(), and do it
> only when the tracepoint gets enabled.
> 
> Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>

I like it! I wasn't aware there was an option to have a registration
callback.

Acked-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>

-- 
Mel Gorman
SUSE Labs