Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466456 > unrolled thread

Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer Peter Zijlstra <peterz@infradead.org> - 2016-08-19 16:10 +0200
    Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer Steven Rostedt <rostedt@goodmis.org> - 2016-08-19 17:00 +0200
      Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer Peter Zijlstra <peterz@infradead.org> - 2016-08-19 17:10 +0200
        Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer John Stultz <john.stultz@linaro.org> - 2016-08-19 18:40 +0200
          Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer Thomas Gleixner <tglx@linutronix.de> - 2016-08-23 11:00 +0200
          Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer Peter Zijlstra <peterz@infradead.org> - 2016-08-23 11:50 +0200

#1466456 — Re: [PATCH v2 0/4] tracing: Add Hardware Latency detector tracer

On Wed, Aug 10, 2016 at 09:53:56AM -0400, Steven Rostedt wrote:
> Changes since v1:
> 
>  . Added checks for CONFIG_GENERIC_SCHED_CLOCK, and if that is set,
>    only nmi_counts will be recorded when an NMI is triggered, but not
>    the time in NMI, because the generic sched_clock is not NMI safe.
>    (suggested by Sebastian Andrzej Siewior)

So any of the platforms using GENERIC_SCHED_CLOCK _have_ NMIs ?

In any case, for those you could probably use ktime_get_mono_fast_ns().

[toc] | [next] | [standalone]

#1466477

On Fri, 19 Aug 2016 16:09:28 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Aug 10, 2016 at 09:53:56AM -0400, Steven Rostedt wrote:
> > Changes since v1:
> > 
> >  . Added checks for CONFIG_GENERIC_SCHED_CLOCK, and if that is set,
> >    only nmi_counts will be recorded when an NMI is triggered, but not
> >    the time in NMI, because the generic sched_clock is not NMI safe.
> >    (suggested by Sebastian Andrzej Siewior)  
> 
> So any of the platforms using GENERIC_SCHED_CLOCK _have_ NMIs ?

From what I gather, the answer is no, so I don't think this is an issue.

> 
> In any case, for those you could probably use ktime_get_mono_fast_ns().

Is that safe to call from NMI? Looking into the code I see:

	now = ktime_to_ns(tkr->base) + timekeeping_get_ns(tkr);

where timekeeping_get_ns() has:

	timekeeping_get_delta()

which does:

	read_seqcount_begin()

which has (eventually):

repeat:
	ret = READ_ONCE(s->sequence);
	if (unlikely(ret & 1)) {
		cpu_relax();
		goto repeat;
	}
	return ret;

Which I think could cause a deadlock from an NMI.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1466484

On Fri, Aug 19, 2016 at 10:26:20AM -0400, Steven Rostedt wrote:
> > In any case, for those you could probably use ktime_get_mono_fast_ns().
> 
> Is that safe to call from NMI?

It should be, we were very careful to make it so. Also read the comment
with __ktime_get_fast_ns().

> Looking into the code I see:
> 
> 	now = ktime_to_ns(tkr->base) + timekeeping_get_ns(tkr);
> 
> where timekeeping_get_ns() has:
> 
> 	timekeeping_get_delta()
> 
> which does:
> 
> 	read_seqcount_begin()
> 
> which has (eventually):

That's a bug in CONFIG_DEBUG_TIMEKEEPING, if you look at the
!timekeeping_get_delta() it does no such thing.

