Message-ID:
 <SN6PR02MB41573CB05D8E32C376EB8546D4432@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 21 Oct 2024 04:17:49 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Easwar Hariharan <eahariha@...ux.microsoft.com>, Praveen Kumar
	<kumarpraveen@...ux.microsoft.com>, "lkp@...el.com" <lkp@...el.com>, "K. Y.
 Srinivasan" <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei
 Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, "open
 list:Hyper-V/Azure CORE AND DRIVERS" <linux-hyperv@...r.kernel.org>, open
 list <linux-kernel@...r.kernel.org>
CC: Naman Jain <namjain@...ux.microsoft.com>, Shradha Gupta
	<shradhagupta@...ux.microsoft.com>
Subject: RE: [RFC PATCH] drivers: hv: Convert open-coded timeouts to
 msecs_to_jiffies()

From: Easwar Hariharan <eahariha@...ux.microsoft.com> Sent: Sunday, October 20, 2024 8:42 PM
> 
> On 10/18/2024 9:59 PM, Michael Kelley wrote:
> > From: Easwar Hariharan <eahariha@...ux.microsoft.com> Sent: Friday, October 18, 2024 3:50 PM
> >>
> >> On 10/18/2024 12:54 AM, Praveen Kumar wrote:
> >>> On 17-10-2024 04:07, Easwar Hariharan wrote:
> >>>> We have several places where timeouts are open-coded as N (seconds) * HZ,
> >>>> but best practice is to use msecs_to_jiffies(). Convert the timeouts to
> >>>> make them HZ invariant.
> >>>>> Signed-off-by: Easwar Hariharan <eahariha@...ux.microsoft.com>
> >>>> ---
> >>>>  drivers/hv/hv_balloon.c  | 9 +++++----
> >>>>  drivers/hv/hv_kvp.c      | 4 ++--
> >>>>  drivers/hv/hv_snapshot.c | 6 ++++--
> >>>>  drivers/hv/vmbus_drv.c   | 2 +-
> >>>>  4 files changed, 12 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> >>>> index c38dcdfcb914d..3017d41f12681 100644
> >>>> --- a/drivers/hv/hv_balloon.c
> >>>> +++ b/drivers/hv/hv_balloon.c
> >>>> @@ -756,7 +756,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
> >>>>  		 * adding succeeded, it is ok to proceed even if the memory was
> >>>>  		 * not onlined in time.
> >>>>  		 */
> >>>> -		wait_for_completion_timeout(&dm_device.ol_waitevent, 5 * HZ);
> >>>> +		wait_for_completion_timeout(&dm_device.ol_waitevent, msecs_to_jiffies(5 * 1000));
> >>>
> >>> Is it correct to convert HZ to 1000 ?
> >>> Also, how are you testing these changes ?
> >>>
> >>
> >> It's a conversion of milliseconds to seconds, rather than HZ to 1000. :)
> >> msecs_to_jiffies() handles the conversion to jiffies with HZ. As Naman
> >> mentioned, this could be equivalently written as 5 * MSECS_PER_SEC, and
> >> would probably be more readable. On testing, this is only
> >> compile-tested, and that's part of the reason why it's an RFC, since I'm
> >> not 100% sure every one of these timeouts is measured in seconds. Hoping
> >> for folks more familiar with the code to take a look.
> >>
> >
> > I believe the current code is correct.  Two things:
> >
> > 1) The values multiplied by HZ are indeed in seconds. The number of
> > seconds are somewhat arbitrary in some of the cases, so you might
> > argue for a different number of seconds. But as coded, the values
> > are in seconds.
> 
> Thanks for reviewing, Michael, and for the confirmation.
> 
> >
> > 2) Unless I'm missing something, the current code uses the correct
> > timeout regardless of the value of HZ because the number of jiffies
> > per second *is* HZ.
> >
> > As such, it might help to be explicit in the commit message that this
> > patch isn't fixing any bugs.
> 
> Will do.
> 
> > As the commit message says, the patch is
> > to bring the code into conformance with best practices. I presume
> > the best practice you reference is described in
> > Documentation/scheduler/completion.rst.
> >
> > I don't understand the statement about making the code "HZ invariant",
> > which I think came from the aforementioned documentation.  Per
> > my #2 above, I think the existing code is already "HZ invariant", at
> > least for how I would interpret "HZ invariant".
> >
> 
> That's right, both the best practice and the statement of HZ-invariance
> came from the scheduler documentation you pointed out. While I can't
> find the source with a quick search right now, I understand that HZ
> varies with CPU frequency and I figure that's what the statement is
> referring to. Unfortunately, there wasn't any discussion on
> HZ-invariance I can find on the lore thread where the statement was
> added. [1] It seems to be one of those "it's so self explanatory it
> doesn't warrant a mention" unless you're one of today's 10,000. [2]

No, HZ is not related to the CPU frequency. HZ is a compile-time fixed
value specified in the .config file.  grep for HZ in the .config file. The
allowed values are 100, 250, 300, and 1000. The jiffies code is set up
so the number of jiffies per sec is HZ. So specifying "5 * HZ" (for
example) as a jiffies value means 5 seconds, regardless of which 
value of HZ the kernel was compiled with. In my interpretation, that
means using "5 * HZ" as a jiffies value is "HZ invariant".

HZ (or some predecessor) originated back in the day when physical
hardware did not offer high precision timers like it does today. Timer
hardware generated "ticks", and ticks were normalized across a wide
range of hardware to occur at frequency HZ. Usually that meant a
timer interrupted HZ times per second. I don't know the full history here,
but jiffies were the coarse measure of the passage of time in the kernel,
so mapping jiffies to HZ made sense. Older internal kernel APIs use
jiffies, mostly for historical reasons even though much higher resolution
timers are usually available. And there may be additional nuances here
that I'm not aware of.

> 
> > Regardless of the meaning of "HZ invariant", I agree with the idea of
> > eliminating the use of HZ in cases like this, and letting msecs_to_jiffies()
> > handle it. Unfortunately, converting from "5 * HZ" to
> > "msecs_to_jiffies(5 * 1000)" makes the code really clunky. I would
> > advocate for adding something like this to include/linux/jiffies.h:
> >
> > #define secs_to_jiffies(secs)    msecs_to_jiffies((secs) * 1000)
> >
> > and then using secs_to_jiffies() for all the cases in this patch. That
> > reduces the clunkiness. But maybe somebody in the past tried to
> > add secs_to_jiffies() and got shot down -- I don't know. It seems like
> > an obvious thing to add ....
> >
> > Michael
> 
> From a quick search on lore with dfb:secs_to_jiffies, it doesn't look
> like anyone has tried to add secs_to_jiffies() to jiffies.h. There is
> one instance of secs_to_jiffies() being defined in
> net/bluetooth/hci_event.c, added in 2021.

That's interesting!  Somebody else had the same idea. Move that
to the jiffies.h file, and then use it in your patch.

Michael

Powered by blists - more mailing lists