From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759176AbcAUKNN (ORCPT ); Thu, 21 Jan 2016 05:13:13 -0500 Received: from www.linutronix.de ([62.245.132.108]:36748 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759031AbcAUKNH (ORCPT ); Thu, 21 Jan 2016 05:13:07 -0500 Date: Thu, 21 Jan 2016 11:12:05 +0100 (CET) From: Thomas Gleixner To: Jeff Merkey cc: LKML , John Stultz Subject: Re: [BUG REPORT] ktime_get_ts64 causes Hard Lockup In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Jeff, On Thu, 21 Jan 2016, Jeff Merkey wrote: > static inline s64 timekeeping_get_ns(struct tk_read_base *tkr) > { > cycle_t delta; > s64 nsec; > > delta = timekeeping_get_delta(tkr); > > nsec = delta * tkr->mult + tkr->xtime_nsec; > nsec >>= tkr->shift; << wrap caused here > > /* If arch requires, add in get_arch_timeoffset() */ > return nsec + arch_gettimeoffset(); > } > > You only have 64 bits of register and the numbers being calculated > here are big. By way of example, I observed the following during > normal operations: > > delta (RAX) | tkr->mult (RDX) > > 0x157876 0x65ee27 > 0xf1855 0x65f158 > 0x16cf05 0x65f408 > 303bc3 0x65f154 > > When this bug occurs different story. > > delta (RAX) | tkr->mult (RDX) > > 0x243283994b8 0x65233 > > So it goes like this: > > nsec = delta * tkr->mult + tkr->xtime_nsec; > 0x243283994b8 * 0x65233 > imul rax,rdx = 0xE6A2Ce1f1ea690a8 > > nsec >>= tkr->shift; << wrap caused here > sar rax,cl = 0xFFFFFFE6BFB3B7C3 That SAR is siomply wrong here. It must be an SHR and it is at least when I'm looking at the assembly of my machine. > the sar instruction doesn't just shift, it backfills the signedness of > the value, so this instruction is not doing what the C code is asking > it to do. I am guessing that somewhere in this mass of macros, > something may have gotten declared wrong or incomplete (declared > signed ?). There is no macro involved. timekeeping_get_ns { nsec = (delta * tkr->mult + tkr->xtime_nsec) >> tkr->shift; } > The assembler output for this section that calls the macro to > calculate nsecs shows the sar instruction: > > delta = timekeeping_get_delta(tkr); > > nsec = delta * tkr->mult + tkr->xtime_nsec; > 29b: 48 0f af c2 imul %rdx,%rax > 29f: 48 03 05 00 00 00 00 add 0x0(%rip),%rax # 2a6 > > nsec >>= tkr->shift; > 2a6: 48 d3 f8 sar %cl,%rax And this is fundamentally wrong. Why is the compiler emitting SAR instead of SHR here? Here is the assembly output from my kernel: nsec = (delta * tkr->mult + tkr->xtime_nsec) >> tkr->shift; 27e: 48 0f af c5 imul %rbp,%rax 282: 48 01 d8 add %rbx,%rax 285: 48 d3 e8 shr %cl,%rax } while (read_seqcount_retry(&tk_core.seq, seq)); So the first thing which needs to be figured out is WHY this results in a SAR on your compiler. > There is another problem with the tkr->read returning an unchanging, > unclearable number when this bug occurs for the delta value. I > appears for whatever reason the clock has gone to sleep or gone away > and is no longer updating its counters. > > static inline cycle_t timekeeping_get_delta(struct tk_read_base *tkr) > { > cycle_t cycle_now, delta; > > /* read clocksource */ > cycle_now = tkr->read(tkr->clock); << returns the same value after > this bug happens > > /* calculate the delta since the last update_wall_time */ > delta = clocksource_delta(cycle_now, tkr->cycle_last, tkr->mask); << > cycle last is also the same value. > > return delta; > } If that value does not change, then the timekeeping update is not running. That might happen because the timer interrupt is not happening or whatever got wreckaged. > I would check how these structs are defined and the vars in them to > see if somewhere they are declared as signed values to the compiler, > because that's what it thinks it was given to compile. Sure. Here you go: nsec = (delta * tkr->mult + tkr->xtime_nsec) >> tkr->shift; delta, mult, xtime_nsec and shift are unsigned. The only signed value is nsec. Does that issue go away if you apply the patch below? Thanks, tglx 8<----------- diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 34b4cedfa80d..d405bcdf9d40 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -301,7 +301,7 @@ static inline u32 arch_gettimeoffset(void) { return 0; } static inline s64 timekeeping_get_ns(struct tk_read_base *tkr) { cycle_t delta; - s64 nsec; + u64 nsec; delta = timekeeping_get_delta(tkr);