From: Easwar Hariharan <eahariha@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> > >> --- > >> 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
