[PATCH] HV: properly delay KVP packets when negotiation is in progress

Long Li longli at microsoft.com
Mon Mar 20 22:44:31 UTC 2017



> -----Original Message-----
> From: Long Li
> Sent: Sunday, March 19, 2017 7:49 PM
> To: 'Vitaly Kuznetsov' <vkuznets at redhat.com>
> Cc: KY Srinivasan <kys at microsoft.com>; Haiyang Zhang
> <haiyangz at microsoft.com>; Stephen Hemminger
> <sthemmin at microsoft.com>; devel at linuxdriverproject.org; linux-
> kernel at vger.kernel.org
> Subject: RE: [PATCH] HV: properly delay KVP packets when negotiation is in
> progress
> 
> 
> 
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets at redhat.com]
> > Sent: Friday, March 17, 2017 9:16 AM
> > To: Long Li <longli at microsoft.com>
> > Cc: KY Srinivasan <kys at microsoft.com>; Haiyang Zhang
> > <haiyangz at microsoft.com>; Stephen Hemminger
> <sthemmin at microsoft.com>;
> > devel at linuxdriverproject.org; linux- kernel at vger.kernel.org
> > Subject: Re: [PATCH] HV: properly delay KVP packets when negotiation
> > is in progress
> >
> > Long Li <longli at microsoft.com> writes:
> >
> > > The host may send multiple KVP packets before the negotiation with
> > > daemon is finished. We need to keep those packets in ring buffer
> > > until the daemon is negotiated and connected.
> >
> > The patch looks OK but previously we always presumed that this can't
> > happen for util drivers and host will never send a new request before
> > we answer to the previous one. If this is not true we may have more
> > issues which need fixing as all three drivers we have are written in a
> 'transaction'
> > fashion.
> >
> > So my question would be: can the host send multiple (KVP) packets
> > _after_ the negotiation with daemon is finished?
> 
> Thanks Vitaly. I'm checking with Windows guys and will update soon.

It's possible that hosts may send a number of staged KVP requests as soon as negotiation is done. The current KVP code can deal with a number of pending KVP requests, and respond to them one by one.

To summarize the issue this patch tries to fix:
1. When host sends a negotiation request, and it times out, the host will send another negotiation request, and so on.
2. The guest can respond to all negotiation requests from the host. All subsequent response (except for the 1st response) are ignored by the host.
3. Before negotiation is done, the host may have staged a number of pending KVP requests.
4. As soon as negotiation is done, the host sends all KVP requests to guest.

There is a corner case that if there is only one pending KVP request after the 2nd (or 3rd etc) negotiation, it may get lost. I'm testing the following code to address this condition:

@@ -705,6 +706,7 @@ void hv_kvp_onchannelcallback(void *context)
                                       VM_PKT_DATA_INBAND, 0);

                host_negotiatied = NEGO_FINISHED;
+               hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
        }

 }

Please drop this patch. I'll send V2.

> 
> >
> >
> > >
> > > This patch is based on the work of Nick Meier
> > > <Nick.Meier at microsoft.com>
> > >
> > > Signed-off-by: Long Li <longli at microsoft.com>
> > > ---
> > >  drivers/hv/hv_kvp.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index
> > > de26371..b9f928d 100644
> > > --- a/drivers/hv/hv_kvp.c
> > > +++ b/drivers/hv/hv_kvp.c
> > > @@ -628,16 +628,17 @@ void hv_kvp_onchannelcallback(void *context)
> > >  		     NEGO_IN_PROGRESS,
> > >  		     NEGO_FINISHED} host_negotiatied =
> > NEGO_NOT_STARTED;
> > >
> > > -	if (host_negotiatied == NEGO_NOT_STARTED &&
> > > -	    kvp_transaction.state < HVUTIL_READY) {
> > > +	if (kvp_transaction.state < HVUTIL_READY) {
> > >  		/*
> > >  		 * If userspace daemon is not connected and host is asking
> > >  		 * us to negotiate we need to delay to not lose messages.
> > >  		 * This is important for Failover IP setting.
> > >  		 */
> > > -		host_negotiatied = NEGO_IN_PROGRESS;
> > > -		schedule_delayed_work(&kvp_host_handshake_work,
> > > +		if (host_negotiatied == NEGO_NOT_STARTED) {
> > > +			host_negotiatied = NEGO_IN_PROGRESS;
> > > +
> > 	schedule_delayed_work(&kvp_host_handshake_work,
> > >  				      HV_UTIL_NEGO_TIMEOUT * HZ);
> > > +		}
> > >  		return;
> > >  	}
> > >  	if (kvp_transaction.state > HVUTIL_READY)
> >
> > --
> >   Vitaly


More information about the devel mailing list