Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964997AbcJ0Vrm (ORCPT ); Thu, 27 Oct 2016 17:47:42 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:45112 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751584AbcJ0Vri (ORCPT ); Thu, 27 Oct 2016 17:47:38 -0400 Date: Thu, 27 Oct 2016 23:44:57 +0200 (CEST) From: Thomas Gleixner To: Alexey Makhalov cc: corbet@lwn.net, akataria@vmware.com, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, pv-drivers@vmware.com Subject: Re: [RESEND PATCH 1/3] x86/vmware: Use tsc_khz value for calibrate_cpu() In-Reply-To: <20161027194454.9729-1-amakhalov@vmware.com> Message-ID: References: <20161027194454.9729-1-amakhalov@vmware.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3407 Lines: 105 On Thu, 27 Oct 2016, Alexey Makhalov wrote: > [RESEND PATCH 1/3] x86/vmware: Use tsc_khz value for calibrate_cpu() Please don't do that. RESEND is a keyword, when the same patch (series) is sent again without any modification vs. the first patch (series). A possible reason to do so is when a patch (series) fell through the cracks and the author wants to bring it to attention again for the next merge window. If you send an updated patch (series) then please add Vn after PATCH, where n is the version number of the patch (series). While at it, please add a cover letter [PATCH Vn 0/n] the next time you send a patch series. git send-email supports that. > After aa297292d708, there are separate native calibrations for cpu_khz and What is aa297292d708? Please make that: commit aa297292d708 ("x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID") ..... > tsc_khz. The code sets x86_platform.calibrate_cpu to native_calibrate_cpu() > which looks in cpuid leaf 0x16 or msrs for the cpu frequency. Which code? And what has the leaf and the msrs to do with your patch? > Since we keep the tsc_khz constant (even after vmotion), the cpu_khz and > tsc_khz may start diverging. Now you talk about vmware related stuff, right? > tsc_init() now does > > cpu_khz = x86_platform.calibrate_cpu(); > tsc_khz = x86_platform.calibrate_tsc(); > if (tsc_khz == 0) > tsc_khz = cpu_khz; > else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz) > cpu_khz = tsc_khz; And here you copy from the referenced commit. How is that helpful? > > We want the cpu_khz and tsc_khz to be sync even if they diverge less then > 10%. We? We as kernel developers, users, guest running in vmware ???? > This patch resolves this issue by setting x86_platform.calibrate_cpu to "This patch" is just bad. We already know that this is a patch, otherwise you wouldn't have sent it as a patch. "this issue" - which issue? Your explanation above does not tell anything what the issue is. > vmware_get_tsc_khz(). Here is an example of a proper changelog for this (except for my potential misinterpretation of the crypto message above): Commit aa297292d708 ("x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID") seperated the calibration mechanisms for cpu_khz and tsc_khz. In a vmware guest this change can lead to divergence between the tsc and the cpu frequency, because .This causes . Due to the internal design of the vmware hypervisor it's required to keep tsc and cpu frequency in sync. Solve this by overriding the x86 platform cpu calibration callback with the vmware specific tsc calibration function. So this describes very clearly: 1) The change which causes the problem 2) The problem itself and the resulting malfunction 3) What needs to be achieved to solve the problem 4) A short explanation what the solution is Documentation/SubmittingPatches has further information about proper change logs. > @@ -83,6 +83,7 @@ static void __init vmware_platform_setup(void) > > vmware_tsc_khz = tsc_khz; > x86_platform.calibrate_tsc = vmware_get_tsc_khz; A comment explaining why you set the cpu calibration to vmware_get_tsc_khz might be helpful for casual readers and yourself when you look at that code 5 month from now. > + x86_platform.calibrate_cpu = vmware_get_tsc_khz; Thanks, tglx