[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4c9e670b-eb37-4bdb-adcf-a4ebbebcefab@linux.microsoft.com>
Date: Thu, 7 Nov 2024 11:14:52 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>,
"K . Y . Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
Dexuan Cui <decui@...rosoft.com>
Cc: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
John Starks <jostarks@...rosoft.com>,
"jacob.pan@...ux.microsoft.com" <jacob.pan@...ux.microsoft.com>,
Easwar Hariharan <eahariha@...ux.microsoft.com>,
Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
Subject: Re: [PATCH v2 2/2] Drivers: hv: vmbus: Log on missing offers
On 11/1/2024 12:44 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@...ux.microsoft.com> Sent: Tuesday, October 29, 2024 1:02 AM
>>
>> When resuming from hibernation, log any channels that were present
>> before hibernation but now are gone.
>> In general, the essential virtual devices configured for a VM, remain
>> same, before and after the hibernation and its not very common that
>> some offers are missing.
>
> The wording here is a bit jumbled. And let's use consistent terminology.
> I'd suggest:
>
> In general, the boot-time devices configured for a resuming VM should be
> the same as the devices in the VM at the time of hibernation. It's uncommon
> for the configuration to have been changed such that offers are missing.
> Changing the configuration violates the rules for hibernation anyway.
Sure, I'll make the required changes.
>
>> The cleanup of missing channels is not
>> straight-forward and dependent on individual device driver
>> functionality and implementation, so it can be added in future as
>> separate changes.
>>
>> Signed-off-by: John Starks <jostarks@...rosoft.com>
>> Co-developed-by: Naman Jain <namjain@...ux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@...ux.microsoft.com>
>> Reviewed-by: Easwar Hariharan <eahariha@...ux.microsoft.com>
>> ---
>> Changes since v1:
>> https://lore.kernel.org/all/20241018115811.5530-1-namjain@linux.microsoft.com/
>> * Added Easwar's Reviewed-By tag
>> * Addressed Saurabh's comments:
>> * Added a note for missing channel cleanup in comments and commit msg
>> ---
>> drivers/hv/vmbus_drv.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index bd3fc41dc06b..08214f28694a 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,22 @@ 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);
>> + /* ToDo: Cleanup these channels here */
>> + }
>> + mutex_unlock(&vmbus_connection.channel_mutex);
>> +
>
> Dexuan and John have explained how in Azure VMs, there should not be
> any VFs assigned to the VM at the time of hibernation. So the above
> check for missing offers does not trigger an error message due to
> VF offers coming after the all-offers-received message.
>
> But what about the case of a VM running on a local Hyper-V? I'm not
> completely clear, but in that case I don't think any VFs are removed
> before the hibernation, especially for VM-initiated hibernation. It's
I am not sure about this behavior. I have requested Dexuan offline
for a comment.
> a reasonable scenario to later resume that same VM, with the same
> VF assigned to the VM. Because of the way current code counts
> the offers, vmbus_bus_resume() waits for the VF to be offered again,
> and all the channels get correct post-resume relids. But the changes
> in this patch set break that scenario. Since vmbus_bus_resume() now
> proceeds before the VF offer arrives, hv_pci_resume() calling
> vmbus_open() could use the pre-hibernation relid for the VF and break
> things. Certainly the "not present after resume" error message would
> be spurious.
>
> Maybe the focus here is Azure, and it's tolerable for the local Hyper-V
> case with a VF to not work pending later fixes. But I thought I'd call
> out the potential issue (assuming my thinking is correct).
>
> Michael
IIUC, below scenarios can happen based on your comment-
Case 1:
VF channel offer is received in time before hv_pci_resume() and there
are no problems.
Case 2:
Resume proceeds just after getting ALLOFFERS_DELIVERED msg and a warning
is printed that this VF channel is not present after resume.
Then two scenarios can happen:
Case 2.1:
VF channel offer is received before hv_pci_resume() and things work
fine. Warning printed is spurious.
Case 2.2:
VM channel offer is not received before hv_pci_resume() and relid is
not yet restored by onoffer. This is a problem. Warning is printed in
this case for missing offer.
I think it all depends on whether or not VFs are removed in local
HyperV VMs. I'll try to get this information. Thanks for pointing this
out.
Regards,
Naman
>
>> /* Reset the event for the next suspend. */
>> reinit_completion(&vmbus_connection.ready_for_suspend_event);
>>
>> --
>> 2.34.1
Powered by blists - more mailing lists