John, looks like 4ca22c2648f9 ("timekeeping: Add warnings when overflows
or underflows are observed") buggered things.

[toc] | [prev] | [next] | [standalone]

#1466550

On Fri, Aug 19, 2016 at 8:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Aug 19, 2016 at 10:26:20AM -0400, Steven Rostedt wrote:
>> > In any case, for those you could probably use ktime_get_mono_fast_ns().
>>
>> Is that safe to call from NMI?
>
> It should be, we were very careful to make it so. Also read the comment
> with __ktime_get_fast_ns().
>
>> Looking into the code I see:
>>
>>       now = ktime_to_ns(tkr->base) + timekeeping_get_ns(tkr);
>>
>> where timekeeping_get_ns() has:
>>
>>       timekeeping_get_delta()
>>
>> which does:
>>
>>       read_seqcount_begin()
>>
>> which has (eventually):
>
> That's a bug in CONFIG_DEBUG_TIMEKEEPING, if you look at the
> !timekeeping_get_delta() it does no such thing.
>
> John, looks like 4ca22c2648f9 ("timekeeping: Add warnings when overflows
> or underflows are observed") buggered things.

Oof. Apologies. I missed that the fast methods called that helper as well.


Any objection to open-coding it for the fast method, like (sorry for
the whitespace damage here):

@@ -401,7 +401,10 @@ static __always_inline u64
__ktime_get_fast_ns(struct tk_fast *tkf)
        do {
                seq = raw_read_seqcount_latch(&tkf->seq);
                tkr = tkf->base + (seq & 0x01);
-               now = ktime_to_ns(tkr->base) + timekeeping_get_ns(tkr);
+               now = ktime_to_ns(tkr->base);
+
+               now += clocksource_delta(tkr->read(tkr->clock),
+                                        tkr->cycle_last, tkr->mask);
        } while (read_seqcount_retry(&tkf->seq, seq));

        return now;

If not I'll get such a patch sorted, tested and tagged for -stable.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1468393

On Tue, 23 Aug 2016, Peter Zijlstra wrote:
> On Fri, Aug 19, 2016 at 09:36:41AM -0700, John Stultz wrote:
> > On Fri, Aug 19, 2016 at 8:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > John, looks like 4ca22c2648f9 ("timekeeping: Add warnings when overflows
> > > or underflows are observed") buggered things.
> > 
> > Oof. Apologies. I missed that the fast methods called that helper as well.
> > 
> > 
> > Any objection to open-coding it for the fast method, like (sorry for
> > the whitespace damage here):
> > 
> > @@ -401,7 +401,10 @@ static __always_inline u64
> > __ktime_get_fast_ns(struct tk_fast *tkf)
> >         do {
> >                 seq = raw_read_seqcount_latch(&tkf->seq);
> >                 tkr = tkf->base + (seq & 0x01);
> > -               now = ktime_to_ns(tkr->base) + timekeeping_get_ns(tkr);
> > +               now = ktime_to_ns(tkr->base);
> > +
> > +               now += clocksource_delta(tkr->read(tkr->clock),
> > +                                        tkr->cycle_last, tkr->mask);
> >         } while (read_seqcount_retry(&tkf->seq, seq));
> > 
> >         return now;
> > 
> > If not I'll get such a patch sorted, tested and tagged for -stable.
> 
> Works for me, Thomas?

No objections.
 

[toc] | [prev] | [next] | [standalone]

#1468444

On Fri, Aug 19, 2016 at 09:36:41AM -0700, John Stultz wrote:
> On Fri, Aug 19, 2016 at 8:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > John, looks like 4ca22c2648f9 ("timekeeping: Add warnings when overflows
> > or underflows are observed") buggered things.
> 
> Oof. Apologies. I missed that the fast methods called that helper as well.
> 
> 
> Any objection to open-coding it for the fast method, like (sorry for
> the whitespace damage here):
> 
> @@ -401,7 +401,10 @@ static __always_inline u64
> __ktime_get_fast_ns(struct tk_fast *tkf)
>         do {
>                 seq = raw_read_seqcount_latch(&tkf->seq);
>                 tkr = tkf->base + (seq & 0x01);
> -               now = ktime_to_ns(tkr->base) + timekeeping_get_ns(tkr);
> +               now = ktime_to_ns(tkr->base);
> +
> +               now += clocksource_delta(tkr->read(tkr->clock),
> +                                        tkr->cycle_last, tkr->mask);
>         } while (read_seqcount_retry(&tkf->seq, seq));
> 
>         return now;
> 
> If not I'll get such a patch sorted, tested and tagged for -stable.

Works for me, Thomas?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web