Re: [patch V6 01/37] tracing/hwlat: Use ktime_get_mono_fast_ns()

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 16 May 2020 01:45:48 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Timestamping in the hardware latency detector uses sched_clock() underneath
> and depends on CONFIG_GENERIC_SCHED_CLOCK=n because sched clocks from that
> subsystem are not NMI safe.
> 
> ktime_get_mono_fast_ns() is NMI safe and available on all architectures.
> 
> Replace the time getter, get rid of the CONFIG_GENERIC_SCHED_CLOCK=n
> dependency and cleanup the horrible macro maze which encapsulates u64 math
> in u64 macros.

Good riddance. That macro maze was due to supporting the same code upstream
as we had in RHEL RT, where the math and time keeping functions available
between the two kernels were different.

That's been dealt with, but the macros never got cleaned up.

> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/trace/trace_hwlat.c |   59 +++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
> 
> --- a/kernel/trace/trace_hwlat.c
> +++ b/kernel/trace/trace_hwlat.c
> @@ -131,29 +131,19 @@ static void trace_hwlat_sample(struct hw
>  		trace_buffer_unlock_commit_nostack(buffer, event);
>  }
>  
> -/* Macros to encapsulate the time capturing infrastructure */
> -#define time_type	u64
> -#define time_get()	trace_clock_local()
> -#define time_to_us(x)	div_u64(x, 1000)
> -#define time_sub(a, b)	((a) - (b))
> -#define init_time(a, b)	(a = b)
> -#define time_u64(a)	a
> -
> +/*
> + * Timestamping uses ktime_get_mono_fast(), the NMI safe access to
> + * CLOCK_MONOTONIC.
> + */
>  void trace_hwlat_callback(bool enter)
>  {
>  	if (smp_processor_id() != nmi_cpu)
>  		return;
>  
> -	/*
> -	 * Currently trace_clock_local() calls sched_clock() and the
> -	 * generic version is not NMI safe.
> -	 */
> -	if (!IS_ENABLED(CONFIG_GENERIC_SCHED_CLOCK)) {
> -		if (enter)
> -			nmi_ts_start = time_get();
> -		else
> -			nmi_total_ts += time_get() - nmi_ts_start;
> -	}
> +	if (enter)
> +		nmi_ts_start = ktime_get_mono_fast_ns();
> +	else
> +		nmi_total_ts += ktime_get_mono_fast_ns() - nmi_ts_start;
>  
>  	if (enter)
>  		nmi_count++;
> @@ -165,20 +155,22 @@ void trace_hwlat_callback(bool enter)
>   * Used to repeatedly capture the CPU TSC (or similar), looking for potential
>   * hardware-induced latency. Called with interrupts disabled and with
>   * hwlat_data.lock held.
> + *
> + * Use ktime_get_mono_fast() here as well because it does not wait on the
> + * timekeeping seqcount like ktime_get_mono().

When doing a "git grep ktime_get_mono" I only find
ktime_get_mono_fast_ns() (and this comment), so I don't know what to compare
that to. Did you mean another function?

The rest looks fine (although, I see other things I need to clean up in
this code ;-)

-- Steve


>   */
>  static int get_sample(void)
>  {
>  	struct trace_array *tr = hwlat_trace;
>  	struct hwlat_sample s;
> -	time_type start, t1, t2, last_t2;
> +	u64 start, t1, t2, last_t2, thresh;
>  	s64 diff, outer_diff, total, last_total = 0;
>  	u64 sample = 0;
> -	u64 thresh = tracing_thresh;
>  	u64 outer_sample = 0;
>  	int ret = -1;
>  	unsigned int count = 0;
>