RE: [PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels

From: Michael Kelley <mikelley@microsoft.com>
To: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	Dexuan Cui <decui@microsoft.com>,
	Boqun Feng <boqun.feng@gmail.com>, vkuznets <vkuznets@redhat.com>
Subject: RE: [PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels
Date: Fri, 10 Apr 2020 17:29:43 +0000	[thread overview]
Message-ID: <MW2PR2101MB10524E75CD610DFB4EBEF091D7DE0@MW2PR2101MB1052.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20200406001514.19876-4-parri.andrea@gmail.com>

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Sunday, April 5, 2020 5:15 PM
> 
> When Hyper-V sends an interrupt to the guest, the guest has to figure
> out which channel the interrupt is associated with.  Hyper-V sets a bit
> in a memory page that is shared with the guest, indicating a particular
> "relid" that the interrupt is associated with.  The current Linux code
> then uses a set of per-CPU linked lists to map a given "relid" to a
> pointer to a channel structure.
> 
> This design introduces a synchronization problem if the CPU that Hyper-V
> will interrupt for a certain channel is changed.  If the interrupt comes
> on the "old CPU" and the channel was already moved to the per-CPU list
> of the "new CPU", then the relid -> channel mapping will fail and the
> interrupt is dropped.  Similarly, if the interrupt comes on the new CPU
> but the channel was not moved to the per-CPU list of the new CPU, then
> the mapping will fail and the interrupt is dropped.
> 
> Relids are integers ranging from 0 to 2047.  The mapping from relids to
> channel structures can be done by setting up an array with 2048 entries,
> each entry being a pointer to a channel structure (hence total size ~16K
> bytes, which is not a problem).  The array is global, so there are no
> per-CPU linked lists to update.  The array can be searched and updated
> by loading from/storing to the array at the specified index.  With no
> per-CPU data structures, the above mentioned synchronization problem is
> avoided and the relid2channel() function gets simpler.
> 
> Suggested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> NOTE.  The inline comment in vmbus_channel_map_relid() follows the argument
> discussed in the RFC [1].  An attempt has been made to make this argument
> more 'explicit' (and, hopefully, more 'robust' against future changes) by
> adding a full memory barrier in vmbus_channel_map_relid(): the barrier takes
> the role of the 'implicit' full memory barriers from queue_work() and from
> check_ready_for_resume_event() referred to in the RFC.
> 
> [1] https://lore.kernel.org/linux-hyperv/20200403133826.GA25401@andrea/
> 
>  drivers/hv/channel_mgmt.c | 179 ++++++++++++++++++++++++--------------
>  drivers/hv/connection.c   |  38 +++-----
>  drivers/hv/hv.c           |   2 -
>  drivers/hv/hyperv_vmbus.h |  14 +--
>  drivers/hv/vmbus_drv.c    |  48 ++++++----
>  include/linux/hyperv.h    |   5 --
>  6 files changed, 160 insertions(+), 126 deletions(-)
> 

Reviewed-by: Michael Kelley <mikelley@microsoft.com>