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

[Long Li]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx]
> Sent: Thursday, March 23, 2017 9:04 AM
> To: Long Li <longli@xxxxxxxxxxxxx>
> Cc: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>; devel@xxxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] HV: properly delay KVP packets when negotiation is
> in progress
> 
> Long Li <longli@xxxxxxxxxxxxx> writes:
> 
> > The host may send multiple negotiation packets (due to timeout) before
> > the KVP user-mode daemon is connected. We need to defer processing
> > those packets until the daemon is negotiated and connected. It's okay
> > for guest to respond to all negotiation packets.
> >
> > In addition, the host may send multiple staged KVP requests as soon as
> > negotiation is done. We need to properly process those packets using
> > one tasklet for exclusive access to ring buffer.
> >
> > This patch is based on the work of Nick Meier
> > <Nick.Meier@xxxxxxxxxxxxx>
> >
> > The patch v2 has incorporated suggestion from Vitaly Kuznetsov
> > <vkuznets@xxxxxxxxxx>.
> >
> > Signed-off-by: Long Li <longli@xxxxxxxxxxxxx>
> > ---
> >  drivers/hv/hv_kvp.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index
> > de26371..845b70b 100644
> > --- a/drivers/hv/hv_kvp.c
> > +++ b/drivers/hv/hv_kvp.c
> > @@ -113,7 +113,7 @@ static void kvp_poll_wrapper(void *channel)  {
> >  	/* Transaction is finished, reset the state here to avoid races. */
> >  	kvp_transaction.state = HVUTIL_READY;
> > -	hv_kvp_onchannelcallback(channel);
> > +	tasklet_schedule(&((struct vmbus_channel*)channel)-
> >callback_event);
> >  }
> 
> There is one more function in the code which calls
> hv_kvp_onchannelcallback():
> 
> static void kvp_host_handshake_func(struct work_struct *dummy) {
> 	hv_poll_channel(kvp_transaction.recv_channel,
> hv_kvp_onchannelcallback); }
> 
> we can't replace hv_kvp_onchannelcallback with kvp_poll_wrapper here as
> we don't want to reset kvp_transaction.state but it seems this should also
> get updated, e.g. hv_poll_channel() here can be replaced with the direct
> 
>  tasklet_schedule(&((struct vmbus_channel*)channel)->callback_event);
> 
> call. This will ensure hv_kvp_onchannelcallback() calls are always serialized.

Thank you. I will send v3.

> 
> >
> >  static void kvp_register_done(void)
> > @@ -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) @@ -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);
> >  	}
> >
> >  }
> 
> --
>   Vitaly