Re: [PATCH v2 2/2] Drivers: hv: vmbus: Log on missing offers

[Naman Jain <namjain@linux.microsoft.com>]

On 11/1/2024 12:44 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.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@microsoft.com>
>> Co-developed-by: Naman Jain <namjain@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> Reviewed-by: Easwar Hariharan <eahariha@linux.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