Messages in this thread Patch in this message | | | Date | Mon, 6 May 2019 15:38:12 +0100 | From | Qais Yousef <> | Subject | Re: [PATCH 4/7] sched: Add sched_load_rq tracepoint |
| |
On 05/06/19 11:08, Peter Zijlstra wrote: > On Sun, May 05, 2019 at 12:57:29PM +0100, Qais Yousef wrote: > > > +/* > > + * Following tracepoints are not exported in tracefs and provide hooking > > + * mechanisms only for testing and debugging purposes. > > + */ > > +DECLARE_TRACE(sched_load_rq, > > + TP_PROTO(int cpu, const char *path, struct sched_avg *avg), > > + TP_ARGS(cpu, path, avg)); > > + > > > +DECLARE_TRACE(sched_load_se, > > + TP_PROTO(int cpu, const char *path, struct sched_entity *se), > > + TP_ARGS(cpu, path, se)); > > + > > > +DECLARE_TRACE(sched_overutilized, > > + TP_PROTO(int overutilized), > > + TP_ARGS(overutilized)); > > This doesn't generate any actual userspace because of the lack of > DEFINE_EVENT() ?
Documentation/trace/tracepoints.rst suggests using DEFINE_TRACE(). But using that causes compilation errors because of some magic that is being redefined. Not doing DEFINE_TRACE() gave the intended effect according to the document, so I assumed it's outdated.
kernel/sched/core.c:27:1: note: in expansion of macro ‘DEFINE_TRACE’ DEFINE_TRACE(sched_overutilized); ^~~~~~~~~~~~ ./include/linux/tracepoint.h:287:20: note: previous definition of ‘__tracepoint_sched_overutilized’ was here struct tracepoint __tracepoint_##name \ ^ ./include/linux/tracepoint.h:293:2: note: in expansion of macro ‘DEFINE_TRACE_FN’ DEFINE_TRACE_FN(name, NULL, NULL); ^~~~~~~~~~~~~~~ ./include/trace/define_trace.h:67:2: note: in expansion of macro ‘DEFINE_TRACE’ DEFINE_TRACE(name) ^~~~~~~~~~~~ ./include/trace/events/sched.h:603:1: note: in expansion of macro ‘DECLARE_TRACE’ DECLARE_TRACE(sched_overutilized, ^~~~~~~~~~~~~
DEFINE_EVENT() is only used with TRACE_EVENT() so certainly we don't want it here.
> > > diff --git a/kernel/sched/sched_tracepoints.h b/kernel/sched/sched_tracepoints.h > > new file mode 100644 > > index 000000000000..f4ded705118e > > --- /dev/null > > +++ b/kernel/sched/sched_tracepoints.h > > @@ -0,0 +1,39 @@ > Like with the other newly introduced header files, this one is lacking > the normal include guard.
I was going to add them but then when I looked in sched.h and autogroup.h they had none. So I thought the convention is to not use guard here.
I will add it.
> > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Scheduler tracepoints that are probe-able only and aren't exported ABI in > > + * tracefs. > > + */ > > + > > +#include <trace/events/sched.h> > > + > > +#define SCHED_TP_PATH_LEN 64 > > + > > + > > +static __always_inline void sched_tp_load_cfs_rq(struct cfs_rq *cfs_rq) > > +{ > > + if (trace_sched_load_rq_enabled()) { > > + int cpu = cpu_of(rq_of(cfs_rq)); > > + char path[SCHED_TP_PATH_LEN]; > > + > > + cfs_rq_tg_path(cfs_rq, path, SCHED_TP_PATH_LEN); > > + trace_sched_load_rq(cpu, path, &cfs_rq->avg); > > + } > > +} > > + > > +static __always_inline void sched_tp_load_rt_rq(struct rq *rq) > > +{ > > + if (trace_sched_load_rq_enabled()) { > > + int cpu = cpu_of(rq); > > + > > + trace_sched_load_rq(cpu, NULL, &rq->avg_rt); > > + } > > +} > > + > > +static __always_inline void sched_tp_load_dl_rq(struct rq *rq) > > +{ > > + if (trace_sched_load_rq_enabled()) { > > + int cpu = cpu_of(rq); > > + > > + trace_sched_load_rq(cpu, NULL, &rq->avg_dl); > > + } > > +} > > > +static __always_inline void sched_tp_load_se(struct sched_entity *se) > > +{ > > + if (trace_sched_load_se_enabled()) { > > + struct cfs_rq *gcfs_rq = group_cfs_rq(se); > > + struct cfs_rq *cfs_rq = cfs_rq_of(se); > > + char path[SCHED_TP_PATH_LEN]; > > + int cpu = cpu_of(rq_of(cfs_rq)); > > + > > + cfs_rq_tg_path(gcfs_rq, path, SCHED_TP_PATH_LEN); > > + trace_sched_load_se(cpu, path, se); > > + } > > +} > > These functions really should be called trace_*()
I can rename the wrappers to trace_pelt_load_rq() or sched_trace_pelt_load_rq() as Steve was suggesting.
I assume you're okay with the name of the tracepoints and your comment was about the wrapper above only? ie: sched_load_rq vs pelt_rq.
> > Also; I _really_ hate how fat they are. Why can't we do simple straight
We can create a percpu variable instead of pushing the path on the stack. But this might fail if the trancepoint is called in a preempt enabled path. Also having the percpu variable always hanging when these mostly disabled is ugly.
Maybe there's a better way to handle extracting this path info without copying it here. Let me see if I can improve on this.
> forward things like: > > trace_pelt_cfq(cfq); > trace_pelt_rq(rq); > trace_pelt_se(se); > > And then have the thing attached to the event do the fat bits like > extract the path and whatnot.
Thanks
-- Qais Yousef
| |