2019-09-06 09:25:31

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH v5 0/9] Enhance the hv_vmbus driver to support hibernation

This patchset (consisting of 9 patches) was part of the v4 patchset (consisting
of 12 patches):
https://lkml.org/lkml/2019/9/2/894

The other 3 patches in v4 are posted in another patchset, which will go
through the tip.git tree.

All the 9 patches here are now rebased to the hyperv tree's hyperv-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
, and all the 9 patches have Michael Kelley's Signed-off-by's.

Please review.

Thanks!
Dexuan

Dexuan Cui (9):
Drivers: hv: vmbus: Break out synic enable and disable operations
Drivers: hv: vmbus: Suspend/resume the synic for hibernation
Drivers: hv: vmbus: Add a helper function is_sub_channel()
Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for
hibernation
Drivers: hv: vmbus: Ignore the offers when resuming from hibernation
Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation
Drivers: hv: vmbus: Clean up hv_sock channels by force upon suspend
Drivers: hv: vmbus: Suspend after cleaning up hv_sock and sub channels
Drivers: hv: vmbus: Resume after fixing up old primary channels

drivers/hv/channel_mgmt.c | 161 +++++++++++++++++++++++++---
drivers/hv/connection.c | 8 +-
drivers/hv/hv.c | 66 +++++++-----
drivers/hv/hyperv_vmbus.h | 30 ++++++
drivers/hv/vmbus_drv.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hyperv.h | 16 ++-
6 files changed, 497 insertions(+), 49 deletions(-)

--
1.8.3.1


2019-09-06 09:43:20

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH v5 1/9] Drivers: hv: vmbus: Break out synic enable and disable operations

Break out synic enable and disable operations into separate
hv_synic_disable_regs() and hv_synic_enable_regs() functions for use by a
later patch to support hibernation.

There is no functional change except the unnecessary check
"if (sctrl.enable != 1) return -EFAULT;" which is removed, because when
we're in hv_synic_cleanup(), we're absolutely sure sctrl.enable must be 1.

