Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
* Josh Poimboeuf wrote:
> On Wed, Jun 08, 2016 at 09:56:12AM +0200, Ingo Molnar wrote:
> >
> > * Josh Poimboeuf wrote:
> >
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -789,6 +789,13 @@ static void update_curr_fair(struct rq *rq)
> > > update_curr(cfs_rq_of(&rq->curr->se));
> > > }
> > >
> > > +void trace_sched_stat_register(void)
> > > +{
> > > +#ifdef CONFIG_SCHEDSTATS
> > > + force_schedstat_enabled();
> > > +#endif
> > > +}
> >
> > I think it would be cleaner to provide an empty force_schedstat_enabled()
> > definition in sched.h, on !CONFIG_SCHEDSTATS.
>
> Yes, agreed.
>
> > But it might make sense to further decouple schedstats from tracing?
>
> Looking at how we could do that:
>
> - The sched_stat_wait tracepoint is dependent on the wait_start schedstat.
Can we maintain wait_start unconditionally? Having config dependent tracepoints
sucks.
> - The sched_stat_sleep tracepoint is dependent on the sleep_start
> schedstat.
Ditto.
> - The sched_stat_iowait and sched_stat_blocked tracepoints are dependent
> on the block_start schedstat.
Ditto.
> We could move those three schedstats values out of sched_statistics and
> into sched_entity, and then always update them regardless of
> CONFIG_SCHEDSTATS.
>
> That would ensure these tracepoints always work. But then again it
> would add a little bit of runtime overhead.
>
> I don't have a strong opinion either way. Thoughts?
Please do a before/after 'size vmlinux' dump of a defconfig build with tracing
disabled. I.e. how much code overhead does the updating of those variables add?
Thanks,
Ingo
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
On Wed, Jun 08, 2016 at 09:56:12AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf wrote:
>
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -789,6 +789,13 @@ static void update_curr_fair(struct rq *rq)
> > update_curr(cfs_rq_of(&rq->curr->se));
> > }
> >
> > +void trace_sched_stat_register(void)
> > +{
> > +#ifdef CONFIG_SCHEDSTATS
> > + force_schedstat_enabled();
> > +#endif
> > +}
>
> I think it would be cleaner to provide an empty force_schedstat_enabled()
> definition in sched.h, on !CONFIG_SCHEDSTATS.
Yes, agreed.
> But it might make sense to further decouple schedstats from tracing?
Looking at how we could do that:
- The sched_stat_wait tracepoint is dependent on the wait_start schedstat.
- The sched_stat_sleep tracepoint is dependent on the sleep_start
schedstat.
- The sched_stat_iowait and sched_stat_blocked tracepoints are dependent
on the block_start schedstat.
We could move those three schedstats values out of sched_statistics and
into sched_entity, and then always update them regardless of
CONFIG_SCHEDSTATS.
That would ensure these tracepoints always work. But then again it
would add a little bit of runtime overhead.
I don't have a strong opinion either way. Thoughts?
--
Josh
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
On Tue, Jun 07, 2016 at 09:54:48PM +0200, Peter Zijlstra wrote: > On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote: > > 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 > > Forcing them enabled doesn't make them useful per se; some of these > tracepoint rely on previously recoded state, which now wasn't previously > recorded because back then it was disabled. > This is unfortunately true. It means there is a window when the data is unreliable after the stats are enabled. It is assumed that either someone enabling the stats knows that or is willing to enable them on the command line. -- Mel Gorman SUSE Labs
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
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: sta...@vger.kernel.org
> Signed-off-by: Josh Poimboeuf
I like it! I wasn't aware there was an option to have a registration
callback.
Acked-by: Mel Gorman
--
Mel Gorman
SUSE Labs
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
* Josh Poimboeuf wrote:
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -789,6 +789,13 @@ static void update_curr_fair(struct rq *rq)
> update_curr(cfs_rq_of(&rq->curr->se));
> }
>
> +void trace_sched_stat_register(void)
> +{
> +#ifdef CONFIG_SCHEDSTATS
> + force_schedstat_enabled();
> +#endif
> +}
I think it would be cleaner to provide an empty force_schedstat_enabled()
definition in sched.h, on !CONFIG_SCHEDSTATS.
But it might make sense to further decouple schedstats from tracing?
Thanks,
Ingo
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote: > @@ -403,9 +402,10 @@ DECLARE_EVENT_CLASS(sched_stat_runtime, > (unsigned long long)__entry->vruntime) > ); > > -DEFINE_EVENT(sched_stat_runtime, sched_stat_runtime, > - TP_PROTO(struct task_struct *tsk, u64 runtime, u64 vruntime), > - TP_ARGS(tsk, runtime, vruntime)); > +DEFINE_EVENT_FN(sched_stat_runtime, sched_stat_runtime, > + TP_PROTO(struct task_struct *tsk, u64 runtime, u64 vruntime), > + TP_ARGS(tsk, runtime, vruntime), > + trace_sched_stat_register, NULL); > > /* > * Tracepoint for showing priority inheritance modifying a tasks Looking closer at the tracepoints, this one doesn't actually seem to have a dependency on schedstats as far as I can tell. So this hunk can probably be removed. -- Josh
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
On Tue, Jun 07, 2016 at 09:54:48PM +0200, Peter Zijlstra wrote: > On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote: > > 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 > > Forcing them enabled doesn't make them useful per se; some of these > tracepoint rely on previously recoded state, which now wasn't previously > recorded because back then it was disabled. Ah, that's true. Though this issue exists regardless of whether we force enable schedstats or the user manually enables them. It's a side effect of allowing schedstats to be enabled after booting. -- Josh
Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events
On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote: > 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 Forcing them enabled doesn't make them useful per se; some of these tracepoint rely on previously recoded state, which now wasn't previously recorded because back then it was disabled. But yes, maybe this is better...
