lkml.org 
[lkml]   [2015]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/4] sched: add trace event for idle injection
    [ CCing Steve Rostedt for the tracing bits ]

    On Fri, Nov 13, 2015 at 11:53:07AM -0800, Jacob Pan wrote:
    > Trace events for idle injection can be used to determine
    > timer activities for checking synchronization. In addition they
    > also helps to determine when the runqueue is throttled.
    >
    > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
    > ---
    > include/linux/sched.h | 5 +++++
    > include/trace/events/sched.h | 25 +++++++++++++++++++++++++
    > kernel/sched/fair.c | 3 +++
    > 3 files changed, 33 insertions(+)
    >
    > diff --git a/include/linux/sched.h b/include/linux/sched.h
    > index ff551a3..99c79bf 100644
    > --- a/include/linux/sched.h
    > +++ b/include/linux/sched.h
    > @@ -3189,6 +3189,11 @@ extern int proc_sched_cfs_idle_inject_duration_handler(struct ctl_table *table,
    > int write,
    > void __user *buffer,
    > size_t *length, loff_t *ppos);
    > +enum cfs_idle_inject_action {
    > + CFS_IDLE_INJECT_TIMER, /* timer sync point */
    > + CFS_IDLE_INJECT_FORCED, /* idle forced in rq */
    > + CFS_IDLE_INJECT_YIELD_SOFTIRQ /* yield to pending softirq */
    > +};
    > #endif
    >
    > #endif
    > diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
    > index 539d6bc..52c11c1 100644
    > --- a/include/trace/events/sched.h
    > +++ b/include/trace/events/sched.h
    > @@ -566,6 +566,31 @@ TRACE_EVENT(sched_wake_idle_without_ipi,
    >
    > TP_printk("cpu=%d", __entry->cpu)
    > );
    > +
    > +#ifdef CONFIG_CFS_IDLE_INJECT
    > +/*
    > + * Tracepoint for idle injection
    > + */
    > +TRACE_EVENT(sched_cfs_idle_inject,
    > +
    > + TP_PROTO(enum cfs_idle_inject_action action, int throttled),
    > +
    > + TP_ARGS(action, throttled),
    > +
    > + TP_STRUCT__entry(
    > + __field(enum cfs_idle_inject_action, action)
    > + __field(int, throttled)
    > + ),
    > +
    > + TP_fast_assign(
    > + __entry->action = action;
    > + __entry->throttled = throttled;
    > + ),
    > +
    > + TP_printk("action:%d throttled:%d", __entry->action, __entry->throttled)
    > +);
    > +#endif
    > +

    One minor nit: can you use key=value (i.e. "throttled=%d") instead for
    consistency with the rest of this file?

    Other than that, I know that Peter suggested an enum for the action,
    but wouldn't it be better to create an EVENT_CLASS and subclass the
    three actions from it? Something like:

    DECLARE_EVENT_CLASS(sched_cfs_idle_inject_template,

    TP_PROTO(int throttled),

    TP_ARGS(throttled),

    TP_STRUCT__entry(
    __field(int, throttled)
    ),

    TP_fast_assign(
    __entry->throttled = throttled;
    ),

    TP_printk("throttled=%d", __entry->throttled)
    );

    DEFINE_EVENT(sched_cfs_idle_inject_template, sched_cfs_idle_inject_timer,
    TP_PROTO(int throttled),
    TP_ARGS(throttled));

    DEFINE_EVENT(sched_cfs_idle_inject_template, sched_cfs_idle_inject_forced,
    TP_PROTO(int throttled),
    TP_ARGS(throttled));

    DEFINE_EVENT(sched_cfs_idle_inject_template,
    sched_cfs_idle_inject_yield_softirq,
    TP_PROTO(int throttled),
    TP_ARGS(throttled));

    > #endif /* _TRACE_SCHED_H */
    >
    > /* This part must be outside protection */
    > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
    > index a0cd777..20027eb 100644
    > --- a/kernel/sched/fair.c
    > +++ b/kernel/sched/fair.c
    > @@ -5248,8 +5248,10 @@ idle:
    > if (in_forced_idle(cfs_rq)) {
    > if (unlikely(local_softirq_pending())) {
    > __unthrottle_cfs_rq(cfs_rq);
    > + trace_sched_cfs_idle_inject(CFS_IDLE_INJECT_YIELD_SOFTIRQ, 1);

    This becomes:

    trace_sched_cfs_idle_inject_yield_softirq(1);

    > goto again;
    > }
    > + trace_sched_cfs_idle_inject(CFS_IDLE_INJECT_FORCED, 1);

    This becomes:
    trace_sched_cfs_idle_inject_forced(1);

    > return NULL;
    > }
    > /*
    > @@ -8432,6 +8434,7 @@ static enum hrtimer_restart idle_inject_timer_fn(struct hrtimer *hrtimer)
    > throttle_rq(cpu);
    > }
    > raw_cpu_write(idle_injected, !status);
    > + trace_sched_cfs_idle_inject(CFS_IDLE_INJECT_TIMER, !status);

    and this would be:

    trace_sched_cfs_idle_inject_timer(!status);

    >
    > return HRTIMER_RESTART;
    > }

    Cheers,
    Javi


    \
     
     \ /
      Last update: 2015-11-19 16:01    [W:5.009 / U:0.280 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site