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

From: Vitaly Kuznetsov
Date: Thu Mar 23 2017 - 12:03:43 EST


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.

>
> 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