[PATCH 0/2] Drivers: hv: vmbus: Wait for offers and log missing offers
After VM requests for channel offers during boot or resume from hibernation, host offers the available channels and then sends a separate message when all offers are delivered. Wait for this message to make this offers request and receipt process synchronous. This is to support user mode (VTL0) in OpenHCL (A Linux based paravisor for Confidential VMs) to ensure that all channel offers are present when init begins in VTL0, and it knows which channels the host has offered and which it has not. This is in analogy to a PCI bus not returning from probe until it has scanned all devices on the bus. As part of this implementation, some code cleanup is also done for the logic which becomes redundant due to this change. Second patch prints the channels which are not offered when resume happens from hibernation to supply more information to the end user. John Starks (1): Drivers: hv: vmbus: Log on missing offers Naman Jain (1): Drivers: hv: vmbus: Wait for offers during boot drivers/hv/channel_mgmt.c | 38 +++--- drivers/hv/connection.c | 4 ++-- drivers/hv/hyperv_vmbus.h | 14 +++--- drivers/hv/vmbus_drv.c| 30 +++--- 4 files changed, 43 insertions(+), 43 deletions(-) -- 2.34.1
Re: [RFC PATCH] drivers: hv: Convert open-coded timeouts to msecs_to_jiffies()
On 10/17/2024 4:07 AM, 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
---
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
<.>
if (wait_for_completion_timeout(
- &vmbus_connection.ready_for_resume_event, 10 * HZ) == 0)
+ &vmbus_connection.ready_for_resume_event, msecs_to_jiffies(10 *
1000)) == 0)
pr_err("Some vmbus device is missing after suspending?\n");
/* Reset the event for the next suspend. */
Looks good to me. There can be different ways of passing arg to
msecs_to_jiffies though-
for 10 seconds
* 1
* 10 * 1000
* 10 * MSEC_PER_SEC
I don't have any strong opinion on this, and you can probably choose
whichever feels better.
Even the current implementation with x * HZ works fine, with different
HZ values. But, yes, I agree that using msecs_to_jiffies is better.
Regards,
Naman
RE: [PATCH] x86/mtrr: Rename mtrr_overwrite_state() to guest_force_mtrr_state()
From: Kirill A. Shutemov Sent: Wednesday,
October 16, 2024 3:51 AM
>
> Rename the helper to better reflect its function.
>
> Signed-off-by: Kirill A. Shutemov
> Suggested-by: Dave Hansen
> ---
> arch/x86/hyperv/ivm.c | 2 +-
> arch/x86/include/asm/mtrr.h| 10 +-
> arch/x86/kernel/cpu/mtrr/generic.c | 6 +++---
> arch/x86/kernel/cpu/mtrr/mtrr.c| 2 +-
> arch/x86/kernel/kvm.c | 2 +-
> arch/x86/xen/enlighten_pv.c| 4 ++--
> 6 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 60fc3ed72830..90aabe1fd3b6 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -664,7 +664,7 @@ void __init hv_vtom_init(void)
> x86_platform.guest.enc_status_change_finish =
> hv_vtom_set_host_visibility;
>
> /* Set WB as the default cache mode. */
> - mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK);
> + guest_force_mtrr_state(NULL, 0, MTRR_TYPE_WRBACK);
> }
>
> #endif /* defined(CONFIG_AMD_MEM_ENCRYPT) ||
> defined(CONFIG_INTEL_TDX_GUEST) */
> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
> index 4218248083d9..c69e269937c5 100644
> --- a/arch/x86/include/asm/mtrr.h
> +++ b/arch/x86/include/asm/mtrr.h
> @@ -58,8 +58,8 @@ struct mtrr_state_type {
> */
> # ifdef CONFIG_MTRR
> void mtrr_bp_init(void);
> -void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
> - mtrr_type def_type);
> +void guest_force_mtrr_state(struct mtrr_var_range *var, unsigned int num_var,
> + mtrr_type def_type);
> extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
> extern void mtrr_save_fixed_ranges(void *);
> extern void mtrr_save_state(void);
> @@ -75,9 +75,9 @@ void mtrr_disable(void);
> void mtrr_enable(void);
> void mtrr_generic_set_state(void);
> # else
> -static inline void mtrr_overwrite_state(struct mtrr_var_range *var,
> - unsigned int num_var,
> - mtrr_type def_type)
> +static inline void guest_force_mtrr_state(struct mtrr_var_range *var,
> + unsigned int num_var,
> + mtrr_type def_type)
> {
> }
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
> b/arch/x86/kernel/cpu/mtrr/generic.c
> index 7b29ebda024f..2fdfda2b60e4 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -423,7 +423,7 @@ void __init mtrr_copy_map(void)
> }
>
> /**
> - * mtrr_overwrite_state - set static MTRR state
> + * guest_force_mtrr_state - set static MTRR state for a guest
> *
> * Used to set MTRR state via different means (e.g. with data obtained from
> * a hypervisor).
> @@ -436,8 +436,8 @@ void __init mtrr_copy_map(void)
> * @num_var: length of the @var array
> * @def_type: default caching type
> */
> -void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
> - mtrr_type def_type)
> +void guest_force_mtrr_state(struct mtrr_var_range *var, unsigned int num_var,
> + mtrr_type def_type)
> {
> unsigned int i;
>
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 989d368be04f..ecbda0341a8a 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -625,7 +625,7 @@ void mtrr_save_state(void)
> static int __init mtrr_init_finalize(void)
> {
> /*
> - * Map might exist if mtrr_overwrite_state() has been called or if
> + * Map might exist if guest_force_mtrr_state() has been called or if
>* mtrr_enabled() returns true.
>*/
> mtrr_copy_map();
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 21e9e4845354..7a422a6c5983 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -983,7 +983,7 @@ static void __init kvm_init_platform(void)
> x86_platform.apic_post_init = kvm_apic_init;
>
> /* Set WB as the default cache mode for SEV-SNP and TDX */
> - mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK);
> + guest_force_mtrr_state(NULL, 0, MTRR_TYPE_WRBACK);
> }
>
> #if defined(CONFIG_AMD_MEM_ENCRYPT)
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index d6818c6cafda..633469fab536 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -171,7 +171,7 @@ static void __init xen_set_mtrr_data(void)
>
> /* Only overwrite MTRR state if any MTRR could be got from Xen. */
> if (reg)
> - mtrr_overwrite_state(var, reg, MTRR_TYPE_UNCACHABLE);
> + guest_force_mtrr_state(var, reg, MTRR_TYPE_UNCACHABLE);
> #endif
> }
>
> @@ -195,7 +195,7 @@ static void __init xen_pv_init_platform(void)
> if (xen_initial_domain())
> xen_set_mtrr_data();
> else
> - mtrr
Re: [RFC PATCH] drivers: hv: Convert open-coded timeouts to msecs_to_jiffies()
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 >> --- >> 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. Thanks, Easwar
Re: [PATCH 1/2] Drivers: hv: vmbus: Wait for offers during boot
On 10/18/2024 4:58 AM, Naman Jain wrote: > Channels offers are requested during vmbus initialization and resume > from hibernation. Add support to wait for all channel offers to be > delivered and processed before returning from vmbus_request_offers. > This is to support user mode (VTL0) in OpenHCL (A Linux based > paravisor for Confidential VMs) to ensure that all channel offers > are present when init begins in VTL0, and it knows which channels > the host has offered and which it has not. > > This is in analogy to a PCI bus not returning from probe until it has > scanned all devices on the bus. > > Without this, user mode can race with vmbus initialization and miss > channel offers. User mode has no way to work around this other than > sleeping for a while, since there is no way to know when vmbus has > finished processing offers. > > With this added functionality, remove earlier logic which keeps track > of count of offered channels post resume from hibernation. Once all > offers delivered message is received, no further offers are going to > be received. Consequently, logic to prevent suspend from happening > after previous resume had missing offers, is also removed. > > Co-developed-by: John Starks > Signed-off-by: John Starks > Signed-off-by: Naman Jain > --- > drivers/hv/channel_mgmt.c | 38 +++--- > drivers/hv/connection.c | 4 ++-- > drivers/hv/hyperv_vmbus.h | 14 +++--- > drivers/hv/vmbus_drv.c| 16 > 4 files changed, 28 insertions(+), 44 deletions(-) Looks good to me. Reviewed-by: Easwar Hariharan
[PATCH 1/2] Drivers: hv: vmbus: Wait for offers during boot
Channels offers are requested during vmbus initialization and resume
from hibernation. Add support to wait for all channel offers to be
delivered and processed before returning from vmbus_request_offers.
This is to support user mode (VTL0) in OpenHCL (A Linux based
paravisor for Confidential VMs) to ensure that all channel offers
are present when init begins in VTL0, and it knows which channels
the host has offered and which it has not.
This is in analogy to a PCI bus not returning from probe until it has
scanned all devices on the bus.
Without this, user mode can race with vmbus initialization and miss
channel offers. User mode has no way to work around this other than
sleeping for a while, since there is no way to know when vmbus has
finished processing offers.
With this added functionality, remove earlier logic which keeps track
of count of offered channels post resume from hibernation. Once all
offers delivered message is received, no further offers are going to
be received. Consequently, logic to prevent suspend from happening
after previous resume had missing offers, is also removed.
Co-developed-by: John Starks
Signed-off-by: John Starks
Signed-off-by: Naman Jain
---
drivers/hv/channel_mgmt.c | 38 +++---
drivers/hv/connection.c | 4 ++--
drivers/hv/hyperv_vmbus.h | 14 +++---
drivers/hv/vmbus_drv.c| 16
4 files changed, 28 insertions(+), 44 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 3c6011a48dab..ac514805dafe 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -944,16 +944,6 @@ void vmbus_initiate_unload(bool crash)
vmbus_wait_for_unload();
}
-static void check_ready_for_resume_event(void)
-{
- /*
-* If all the old primary channels have been fixed up, then it's safe
-* to resume.
-*/
- if (atomic_dec_and_test(&vmbus_connection.nr_chan_fixup_on_resume))
- complete(&vmbus_connection.ready_for_resume_event);
-}
-
static void vmbus_setup_channel_state(struct vmbus_channel *channel,
struct vmbus_channel_offer_channel *offer)
{
@@ -1109,8 +1099,6 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
/* Add the channel back to the array of channels. */
vmbus_channel_map_relid(oldchannel);
- check_ready_for_resume_event();
-
mutex_unlock(&vmbus_connection.channel_mutex);
return;
}
@@ -1297,12 +1285,11 @@ EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
/*
* vmbus_onoffers_delivered -
* This is invoked when all offers have been delivered.
- *
- * Nothing to do here.
*/
static void vmbus_onoffers_delivered(
struct vmbus_channel_message_header *hdr)
{
+ complete(&vmbus_connection.all_offers_delivered_event);
}
/*
@@ -1578,7 +1565,8 @@ void vmbus_onmessage(struct vmbus_channel_message_header
*hdr)
}
/*
- * vmbus_request_offers - Send a request to get all our pending offers.
+ * vmbus_request_offers - Send a request to get all our pending offers
+ * and wait for all offers to arrive.
*/
int vmbus_request_offers(void)
{
@@ -1596,6 +1584,10 @@ int vmbus_request_offers(void)
msg->msgtype = CHANNELMSG_REQUESTOFFERS;
+ /*
+* This REQUESTOFFERS message will result in the host sending an all
+* offers delivered message.
+*/
ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_message_header),
true);
@@ -1607,6 +1599,22 @@ int vmbus_request_offers(void)
goto cleanup;
}
+ /* Wait for the host to send all offers. */
+ while (wait_for_completion_timeout(
+ &vmbus_connection.all_offers_delivered_event,
msecs_to_jiffies(10 * 1000)) == 0) {
+ pr_warn("timed out waiting for all offers to be
delivered...\n");
+ }
+
+ /*
+* Flush handling of offer messages (which may initiate work on
+* other work queues).
+*/
+ flush_workqueue(vmbus_connection.work_queue);
+
+ /* Flush processing the incoming offers. */
+ flush_workqueue(vmbus_connection.handle_primary_chan_wq);
+ flush_workqueue(vmbus_connection.handle_sub_chan_wq);
+
cleanup:
kfree(msginfo);
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index f001ae880e1d..8351360bba16 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -34,8 +34,8 @@ struct vmbus_connection vmbus_connection = {
.ready_for_suspend_event = COMPLETION_INITIALIZER(
vmbus_connection.ready_for_suspend_event),
- .ready_for_resume_event = COMPLETION_INITIALIZER(
- vmbus_connection.ready_for_resume_event),
+ .all_offers_delivered_event = COMPLETION_INITIALIZER(
+
[PATCH net,v3] hv_netvsc: Fix VF namespace also in synthetic NIC NETDEV_REGISTER event
The existing code moves VF to the same namespace as the synthetic NIC
during netvsc_register_vf(). But, if the synthetic device is moved to a
new namespace after the VF registration, the VF won't be moved together.
To make the behavior more consistent, add a namespace check for synthetic
NIC's NETDEV_REGISTER event (generated during its move), and move the VF
if it is not in the same namespace.
Cc: sta...@vger.kernel.org
Fixes: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device")
Suggested-by: Stephen Hemminger
Signed-off-by: Haiyang Zhang
Reviewed-by: Simon Horman
---
v3: Use RCT order as suggested by Simon.
v2: Move my fix to synthetic NIC's NETDEV_REGISTER event as suggested by
Stephen.
---
drivers/net/hyperv/netvsc_drv.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 153b97f8ec0d..23180f7b67b6 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2798,6 +2798,31 @@ static struct hv_driver netvsc_drv = {
},
};
+/* Set VF's namespace same as the synthetic NIC */
+static void netvsc_event_set_vf_ns(struct net_device *ndev)
+{
+ struct net_device_context *ndev_ctx = netdev_priv(ndev);
+ struct net_device *vf_netdev;
+ int ret;
+
+ vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
+ if (!vf_netdev)
+ return;
+
+ if (!net_eq(dev_net(ndev), dev_net(vf_netdev))) {
+ ret = dev_change_net_namespace(vf_netdev, dev_net(ndev),
+ "eth%d");
+ if (ret)
+ netdev_err(vf_netdev,
+ "Cannot move to same namespace as %s: %d\n",
+ ndev->name, ret);
+ else
+ netdev_info(vf_netdev,
+ "Moved VF to namespace with: %s\n",
+ ndev->name);
+ }
+}
+
/*
* On Hyper-V, every VF interface is matched with a corresponding
* synthetic interface. The synthetic interface is presented first
@@ -2810,6 +2835,11 @@ static int netvsc_netdev_event(struct notifier_block
*this,
struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
int ret = 0;
+ if (event_dev->netdev_ops == &device_ops && event == NETDEV_REGISTER) {
+ netvsc_event_set_vf_ns(event_dev);
+ return NOTIFY_DONE;
+ }
+
ret = check_dev_is_matching_vf(event_dev);
if (ret != 0)
return NOTIFY_DONE;
--
2.34.1
[PATCH 2/2] Drivers: hv: vmbus: Log on missing offers
From: John Starks
When resuming from hibernation, log any channels that were present
before hibernation but now are gone.
Signed-off-by: John Starks
Co-developed-by: Naman Jain
Signed-off-by: Naman Jain
---
drivers/hv/vmbus_drv.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index bd3fc41dc06b..1f56d138210e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2462,6 +2462,7 @@ static int vmbus_bus_suspend(struct device *dev)
static int vmbus_bus_resume(struct device *dev)
{
+ struct vmbus_channel *channel;
struct vmbus_channel_msginfo *msginfo;
size_t msgsize;
int ret;
@@ -2494,6 +2495,21 @@ static int vmbus_bus_resume(struct device *dev)
vmbus_request_offers();
+ mutex_lock(&vmbus_connection.channel_mutex);
+ list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
+ if (channel->offermsg.child_relid != INVALID_RELID)
+ continue;
+
+ /* hvsock channels are not expected to be present. */
+ if (is_hvsock_channel(channel))
+ continue;
+
+ pr_err("channel %pUl/%pUl not present after resume.\n",
+ &channel->offermsg.offer.if_type,
+ &channel->offermsg.offer.if_instance);
+ }
+ mutex_unlock(&vmbus_connection.channel_mutex);
+
/* Reset the event for the next suspend. */
reinit_completion(&vmbus_connection.ready_for_suspend_event);
--
2.34.1
RE: [RFC PATCH] drivers: hv: Convert open-coded timeouts to msecs_to_jiffies()
From: Easwar Hariharan 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 > >> --- > >> 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
Re: [RFC PATCH] drivers: hv: Convert open-coded timeouts to msecs_to_jiffies()
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 > --- > 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 ? Regards, ~Praveen.
