RE: [PATCH] Hyper-V: fix for unwanted manipulation of sched_clock when TSC marked unstable

On Tue, 13 Jul 2021, Michael Kelley wrote:

From: Ani Sinha <redacted> Sent: Tuesday, July 13, 2021 10:49 AM

quoted

On Tue, 13 Jul 2021, Michael Kelley wrote:

quoted

From: Ani Sinha <redacted> Sent: Monday, July 12, 2021 8:05 PM

quoted

Marking TSC as unstable has a side effect of marking sched_clock as
unstable when TSC is still being used as the sched_clock. This is not
desirable. Hyper-V ultimately uses a paravirtualized clock source that
provides a stable scheduler clock even on systems without TscInvariant
CPU capability. Hence, mark_tsc_unstable() call should be called _after_
scheduler clock has been changed to the paravirtualized clocksource. This
will prevent any unwanted manipulation of the sched_clock. Only TSC will
be correctly marked as unstable.

Signed-off-by: Ani Sinha <redacted>
---
 arch/x86/kernel/cpu/mshyperv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 22f13343b5da..715458b7729a 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c

@@ -370,8 +370,6 @@ static void __init ms_hyperv_init_platform(void)
 	if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
 		wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL, 0x1);
 		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
-	} else {
-		mark_tsc_unstable("running on Hyper-V");
 	}

 	/*

@@ -432,6 +430,12 @@ static void __init ms_hyperv_init_platform(void)
 	/* Register Hyper-V specific clocksource */
 	hv_init_clocksource();
 #endif
+	/* TSC should be marked as unstable only after Hyper-V
+	 * clocksource has been initialized. This ensures that the
+	 * stability of the sched_clock is not altered.
+	 */

For multi-line comments like the above, the first comment line
should just be "/*".  So:

Hmm, checkpatch.pl in kernel tree did not complain :

total: 0 errors, 0 warnings, 20 lines checked

0001-Hyper-V-fix-for-unwanted-manipulation-of-sched_clock.patch has no
obvious style problems and is ready for submission.

However, I do know from my experience of submitting Qemu patches last
year that this is a requirement imposed by the Qemu community as
checkpatch.pl in qemu tree would complain otherwise. I also took a peek at
the Qemu git history. It seems they imported this check from the kernel's
checkpatch.pl with this commit in Qemu tree:

commit 8c06fbdf36bf4d4d486116200248730887a4d7d6
Author: Peter Maydell [off-list ref]
Date:   Fri Dec 14 13:30:48 2018 +0000

    scripts/checkpatch.pl: Enforce multiline comment syntax

Which adds this rule:

+               # Block comments use /* on a line of its own
+               if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&      #inline /*...*/
+                   $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank
+                       WARN("Block comments use a leading /* on a separate line\n" . $herecurr);
+               }


But in kernel there is no such rule. Hmm. strange!

See section 8 of "Documentation/process/coding-style.rst" in a Linux kernel
source code tree. :-)

Yes that is fine. What I was confused about is that the commenting rule
existed in Qemu and checkpatch.pl there enforced this. Upon digging, I
found that the Qemu community themselves "imported" this formating rule
from kernel. Yet, kernel's own checkpatch.pl did not enforce this although
as you point above, the rule exists in written form.

This diff will fix kernel's checkpatch.pl without breaking drivers/net or
net/ . Enforcing rules through proper tooling saves everyone's time and
also ensures consistency.

Will send out this patch soon.

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 23697a6b1eaa..5f047b762aa1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -3833,6 +3833,14 @@ sub process {
 			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
 		}

+# Block comments use /* on a line of its own
+		if (!($realfile =~ m@^(drivers/net/|net/)@) &&
+		    $rawline !~ m@^\+.*/\*.*\*/[ \t)}]*$@ &&	#inline /*...*/
+		    $rawline =~ m@^\+.*/\*\*?+[ \t]*[^ \t]@) { # /* or /** non-blank
+		    WARN("BLOCK_COMMENT_STYLE",
+			 "Block comments use a leading /* on a separate line\n" . $herecurr);
+		}
+
 # Block comments use * on subsequent lines
 		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
 		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*