Message-ID: <alpine.DEB.2.22.394.2107140841240.2156790@anisinha-lenovo>
Date:   Wed, 14 Jul 2021 08:46:36 +0530 (IST)
From:   Ani Sinha <ani@...sinha.ca>
To:     Michael Kelley <mikelley@...rosoft.com>
cc:     Ani Sinha <ani@...sinha.ca>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "anirban.sinha@...ia.com" <anirban.sinha@...ia.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "x86@...nel.org" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>
Subject: 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 <ani@...sinha.ca> Sent: Tuesday, July 13, 2021 10:49 AM
> >
> > On Tue, 13 Jul 2021, Michael Kelley wrote:
> >
> > > From: Ani Sinha <ani@...sinha.ca> Sent: Monday, July 12, 2021 8:05 PM
> > > >
> > > > 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 <ani@...sinha.ca>
> > > > ---
> > > >  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 <peter.maydell@...aro.org>
> > 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 /*

Powered by blists - more mailing lists