Re: [PATCH] Sched: Add a tracepoint to track rq->nr_running

From: Steven Rostedt <rostedt@goodmis.org>
To: Phil Auld <pauld@redhat.com>
Cc: linux-kernel@vger.kernel.org, Qais Yousef <qais.yousef@arm.com>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>
Subject: Re: [PATCH] Sched: Add a tracepoint to track rq->nr_running
Date: Fri, 19 Jun 2020 12:46:41 -0400	[thread overview]
Message-ID: <20200619124641.7f94ad14@oasis.local.home> (raw)
In-Reply-To: <20200619141120.1476-1-pauld@redhat.com>

On Fri, 19 Jun 2020 10:11:20 -0400
Phil Auld <pauld@redhat.com> 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.

-- Steve