The new tracepoint allows tracking PELT signals at rq level for all
scheduling classes.
Signed-off-by: Qais Yousef <[email protected]>
---
include/trace/events/sched.h | 9 ++++++++
kernel/sched/fair.c | 9 ++++++--
kernel/sched/pelt.c | 4 ++++
kernel/sched/sched_tracepoints.h | 39 ++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 2 deletions(-)
create mode 100644 kernel/sched/sched_tracepoints.h
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 9a4bdfadab07..2be4c471c6e9 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -587,6 +587,15 @@ TRACE_EVENT(sched_wake_idle_without_ipi,
TP_printk("cpu=%d", __entry->cpu)
);
+
+/*
+ * 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));
+
#endif /* _TRACE_SCHED_H */
/* This part must be outside protection */
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 2b4963bbeab4..e1e0cc7db7f6 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -21,8 +21,7 @@
* Copyright (C) 2007 Red Hat, Inc., Peter Zijlstra
*/
#include "sched.h"
-
-#include <trace/events/sched.h>
+#include "sched_tracepoints.h"
/*
* Targeted preemption latency for CPU-bound tasks:
@@ -3139,6 +3138,8 @@ static inline int propagate_entity_load_avg(struct sched_entity *se)
update_tg_cfs_util(cfs_rq, se, gcfs_rq);
update_tg_cfs_runnable(cfs_rq, se, gcfs_rq);
+ sched_tp_load_cfs_rq(cfs_rq);
+
return 1;
}
@@ -3291,6 +3292,8 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
add_tg_cfs_propagate(cfs_rq, se->avg.load_sum);
cfs_rq_util_change(cfs_rq, flags);
+
+ sched_tp_load_cfs_rq(cfs_rq);
}
/**
@@ -3310,6 +3313,8 @@ static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
add_tg_cfs_propagate(cfs_rq, -se->avg.load_sum);
cfs_rq_util_change(cfs_rq, 0);
+
+ sched_tp_load_cfs_rq(cfs_rq);
}
/*
diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c
index befce29bd882..302affb14302 100644
--- a/kernel/sched/pelt.c
+++ b/kernel/sched/pelt.c
@@ -26,6 +26,7 @@
#include <linux/sched.h>
#include "sched.h"
+#include "sched_tracepoints.h"
#include "pelt.h"
/*
@@ -292,6 +293,7 @@ int __update_load_avg_cfs_rq(u64 now, struct cfs_rq *cfs_rq)
cfs_rq->curr != NULL)) {
___update_load_avg(&cfs_rq->avg, 1, 1);
+ sched_tp_load_cfs_rq(cfs_rq);
return 1;
}
@@ -317,6 +319,7 @@ int update_rt_rq_load_avg(u64 now, struct rq *rq, int running)
running)) {
___update_load_avg(&rq->avg_rt, 1, 1);
+ sched_tp_load_rt_rq(rq);
return 1;
}
@@ -340,6 +343,7 @@ int update_dl_rq_load_avg(u64 now, struct rq *rq, int running)
running)) {
___update_load_avg(&rq->avg_dl, 1, 1);
+ sched_tp_load_dl_rq(rq);
return 1;
}
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 @@
+/* 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);
+ }
+}
--
2.17.1
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() ?
> 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.
> +/* 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_*()
Also; I _really_ hate how fat they are. Why can't we do simple straight
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.
On Mon, May 06, 2019 at 11:08:59AM +0200, Peter Zijlstra wrote:
> Also; I _really_ hate how fat they are. Why can't we do simple straight
> 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.
ARGH, because we don't export any of those data structures (for good
reason).. bah I hate all this.
On Mon, 6 May 2019 11:08:59 +0200
Peter Zijlstra <[email protected]> wrote:
> These functions really should be called trace_*()
>
> Also; I _really_ hate how fat they are. Why can't we do simple straight
> 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.
I'd like to avoid functions called "trace_*" that are not trace events.
It's getting confusing when I see a "trace_*()" function and then go
look for the corresponding TRACE_EVENT() just to find out that one does
not exist.
sched_trace_*() maybe?
-- Steve
On Mon, 6 May 2019 15:42:00 +0100
Qais Yousef <[email protected]> wrote:
> I can control that for the wrappers I'm introducing. But the actual tracepoint
> get the 'trace_' part prepended automatically by the macros.
>
> ie DECLARE_TRACE(pelt_rq, ...) will automatically generate a function called
> trace_pelt_se(...)
>
> Or am I missing something?
No trace comes from the trace points.
So basically, we are going back to having tracepoints without
associated trace events. Which basically is just saying "we want trace
events here, but don't want an API". Of course, we can create a module
that can attach to them and create the trace events as well.
I'm not a big fan of this, but I'll let Peter decide.
-- Steve
On 05/06/19 09:52, Steven Rostedt wrote:
> On Mon, 6 May 2019 11:08:59 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > These functions really should be called trace_*()
> >
> > Also; I _really_ hate how fat they are. Why can't we do simple straight
> > 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.
>
> I'd like to avoid functions called "trace_*" that are not trace events.
> It's getting confusing when I see a "trace_*()" function and then go
> look for the corresponding TRACE_EVENT() just to find out that one does
> not exist.
>
> sched_trace_*() maybe?
I can control that for the wrappers I'm introducing. But the actual tracepoint
get the 'trace_' part prepended automatically by the macros.
ie DECLARE_TRACE(pelt_rq, ...) will automatically generate a function called
trace_pelt_se(...)
Or am I missing something?
Thanks
--
Qais Yousef
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
On 05/06/19 10:46, Steven Rostedt wrote:
> On Mon, 6 May 2019 15:42:00 +0100
> Qais Yousef <[email protected]> wrote:
>
> > I can control that for the wrappers I'm introducing. But the actual tracepoint
> > get the 'trace_' part prepended automatically by the macros.
> >
> > ie DECLARE_TRACE(pelt_rq, ...) will automatically generate a function called
> > trace_pelt_se(...)
> >
> > Or am I missing something?
>
> No trace comes from the trace points.
If you want I can do something like below to help create a distinction. It is
none enforcing though.
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 9c3186578ce0..f654ced20045 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -232,6 +232,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
*/
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
extern struct tracepoint __tracepoint_##name; \
+ static inline void tp_##name(proto) __alias(trace_##name); \
static inline void trace_##name(proto) \
{ \
if (static_key_false(&__tracepoint_##name.key)) \
Another option is to extend DECLARE_TRACE() to take a new argument IS_TP and
based on that select the function name. This will be enforcing but I will have
to go fixup many places.
Of course 'TP' can be replaced with anything more appealing.
--
Qais Yousef
On Mon, 6 May 2019 16:33:17 +0100
Qais Yousef <[email protected]> wrote:
> On 05/06/19 10:46, Steven Rostedt wrote:
> > On Mon, 6 May 2019 15:42:00 +0100
> > Qais Yousef <[email protected]> wrote:
> >
> > > I can control that for the wrappers I'm introducing. But the actual tracepoint
> > > get the 'trace_' part prepended automatically by the macros.
> > >
> > > ie DECLARE_TRACE(pelt_rq, ...) will automatically generate a function called
> > > trace_pelt_se(...)
> > >
> > > Or am I missing something?
> >
> > No trace comes from the trace points.
Re-reading that line, I see I totally didn't express what I meant :-p
>
> If you want I can do something like below to help create a distinction. It is
> none enforcing though.
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 9c3186578ce0..f654ced20045 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -232,6 +232,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> */
> #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
> extern struct tracepoint __tracepoint_##name; \
> + static inline void tp_##name(proto) __alias(trace_##name); \
> static inline void trace_##name(proto) \
> { \
> if (static_key_false(&__tracepoint_##name.key)) \
>
>
> Another option is to extend DECLARE_TRACE() to take a new argument IS_TP and
> based on that select the function name. This will be enforcing but I will have
> to go fixup many places.
>
> Of course 'TP' can be replaced with anything more appealing.
No no no, I meant to say...
"No that's OK. The "trace_" *is* from the trace points, and trace
events build on top of them."
-- Steve
On 05/06/19 12:01, Steven Rostedt wrote:
> On Mon, 6 May 2019 16:33:17 +0100
> Qais Yousef <[email protected]> wrote:
>
> > On 05/06/19 10:46, Steven Rostedt wrote:
> > > On Mon, 6 May 2019 15:42:00 +0100
> > > Qais Yousef <[email protected]> wrote:
> > >
> > > > I can control that for the wrappers I'm introducing. But the actual tracepoint
> > > > get the 'trace_' part prepended automatically by the macros.
> > > >
> > > > ie DECLARE_TRACE(pelt_rq, ...) will automatically generate a function called
> > > > trace_pelt_se(...)
> > > >
> > > > Or am I missing something?
> > >
> > > No trace comes from the trace points.
>
> Re-reading that line, I see I totally didn't express what I meant :-p
>
> >
> > If you want I can do something like below to help create a distinction. It is
> > none enforcing though.
> >
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 9c3186578ce0..f654ced20045 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -232,6 +232,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > */
> > #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
> > extern struct tracepoint __tracepoint_##name; \
> > + static inline void tp_##name(proto) __alias(trace_##name); \
> > static inline void trace_##name(proto) \
> > { \
> > if (static_key_false(&__tracepoint_##name.key)) \
> >
> >
> > Another option is to extend DECLARE_TRACE() to take a new argument IS_TP and
> > based on that select the function name. This will be enforcing but I will have
> > to go fixup many places.
> >
> > Of course 'TP' can be replaced with anything more appealing.
>
> No no no, I meant to say...
>
> "No that's OK. The "trace_" *is* from the trace points, and trace
> events build on top of them."
I did have to stare at the original statement for a bit :-)
This makes more sense now. Thanks for the clarification.
--
Qais Yousef
On 05/06/19 11:18, Peter Zijlstra wrote:
> On Mon, May 06, 2019 at 11:08:59AM +0200, Peter Zijlstra wrote:
> > Also; I _really_ hate how fat they are. Why can't we do simple straight
> > 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.
>
> ARGH, because we don't export any of those data structures (for good
> reason).. bah I hate all this.
I am not a big fan either..
FWIW struct sched_entity and struct sched_avg are exported but only used in
kernel/sched/*. Are the reasons behind not exporting struct cfs_rq and struct
rq are really different to the other 2?
Anyways. I have v2 almost ready but thought I'd ask before posting if we want
to handle this in a different way.
Thanks
--
Qais Yousef
Hi Qais,
On 5/5/19 1:57 PM, Qais Yousef wrote:
[...]
> 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 @@
> +/* 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);
This will let a !CONFIG_SMP build fail.
> + }
> +}
> +
> +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);
Same here.
> + }
> +}
> +
> +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);
and here.
On 05/10/19 10:51, Dietmar Eggemann wrote:
> Hi Qais,
>
> On 5/5/19 1:57 PM, Qais Yousef wrote:
>
> [...]
>
> > 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 @@
> > +/* 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);
>
> This will let a !CONFIG_SMP build fail.
You're right. sched_avg is only defined if CONFIG_SMP. Fixed all three
functions.
Thanks!
--
Qais Yousef