RE: [PATCH v3 12/12] Drivers: hv: vmbus: Resume after fixing up old primary channels

From: Michael Kelley <mikelley@microsoft.com>
To: Dexuan Cui <decui@microsoft.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Sasha Levin <Alexander.Levin@microsoft.com>,
	"sashal@kernel.org" <sashal@kernel.org>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	KY Srinivasan <kys@microsoft.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 12/12] Drivers: hv: vmbus: Resume after fixing up old primary channels
Date: Fri, 23 Aug 2019 20:25:02 +0000	[thread overview]
Message-ID: <DM5PR21MB01370691E881D59773B9EF60D7A40@DM5PR21MB0137.namprd21.prod.outlook.com> (raw)
In-Reply-To: <1566265863-21252-13-git-send-email-decui@microsoft.com>

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, August 19, 2019 6:52 PM
> 
> 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 <decui@microsoft.com>
> ---
>  drivers/hv/channel_mgmt.c | 76 +++++++++++++++++++++++++++++++++++------------
>  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, 93 insertions(+), 19 deletions(-)
> 
> @@ -875,12 +913,21 @@ 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",
>  			 offer->child_relid);
> @@ -890,6 +937,11 @@ 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);
> +
> +		vmbus_setup_channel_state(oldchannel, offer);
> +
> +		check_ready_for_resume_event();

This is the error case where the new offer didn't match some aspect of
the old offer.  Is the intent to proceed and use the new offer?   I can see
that check_ready_for_resume_event() has to be called in the error case,
otherwise the resume operation will hang forever, but I'm not sure about
setting up the channel state and then proceeding as if all is good.

> +
>  		return;
>  	}
> 