linux-kernel - RE: [RFC PATCH] drivers: hv: Convert open-coded timeouts to msecs_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SN6PR02MB41573004E0B25DA75F38F0AED4412@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sat, 19 Oct 2024 04:59:25 +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: 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.

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.  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".

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