From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756921AbcBWCiQ (ORCPT ); Mon, 22 Feb 2016 21:38:16 -0500 Received: from mga03.intel.com ([134.134.136.65]:47286 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756769AbcBWCiO (ORCPT ); Mon, 22 Feb 2016 21:38:14 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,487,1449561600"; d="scan'208";a="909033271" Content-Type: text/plain; charset=utf-8; format=flowed; delsp=yes To: tglx@linutronix.de, richardcochran@gmail.com, mingo@redhat.com, john.stultz@linaro.org, hpa@zytor.com, jeffrey.t.kirsher@intel.com, "Andy Lutomirski" , "Peter Zijlstra" Cc: x86@kernel.org, linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, kevin.b.stanton@intel.com, kevin.j.clarke@intel.com Subject: Re: [PATCH v7 6/8] x86: tsc: Always Running Timer (ART) correlated clocksource References: <1455308729-6280-1-git-send-email-christopher.s.hall@intel.com> <1455308729-6280-7-git-send-email-christopher.s.hall@intel.com> <56C63385.9050703@kernel.org> Date: Mon, 22 Feb 2016 18:38:11 -0800 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: "Christopher Hall" Organization: Intel Corporation Message-ID: In-Reply-To: <56C63385.9050703@kernel.org> User-Agent: Opera Mail/1.0 (Win32) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 18 Feb 2016 13:11:33 -0800, Andy Lutomirski wrote: >> +#define X86_FEATURE_INVARIANT_TSC (7*32+4) /* Intel Invariant TSC */ This is removed. It was basically an alias for NONSTOP_TSC and not needed. > >> +/* >> + * Convert ART to TSC given numerator/denominator found in detect_art() >> + */ >> +struct system_counterval_t convert_art_to_tsc(cycle_t art) >> +{ >> + u64 tmp, res, rem; >> + >> + rem = do_div(art, art_to_tsc_denominator); >> + >> + res = art * art_to_tsc_numerator; >> + tmp = rem * art_to_tsc_numerator; >> + >> + do_div(tmp, art_to_tsc_denominator); >> + res += tmp; >> + >> + return (struct system_counterval_t) {.cs = art_related_clocksource, >> + .cycles = res}; > > The SDM and the patch description both mention an offset "k". Shouldn't > this code at least have a comment about how it deals with the k != 0 > case? I don't deal with the k != 0 case. I assume that IA32 TSC adjust MSR is 0 because it's almost always a *bad idea* to change it. I've discussed this with a few other developers and there is some consensus agreeing. From an earlier related thread Peter Zijlstra asserts that TSC adjust "had better" be 0.(http://lkml.iu.edu/hypermail/linux/kernel/1507.3/03734.html). Do we really need to accommodate BIOS's that do this? Chris From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Hall Date: Mon, 22 Feb 2016 18:38:11 -0800 Subject: [Intel-wired-lan] [PATCH v7 6/8] x86: tsc: Always Running Timer (ART) correlated clocksource In-Reply-To: <56C63385.9050703@kernel.org> References: <1455308729-6280-1-git-send-email-christopher.s.hall@intel.com> <1455308729-6280-7-git-send-email-christopher.s.hall@intel.com> <56C63385.9050703@kernel.org> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Thu, 18 Feb 2016 13:11:33 -0800, Andy Lutomirski wrote: >> +#define X86_FEATURE_INVARIANT_TSC (7*32+4) /* Intel Invariant TSC */ This is removed. It was basically an alias for NONSTOP_TSC and not needed. > >> +/* >> + * Convert ART to TSC given numerator/denominator found in detect_art() >> + */ >> +struct system_counterval_t convert_art_to_tsc(cycle_t art) >> +{ >> + u64 tmp, res, rem; >> + >> + rem = do_div(art, art_to_tsc_denominator); >> + >> + res = art * art_to_tsc_numerator; >> + tmp = rem * art_to_tsc_numerator; >> + >> + do_div(tmp, art_to_tsc_denominator); >> + res += tmp; >> + >> + return (struct system_counterval_t) {.cs = art_related_clocksource, >> + .cycles = res}; > > The SDM and the patch description both mention an offset "k". Shouldn't > this code at least have a comment about how it deals with the k != 0 > case? I don't deal with the k != 0 case. I assume that IA32 TSC adjust MSR is 0 because it's almost always a *bad idea* to change it. I've discussed this with a few other developers and there is some consensus agreeing. From an earlier related thread Peter Zijlstra asserts that TSC adjust "had better" be 0.(http://lkml.iu.edu/hypermail/linux/kernel/1507.3/03734.html). Do we really need to accommodate BIOS's that do this? Chris