Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754026AbcKPTku (ORCPT ); Wed, 16 Nov 2016 14:40:50 -0500 Received: from mail-it0-f45.google.com ([209.85.214.45]:36325 "EHLO mail-it0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752275AbcKPTks (ORCPT ); Wed, 16 Nov 2016 14:40:48 -0500 MIME-Version: 1.0 In-Reply-To: References: <1479315472-5245-1-git-send-email-cmetcalf@mellanox.com> From: John Stultz Date: Wed, 16 Nov 2016 11:40:46 -0800 Message-ID: Subject: Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits To: Chris Metcalf Cc: Thomas Gleixner , Salman Qazi , Paul Turner , Tony Lindgren , Steven Miao , lkml Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2194 Lines: 59 On Wed, Nov 16, 2016 at 11:30 AM, Chris Metcalf wrote: > On 11/16/2016 1:04 PM, John Stultz wrote: >> >> On Wed, Nov 16, 2016 at 8:57 AM, Chris Metcalf >> wrote: >>> >>> include/linux/clocksource.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h >>> index 08398182f56e..b2a022acf232 100644 >>> --- a/include/linux/clocksource.h >>> +++ b/include/linux/clocksource.h >>> @@ -175,7 +175,7 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 >>> shift_constant) >>> */ >>> static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 >>> shift) >>> { >>> - return ((u64) cycles * mult) >> shift; >>> + return mult_frac(cycles, mult, 1ULL << shift); >>> } >> >> >> So clocksource_cyc2ns() was never intended to be used with >> indefinitely large cycle values, and it looks like tile and blackfin >> are abusing the interface (the omap usage provide cycle deltas rather >> then just the current counter value). > > > Well, the interface does just say "convert clocksource cycles to > nanoseconds". :-) Right, and I can understand the confusion, but its not being used with a struct clocksource. Its just being used to convert get_cycles(). > If you think it's more important that it be a little faster, we should > adjust the > documentation to say it is only appropriate for delta-cycles, not absolute > cycles. > I've appended a commit that does this if you'd like to take it. That's fair. Thanks for sending that, II'll queue that in my tree here in a moment. >> I'd suggest instead to move tile/blackfin to using the generic >> sched_clock implementation that most of the architectures use, or >> special case the code in the arch specific sched_clock >> implementations(as x86 does) instead of modifying the common interface >> to better handle a use case its not intended for. > > > Yes, since tile has a full 64-bit cycle counter, the best thing is to just > directly > open-code the mult_frac() in tile's sched_clock(). I'll push that change. > Steven Miao, I assume you should do the same for blackfin. thanks -john