RE: [PATCH 1/4] drivers/clocksource/hyper-v: Introduce a pointer to TSC page

From: "Michael Kelley (LINUX)" <mikelley@microsoft.com>
To: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>
Cc: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/4] drivers/clocksource/hyper-v: Introduce a pointer to TSC page
Date: Wed, 2 Nov 2022 20:44:12 +0000	[thread overview]
Message-ID: <BYAPR21MB16887D9653B419A14CC29511D7399@BYAPR21MB1688.namprd21.prod.outlook.com> (raw)
In-Reply-To: <CA+DrgLxD8X3cjFNAXYjxr-1opJG_uzU-Ajvz_poMccaiANtQ3g@mail.gmail.com>

From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com> Sent: Wednesday, November 2, 2022 12:19 PM

> ср, 2 нояб. 2022 г. в 11:56, Michael Kelley (LINUX) <mailto:mikelley@microsoft.com>:
> From: Stanislav Kinsburskii <mailto:skinsburskii@linux.microsoft.com> Sent: Tuesday, November 1, 2022 10:31 AM
> > > 
> > > Will be used later keep the address of the remapped page for the root
> > > partition.
> >
> > s/later keep/later to keep/
> 
> I'll fix it, thanks.
>  
> > I'd like to see a more robust commit message, and not a partial
> > sentence that is a continuation of the commit title.  Something along the lines of:
> > 
> > When Linux is running in the root partition of the Microsoft Hypervisor,
> > the memory for the TSC page is provided by the hypervisor, so the TSC
> > page address can't be the address of a Linux global variable.
> >
> > Introduce a global variable to contain the TSC page address.  For a guest VM,
> > it defaults to the address of the Linux global variable.  If running in the root
> > partition, a later patch overrides to be the address of the page provided by
> > the hypervisor.
> 
> This is a cleanup patch whose goal is to provide a clear separation between the
> actual feature (which comes in the last patch of the series) and other precursor
> changes, making the feature introduction more laconic and clean.
> 
> I doubt it needs any additional text to expose the details of the resulting goal.
> Why do you think otherwise?

To me, the additional text clearly answers the "why" question for the patch.  Here's
the quote from Documentation/process/submitting-patches.rst:

   The explanation body will be committed to the permanent source
   changelog, so should make sense to a competent reader who has long since
   forgotten the immediate details of the discussion that might have led to
   this patch. Including symptoms of the failure which the patch addresses
   (kernel log messages, oops messages, etc.) are especially useful for
   people who might be searching the commit logs looking for the applicable
   patch. The text should be written in such detail so that when read
   weeks, months or even years later, it can give the reader the needed
   details to grasp the reasoning for **why** the patch was created.

Certainly, it's somewhat a matter of personal style, and I tend to lean toward the
"more explanation is better" approach.  But if no one else cares to weigh in on
the topic, it's not a blocker for me.

Michael