Signed-off-by: Dexuan Cui <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
---
drivers/hv/hv.c | 66 ++++++++++++++++++++++++++---------------------
drivers/hv/hyperv_vmbus.h | 2 ++
2 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 6188fb7..fcc5279 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -154,7 +154,7 @@ void hv_synic_free(void)
* retrieve the initialized message and event pages. Otherwise, we create and
* initialize the message and event pages.
*/
-int hv_synic_init(unsigned int cpu)
+void hv_synic_enable_regs(unsigned int cpu)
{
struct hv_per_cpu_context *hv_cpu
= per_cpu_ptr(hv_context.cpu_context, cpu);
@@ -196,6 +196,11 @@ int hv_synic_init(unsigned int cpu)
sctrl.enable = 1;

hv_set_synic_state(sctrl.as_uint64);
+}
+
+int hv_synic_init(unsigned int cpu)
+{
+ hv_synic_enable_regs(cpu);

hv_stimer_init(cpu);

@@ -205,20 +210,45 @@ int hv_synic_init(unsigned int cpu)
/*
* hv_synic_cleanup - Cleanup routine for hv_synic_init().
*/
-int hv_synic_cleanup(unsigned int cpu)
+void hv_synic_disable_regs(unsigned int cpu)
{
union hv_synic_sint shared_sint;
union hv_synic_simp simp;
union hv_synic_siefp siefp;
union hv_synic_scontrol sctrl;
+
+ hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
+
+ shared_sint.masked = 1;
+
+ /* Need to correctly cleanup in the case of SMP!!! */
+ /* Disable the interrupt */
+ hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
+
+ hv_get_simp(simp.as_uint64);
+ simp.simp_enabled = 0;
+ simp.base_simp_gpa = 0;
+
+ hv_set_simp(simp.as_uint64);
+
+ hv_get_siefp(siefp.as_uint64);
+ siefp.siefp_enabled = 0;
+ siefp.base_siefp_gpa = 0;
+
+ hv_set_siefp(siefp.as_uint64);
+
+ /* Disable the global synic bit */
+ hv_get_synic_state(sctrl.as_uint64);
+ sctrl.enable = 0;
+ hv_set_synic_state(sctrl.as_uint64);
+}
+
+int hv_synic_cleanup(unsigned int cpu)
+{
struct vmbus_channel *channel, *sc;
bool channel_found = false;
unsigned long flags;

- hv_get_synic_state(sctrl.as_uint64);
- if (sctrl.enable != 1)
- return -EFAULT;
-
/*
* Search for channels which are bound to the CPU we're about to
* cleanup. In case we find one and vmbus is still connected we need to
@@ -249,29 +279,7 @@ int hv_synic_cleanup(unsigned int cpu)

hv_stimer_cleanup(cpu);

- hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
-
- shared_sint.masked = 1;
-
- /* Need to correctly cleanup in the case of SMP!!! */
- /* Disable the interrupt */
- hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
-
- hv_get_simp(simp.as_uint64);
- simp.simp_enabled = 0;
- simp.base_simp_gpa = 0;
-
- hv_set_simp(simp.as_uint64);
-
- hv_get_siefp(siefp.as_uint64);
- siefp.siefp_enabled = 0;
- siefp.base_siefp_gpa = 0;
-
- hv_set_siefp(siefp.as_uint64);
-
- /* Disable the global synic bit */
- sctrl.enable = 0;
- hv_set_synic_state(sctrl.as_uint64);
+ hv_synic_disable_regs(cpu);

return 0;
}
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 362e70e..26ea161 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -171,8 +171,10 @@ extern int hv_post_message(union hv_connection_id connection_id,

extern void hv_synic_free(void);

+extern void hv_synic_enable_regs(unsigned int cpu);
extern int hv_synic_init(unsigned int cpu);

+extern void hv_synic_disable_regs(unsigned int cpu);
extern int hv_synic_cleanup(unsigned int cpu);

/* Interface */
--
1.8.3.1

2019-09-06 09:43:36

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH v5 2/9] Drivers: hv: vmbus: Suspend/resume the synic for hibernation

This is needed when we resume the old kernel from the "current" kernel.

Note: when hv_synic_suspend() and hv_synic_resume() run, all the
non-boot CPUs have been offlined, and interrupts are disabled on CPU0.

Signed-off-by: Dexuan Cui <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
---
drivers/hv/vmbus_drv.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index ebd35fc..2ef375c 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -30,6 +30,7 @@
#include <linux/kdebug.h>
#include <linux/efi.h>
#include <linux/random.h>
+#include <linux/syscore_ops.h>
#include <clocksource/hyperv_timer.h>
#include "hyperv_vmbus.h"

@@ -2086,6 +2087,47 @@ static void hv_crash_handler(struct pt_regs *regs)
hyperv_cleanup();
};

