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

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

[Long Li]

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.

This patch is based on the work of Nick Meier <Nick.Meier@microsoft.com>

Signed-off-by: Long Li <longli@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)
-- 
2.7.4

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

[Joshua R. Poulson]

This should be submitted to stable, we're seeing KVP fail to start in
some tests of 4.10.

On Thu, Mar 16, 2017 at 12:51 PM, Long Li <longli@microsoft.com> wrote:
> 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.
>
> This patch is based on the work of Nick Meier <Nick.Meier@microsoft.com>
>
> Signed-off-by: Long Li <longli@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)
> --
> 2.7.4

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

[Vitaly Kuznetsov]

Long Li <longli@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?


>
> This patch is based on the work of Nick Meier <Nick.Meier@microsoft.com>
>
> Signed-off-by: Long Li <longli@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

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

[Long Li]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Friday, March 17, 2017 9:16 AM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] HV: properly delay KVP packets when negotiation is in
> progress
> 
> Long Li <longli@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.

> 
> 
> >
> > This patch is based on the work of Nick Meier
> > <Nick.Meier@microsoft.com>
> >
> > Signed-off-by: Long Li <longli@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

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

[Long Li]

> -----Original Message-----
> From: Long Li
> Sent: Sunday, March 19, 2017 7:49 PM
> To: 'Vitaly Kuznetsov' <vkuznets@redhat.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH] HV: properly delay KVP packets when negotiation is in
> progress
> 
> 
> 
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > Sent: Friday, March 17, 2017 9:16 AM
> > To: Long Li <longli@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>;
> > devel@linuxdriverproject.org; linux- kernel@vger.kernel.org
> > Subject: Re: [PATCH] HV: properly delay KVP packets when negotiation
> > is in progress
> >
> > Long Li <longli@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@microsoft.com>
> > >
> > > Signed-off-by: Long Li <longli@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