Re: [RFC PATCH] drivers: hv: Convert open-coded timeouts to msecs_to_jiffies()

[Easwar Hariharan <eahariha@linux.microsoft.com>]

On 10/18/2024 9:59 PM, Michael Kelley wrote:
> From: Easwar Hariharan <eahariha@linux.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@linux.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]

[1]
https://lore.kernel.org/all/1539183392-239389-1-git-send-email-john.garry@huawei.com/T/#u
[2] https://xkcd.com/1053/

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

Thanks,
Easwar