+static int hv_synic_suspend(void)
+{
+ /*
+ * When we reach here, all the non-boot CPUs have been offlined, and
+ * the stimers on them have been unbound in hv_synic_cleanup() ->
+ * hv_stimer_cleanup() -> clockevents_unbind_device().
+ *
+ * hv_synic_suspend() only runs on CPU0 with interrupts disabled. Here
+ * we do not unbind the stimer on CPU0 because: 1) it's unnecessary
+ * because the interrupts remain disabled between syscore_suspend()
+ * and syscore_resume(): see create_image() and resume_target_kernel();
+ * 2) the stimer on CPU0 is automatically disabled later by
+ * syscore_suspend() -> timekeeping_suspend() -> tick_suspend() -> ...
+ * -> clockevents_shutdown() -> ... -> hv_ce_shutdown(); 3) a warning
+ * would be triggered if we call clockevents_unbind_device(), which
+ * may sleep, in an interrupts-disabled context. So, we intentionally
+ * don't call hv_stimer_cleanup(0) here.
+ */
+
+ hv_synic_disable_regs(0);
+
+ return 0;
+}
+
+static void hv_synic_resume(void)
+{
+ hv_synic_enable_regs(0);
+
+ /*
+ * Note: we don't need to call hv_stimer_init(0), because the timer
+ * on CPU0 is not unbound in hv_synic_suspend(), and the timer is
+ * automatically re-enabled in timekeeping_resume().
+ */
+}
+
+/* The callbacks run only on CPU0, with irqs_disabled. */
+static struct syscore_ops hv_synic_syscore_ops = {
+ .suspend = hv_synic_suspend,
+ .resume = hv_synic_resume,
+};
+
static int __init hv_acpi_init(void)
{
int ret, t;
@@ -2116,6 +2158,8 @@ static int __init hv_acpi_init(void)
hv_setup_kexec_handler(hv_kexec_handler);
hv_setup_crash_handler(hv_crash_handler);

+ register_syscore_ops(&hv_synic_syscore_ops);
+
return 0;

cleanup:
@@ -2128,6 +2172,8 @@ static void __exit vmbus_exit(void)
{
int cpu;

+ unregister_syscore_ops(&hv_synic_syscore_ops);
+
hv_remove_kexec_handler();
hv_remove_crash_handler();
vmbus_connection.conn_state = DISCONNECTED;
--
1.8.3.1

2019-09-06 09:53:00

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH v5 9/9] Drivers: hv: vmbus: Resume after fixing up old primary channels

When the host re-offers the primary channels upon resume, the host only
guarantees the Instance GUID doesn't change, so vmbus_bus_suspend()
should invalidate channel->offermsg.child_relid and figure out the
number of primary channels that need to be fixed up upon resume.

Upon resume, vmbus_onoffer() finds the old channel structs, and maps
the new offers to the old channels, and fixes up the old structs,
and finally the resume callbacks of the VSC drivers will re-open
the channels.

Signed-off-by: Dexuan Cui <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
---
drivers/hv/channel_mgmt.c | 85 ++++++++++++++++++++++++++++++++++++-----------
drivers/hv/connection.c | 2 ++
drivers/hv/hyperv_vmbus.h | 14 ++++++++
drivers/hv/vmbus_drv.c | 17 ++++++++++
include/linux/hyperv.h | 3 ++
5 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 5518d03..8eb1675 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -407,7 +407,15 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
cpumask_clear_cpu(channel->target_cpu,
&primary_channel->alloced_cpus_in_node);

- vmbus_release_relid(channel->offermsg.child_relid);
+ /*
+ * Upon suspend, an in-use hv_sock channel is marked as "rescinded" and
+ * the relid is invalidated; after hibernation, when the user-space app
+ * destroys the channel, the relid is INVALID_RELID, and in this case
+ * it's unnecessary and unsafe to release the old relid, since the same
+ * relid can refer to a completely different channel now.
+ */
+ if (channel->offermsg.child_relid != INVALID_RELID)
+ vmbus_release_relid(channel->offermsg.child_relid);

free_channel(channel);
}
@@ -851,6 +859,36 @@ 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)
+{
+ /*
+ * Setup state for signalling the host.
+ */
+ channel->sig_event = VMBUS_EVENT_CONNECTION_ID;
+
+ if (vmbus_proto_version != VERSION_WS2008) {
+ channel->is_dedicated_interrupt =
+ (offer->is_dedicated_interrupt != 0);
+ channel->sig_event = offer->connection_id;
+ }
+
+ memcpy(&channel->offermsg, offer,
+ sizeof(struct vmbus_channel_offer_channel));
+ channel->monitor_grp = (u8)offer->monitorid / 32;
+ channel->monitor_bit = (u8)offer->monitorid % 32;
+}
+
/*
* find_primary_channel_by_offer - Get the channel object given the new offer.
* This is only used in the resume path of hibernation.
@@ -902,14 +940,29 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
atomic_dec(&vmbus_connection.offer_in_progress);

/*
- * We're resuming from hibernation: we expect the host to send
- * exactly the same offers that we had before the hibernation.
+ * We're resuming from hibernation: all the sub-channel and
+ * hv_sock channels we had before the hibernation should have
+ * been cleaned up, and now we must be seeing a re-offered
+ * primary channel that we had before the hibernation.
*/
+
+ WARN_ON(oldchannel->offermsg.child_relid != INVALID_RELID);
+ /* Fix up the relid. */
+ oldchannel->offermsg.child_relid = offer->child_relid;
+
offer_sz = sizeof(*offer);
- if (memcmp(offer, &oldchannel->offermsg, offer_sz) == 0)
+ if (memcmp(offer, &oldchannel->offermsg, offer_sz) == 0) {
+ check_ready_for_resume_event();
return;
+ }

