From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965582AbdCWQDm (ORCPT ); Thu, 23 Mar 2017 12:03:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39944 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965036AbdCWQDj (ORCPT ); Thu, 23 Mar 2017 12:03:39 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1E24C3F75C Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=vkuznets@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 1E24C3F75C From: Vitaly Kuznetsov To: Long Li Cc: KY Srinivasan , Haiyang Zhang , Stephen Hemminger , "devel\@linuxdriverproject.org" , "linux-kernel\@vger.kernel.org" , "stable\@vger.kernel.org" Subject: Re: [PATCH v2] HV: properly delay KVP packets when negotiation is in progress References: Date: Thu, 23 Mar 2017 17:03:36 +0100 In-Reply-To: (Long Li's message of "Wed, 22 Mar 2017 18:48:14 +0000") Message-ID: <87bmssyo87.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 23 Mar 2017 16:03:39 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Long Li 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 > > > The patch v2 has incorporated suggestion from Vitaly Kuznetsov > . > > Signed-off-by: Long Li > --- > 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