Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Apr 2021 13:39:18 +0200
From:   Andrea Parri <parri.andrea@gmail.com>
To:     Michael Kelley <mikelley@microsoft.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        "wei.liu@kernel.org" <wei.liu@kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] Drivers: hv: vmbus: Check for pending channel
 interrupts before taking a CPU offline
Message-ID: <20210415113918.GA75451@anparri>
References: <20210414150118.2843-1-parri.andrea@gmail.com>
 <20210414150118.2843-4-parri.andrea@gmail.com>
 <MWHPR21MB15931F523196BCE9E23293E4D74E9@MWHPR21MB1593.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MWHPR21MB15931F523196BCE9E23293E4D74E9@MWHPR21MB1593.namprd21.prod.outlook.com>
Precedence: bulk

> > @@ -336,6 +372,19 @@ int hv_synic_cleanup(unsigned int cpu)
> >  	if (channel_found && vmbus_connection.conn_state == CONNECTED)
> >  		return -EBUSY;
> > 
> > +	if (vmbus_proto_version >= VERSION_WIN10_V4_1) {
> > +		/*
> > +		 * channel_found == false means that any channels that were previously
> > +		 * assigned to the CPU have been reassigned elsewhere with a call of
> > +		 * vmbus_send_modifychannel().  Scan the event flags page looking for
> > +		 * bits that are set and waiting with a timeout for vmbus_chan_sched()
> > +		 * to process such bits.  If bits are still set after this operation
> > +		 * and VMBus is connected, fail the CPU offlining operation.
> > +		 */
> > +		if (hv_synic_event_pending() && vmbus_connection.conn_state == CONNECTED)
> > +			return -EBUSY;
> > +	}
> > +
> 
> Perhaps the test for conn_state == CONNECTED could be factored out as follows.  If we're
> not CONNECTED (i.e., in the panic or kexec path) we should be able to also skip the search
> for channels that are bound to the CPU since we will ignore the result anyway.
> 
> 	if (vmbus_connection.conn_state != CONNECTED)
> 		goto always_cleanup;
> 
> 	if (cpu == VMBUS_CONNECT_CPU)
> 		return -EBUSY;
> 
> 	[Code to search for channels that are bound to the CPU we're about to clean up]
> 	
> 	if (channel_found)
> 		return -EBUSY;
> 
> 	/*
> 	 * channel_found == false means that any channels that were previously
> 	 * assigned to the CPU have been reassigned elsewhere with a call of
> 	 * vmbus_send_modifychannel().  Scan the event flags page looking for
> 	 * bits that are set and waiting with a timeout for vmbus_chan_sched()
> 	 * to process such bits.  If bits are still set after this operation
> 	 * and VMBus is connected, fail the CPU offlining operation.
> 	 */
> 	if (vmbus_proto_version >= VERSION_WIN10_V4_1 && hv_synic_event_pending())
> 		return -EBUSY;
> 
> always_cleanup:

Agreed, applied.  Thank you for the suggestion,

  Andrea