- pr_debug("Mismatched offer from the host (relid=%d)\n",
+ /*
+ * This is not an error, since the host can also change the
+ * other field(s) of the offer, e.g. on WS RS5 (Build 17763),
+ * the offer->connection_id of the Mellanox VF vmbus device
+ * can change when the host reoffers the device upon resume.
+ */
+ pr_debug("vmbus offer changed: relid=%d\n",
offer->child_relid);

print_hex_dump_debug("Old vmbus offer: ", DUMP_PREFIX_OFFSET,
@@ -917,6 +970,12 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
false);
print_hex_dump_debug("New vmbus offer: ", DUMP_PREFIX_OFFSET,
16, 4, offer, offer_sz, false);
+
+ /* Fix up the old channel. */
+ vmbus_setup_channel_state(oldchannel, offer);
+
+ check_ready_for_resume_event();
+
return;
}

@@ -929,21 +988,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
return;
}

- /*
- * Setup state for signalling the host.
- */
- newchannel->sig_event = VMBUS_EVENT_CONNECTION_ID;
-
- if (vmbus_proto_version != VERSION_WS2008) {
- newchannel->is_dedicated_interrupt =
- (offer->is_dedicated_interrupt != 0);
- newchannel->sig_event = offer->connection_id;
- }
-
- memcpy(&newchannel->offermsg, offer,
- sizeof(struct vmbus_channel_offer_channel));
- newchannel->monitor_grp = (u8)offer->monitorid / 32;
- newchannel->monitor_bit = (u8)offer->monitorid % 32;
+ vmbus_setup_channel_state(newchannel, offer);

vmbus_process_offer(newchannel);
}
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 99851ea..6e4c015 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -29,6 +29,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),
};
EXPORT_SYMBOL_GPL(vmbus_connection);

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 974b747..f7a5f56 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -272,6 +272,20 @@ struct vmbus_connection {
* drop to zero.
*/
struct completion ready_for_suspend_event;
+
+ /*
+ * The number of primary channels that should be "fixed up"
+ * upon resume: these channels are re-offered upon resume, and some
+ * fields of the channel offers (i.e. child_relid and connection_id)
+ * can change, so the old offermsg must be fixed up, before the resume
+ * callbacks of the VSC drivers start to further touch the channels.
+ */
+ atomic_t nr_chan_fixup_on_resume;
+ /*
+ * vmbus_bus_resume() waits for "nr_chan_fixup_on_resume" to
+ * drop to zero.
+ */
+ struct completion ready_for_resume_event;
};


diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 32ec951..391f0b2 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2164,9 +2164,17 @@ static int vmbus_bus_suspend(struct device *dev)
if (atomic_read(&vmbus_connection.nr_chan_close_on_suspend) > 0)
wait_for_completion(&vmbus_connection.ready_for_suspend_event);

+ WARN_ON(atomic_read(&vmbus_connection.nr_chan_fixup_on_resume) != 0);
+
mutex_lock(&vmbus_connection.channel_mutex);

list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
+ /*
+ * Invalidate the field. Upon resume, vmbus_onoffer() will fix
+ * up the field, and the other fields (if necessary).
+ */
+ channel->offermsg.child_relid = INVALID_RELID;
+
if (is_hvsock_channel(channel)) {
if (!channel->rescind) {
pr_err("hv_sock channel not rescinded!\n");
@@ -2181,6 +2189,8 @@ static int vmbus_bus_suspend(struct device *dev)
WARN_ON_ONCE(1);
}
spin_unlock_irqrestore(&channel->lock, flags);
+
+ atomic_inc(&vmbus_connection.nr_chan_fixup_on_resume);
}

mutex_unlock(&vmbus_connection.channel_mutex);
@@ -2189,6 +2199,9 @@ static int vmbus_bus_suspend(struct device *dev)

vmbus_connection.conn_state = DISCONNECTED;

+ /* Reset the event for the next resume. */
+ reinit_completion(&vmbus_connection.ready_for_resume_event);
+
return 0;
}

@@ -2223,8 +2236,12 @@ static int vmbus_bus_resume(struct device *dev)
if (ret != 0)
return ret;

+ WARN_ON(atomic_read(&vmbus_connection.nr_chan_fixup_on_resume) == 0);
+
vmbus_request_offers();

+ wait_for_completion(&vmbus_connection.ready_for_resume_event);
+
/* Reset the event for the next suspend. */
reinit_completion(&vmbus_connection.ready_for_suspend_event);

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 8a60e77..a3aa9e9 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -426,6 +426,9 @@ enum vmbus_channel_message_type {
CHANNELMSG_COUNT
};

