On Fri, Jun 19, 2020 at 12:46:41PM -0400 Steven Rostedt wrote: > On Fri, 19 Jun 2020 10:11:20 -0400 > Phil Auld <[email protected]> wrote: > > > > > diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h > > index ed168b0e2c53..a6d9fe5a68cf 100644 > > --- a/include/trace/events/sched.h > > +++ b/include/trace/events/sched.h > > @@ -634,6 +634,10 @@ DECLARE_TRACE(sched_overutilized_tp, > > TP_PROTO(struct root_domain *rd, bool overutilized), > > TP_ARGS(rd, overutilized)); > > > > +DECLARE_TRACE(sched_update_nr_running_tp, > > + TP_PROTO(int cpu, int change, unsigned int nr_running), > > + TP_ARGS(cpu, change, nr_running)); > > + > > #endif /* _TRACE_SCHED_H */ > > > > /* This part must be outside protection */ > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 9a2fbf98fd6f..6f28fdff1d48 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -6,7 +6,10 @@ > > * > > * Copyright (C) 1991-2002 Linus Torvalds > > */ > > + > > +#define SCHED_CREATE_TRACE_POINTS > > #include "sched.h" > > +#undef SCHED_CREATE_TRACE_POINTS > > Because of the macro magic, and really try not to have trace events > defined in any headers. Otherwise, we have weird defines like you are > doing, and it doesn't fully protect it if a C file adds this header and > defines CREATE_TRACE_POINTS first. > > > > > > #include <linux/nospec.h> > > > > @@ -21,9 +24,6 @@ > > > > --- a/kernel/sched/sched.h > > +++ b/kernel/sched/sched.h > > @@ -75,6 +75,15 @@ > > #include "cpupri.h" > > #include "cpudeadline.h" > > > > +#ifdef SCHED_CREATE_TRACE_POINTS > > +#define CREATE_TRACE_POINTS > > +#endif > > +#include <trace/events/sched.h> > > + > > +#ifdef SCHED_CREATE_TRACE_POINTS > > +#undef CREATE_TRACE_POINTS > > +#endif > > + > > #ifdef CONFIG_SCHED_DEBUG > > # define SCHED_WARN_ON(x) WARN_ONCE(x, #x) > > #else > > @@ -1959,6 +1968,7 @@ static inline void add_nr_running(struct rq *rq, > > unsigned count) > > unsigned prev_nr = rq->nr_running; > > > > rq->nr_running = prev_nr + count; > > + trace_sched_update_nr_running_tp(cpu_of(rq), count, rq->nr_running); > > Instead of having sched.h define CREATE_TRACE_POINTS, I would have the > following: > > if (trace_sched_update_nr_running_tp_enabled()) { > call_trace_sched_update_nr_runnig(rq, count); > } > > Then in sched/core.c: > > void trace_sched_update_nr_running(struct rq *rq, int count) > { > trace_sched_update_nr_running_tp(cpu_of(rq), count, rq->nr_running); > } > > The trace_*_enabled() above uses static branches, where the if turns to > a nop (pass through) when disabled and a jmp when enabled (same logic > that trace points use themselves). > > Then you don't need this macro dance, and risk having another C file > define CREATE_TRACE_POINTS and spend hours debugging why it suddenly > broke. >
Awesome, thanks Steve. I was really hoping there was a better way to do that. I try it this way. Cheers, Phil > -- Steve > --