+/* Hyper-V supports about 2048 channels, and the RELIDs start with 1. */
+#define INVALID_RELID U32_MAX
+
struct vmbus_channel_message_header {
enum vmbus_channel_message_type msgtype;
u32 padding;
--
1.8.3.1

2019-09-07 17:48:28

by Sasha Levin

[permalink] [raw]
Subject: Re: [PATCH v5 0/9] Enhance the hv_vmbus driver to support hibernation

On Thu, Sep 05, 2019 at 11:01:14PM +0000, Dexuan Cui wrote:
>This patchset (consisting of 9 patches) was part of the v4 patchset (consisting
>of 12 patches):
> https://lkml.org/lkml/2019/9/2/894
>
>The other 3 patches in v4 are posted in another patchset, which will go
>through the tip.git tree.
>
>All the 9 patches here are now rebased to the hyperv tree's hyperv-next branch:
>https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
>, and all the 9 patches have Michael Kelley's Signed-off-by's.
>
>Please review.

Given that these two series depend on each other, I'd much prefer for
them to go through one tree.

But, I may be wrong, and I'm going to see if a scenario such as this
make sense. I've queued this one to the hyperv-next, but I'll wait for
the x86 folks to send their pull request to Linus first before I do it
for these patches.

Usually cases such as these are the exception, but for Hyper-V it seems
to be the norm, so I'm curious to see how this will unfold.

--
Thanks,
Sasha

2019-09-08 08:08:48

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH v5 0/9] Enhance the hv_vmbus driver to support hibernation

> From: Sasha Levin <[email protected]>
> Sent: Friday, September 6, 2019 1:03 PM
> On Thu, Sep 05, 2019 at 11:01:14PM +0000, Dexuan Cui wrote:
> >This patchset (consisting of 9 patches) was part of the v4 patchset (consisting
> >of 12 patches):
> >
> >The other 3 patches in v4 are posted in another patchset, which will go
> >through the tip.git tree.
> >
> >All the 9 patches here are now rebased to the hyperv tree's hyperv-next
> branch, and all the 9 patches have Michael Kelley's Signed-off-by's.
> >
> >Please review.
>
> Given that these two series depend on each other, I'd much prefer for
> them to go through one tree.

Hi Sasha,
Yeah, that would be ideal. The problem here is: the other patchset conflicts
with the existing patches in the tip.git tree's timers/core branch, so IMO
the 3 patches have to go through the tip tree:

[PATCH v5 1/3] x86/hyper-v: Suspend/resume the hypercall page for hibernation
[PATCH v5 2/3] x86/hyper-v: Implement hv_is_hibernation_supported()
[PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

> But, I may be wrong, and I'm going to see if a scenario such as this
> make sense. I've queued this one to the hyperv-next, but I'll wait for
> the x86 folks to send their pull request to Linus first before I do it
> for these patches.

Actually IMHO you don't need to wait, because there is not a build
dependency, so either patchset can go into the Linus's tree first.

The 2 patchsets are just the first step to make hibernation work for Linux VM
running on Hyper-V. Next I'm going to post some high-level VSC patches for
hv_balloon, hv_utils, hv_netvsc, hid_hyperv, hv_storvsc, hyperv_keyboard,
hyperv_fb,etc. All of these should go through the hyperv tree, since they're
pure hyper-v changes, and they depend on this 9-patch patchset. I'll make
a note in every patch so the subsystem maintainers will be aware and ack it.

Among the VSC patches, the hv_balloon patch does depend on the 2nd patch:
[PATCH v5 2/3] x86/hyper-v: Implement hv_is_hibernation_supported().
I think I'll wait for the aforementioned 2 patchsets to be in first, before posting
the hv_balloon patch.

> Usually cases such as these are the exception, but for Hyper-V it seems
> to be the norm, so I'm curious to see how this will unfold.
>
> Thanks,
> Sasha

Thanks for taking care all the patches!

Thanks,
-- Dexuan

2019-09-09 09:53:04

by Sasha Levin

[permalink] [raw]
Subject: Re: [PATCH v5 0/9] Enhance the hv_vmbus driver to support hibernation

On Fri, Sep 06, 2019 at 10:45:31PM +0000, Dexuan Cui wrote:
>> From: Sasha Levin <[email protected]>
>> Sent: Friday, September 6, 2019 1:03 PM
>> On Thu, Sep 05, 2019 at 11:01:14PM +0000, Dexuan Cui wrote:
>> >This patchset (consisting of 9 patches) was part of the v4 patchset (consisting
>> >of 12 patches):
>> >
>> >The other 3 patches in v4 are posted in another patchset, which will go
>> >through the tip.git tree.
>> >
>> >All the 9 patches here are now rebased to the hyperv tree's hyperv-next
>> branch, and all the 9 patches have Michael Kelley's Signed-off-by's.
>> >
>> >Please review.
>>
>> Given that these two series depend on each other, I'd much prefer for
>> them to go through one tree.
>
>Hi Sasha,
>Yeah, that would be ideal. The problem here is: the other patchset conflicts
>with the existing patches in the tip.git tree's timers/core branch, so IMO
>the 3 patches have to go through the tip tree:
>
>[PATCH v5 1/3] x86/hyper-v: Suspend/resume the hypercall page for hibernation
>[PATCH v5 2/3] x86/hyper-v: Implement hv_is_hibernation_supported()
>[PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation
>
>> But, I may be wrong, and I'm going to see if a scenario such as this
>> make sense. I've queued this one to the hyperv-next, but I'll wait for
>> the x86 folks to send their pull request to Linus first before I do it
>> for these patches.
>
>Actually IMHO you don't need to wait, because there is not a build
>dependency, so either patchset can go into the Linus's tree first.

It'll build, sure. But did anyone actually test one without the other?
What happens if Thomas doesn't send his batch at all during the merge
window?

--
Thanks,
Sasha

2019-09-09 17:22:19

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH v5 0/9] Enhance the hv_vmbus driver to support hibernation

> From: Sasha Levin <[email protected]>
> Sent: Sunday, September 8, 2019 5:13 AM
> On Fri, Sep 06, 2019 at 10:45:31PM +0000, Dexuan Cui wrote:
> >> From: Sasha Levin <[email protected]>
> >> Sent: Friday, September 6, 2019 1:03 PM
> >> On Thu, Sep 05, 2019 at 11:01:14PM +0000, Dexuan Cui wrote:
> >> >This patchset (consisting of 9 patches) was part of the v4 patchset
> (consisting
> >> >of 12 patches):
> >> >
> >> >The other 3 patches in v4 are posted in another patchset, which will go
> >> >through the tip.git tree.
> >> >
> >> >All the 9 patches here are now rebased to the hyperv tree's hyperv-next
> >> branch, and all the 9 patches have Michael Kelley's Signed-off-by's.
> >> >
> >> >Please review.
> >>
> >> Given that these two series depend on each other, I'd much prefer for
> >> them to go through one tree.
> >
> >Hi Sasha,
> >Yeah, that would be ideal. The problem here is: the other patchset conflicts
> >with the existing patches in the tip.git tree's timers/core branch, so IMO
> >the 3 patches have to go through the tip tree:
> >
> >[PATCH v5 1/3] x86/hyper-v: Suspend/resume the hypercall page for
> hibernation
> >[PATCH v5 2/3] x86/hyper-v: Implement hv_is_hibernation_supported()
> >[PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for
> hibernation
> >
> >> But, I may be wrong, and I'm going to see if a scenario such as this
> >> make sense. I've queued this one to the hyperv-next, but I'll wait for
> >> the x86 folks to send their pull request to Linus first before I do it
> >> for these patches.
> >
> >Actually IMHO you don't need to wait, because there is not a build
> >dependency, so either patchset can go into the Linus's tree first.
>
> It'll build, sure. But did anyone actually test one without the other?

Nobody tested this.

The fact is: even if we have the 2 patchsets, hibernation still can not work
for Linux VM on Hyper-V, because we also need the high level driver
changes to hv_netvsc, hv_storvsc, etc. I'm going to send out these
patches soon.

> What happens if Thomas doesn't send his batch at all during the merge
> window?
> Sasha

We need all the patches together to make hibernation work.
I just meant that the 2 patchsets don't have to go into Linus's tree in
a special order, as there is no build issue.

Thanks,
-- Dexuan