* Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
From: Vitaly Kuznetsov @ 2020-04-02 16:26 UTC (permalink / raw)
To: 163, linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <3ed15a02-0b86-0ec1-6daf-df94f8fc6ba5@163.com>
163 <freedomsky1986@163.com> writes:
> On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
>> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
>> messages. Each message comes with a header (16 bytes) which specifies the
>> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
>> look at the real message length and copies the whole slot to a temporary
>> buffer before passing it to message handlers. This is potentially dangerous
>> as hypervisor doesn't have to clean the whole slot when putting a new
>> message there and a message handler can get access to some data which
>> belongs to a previous message.
>>
>> Note, this is not currently a problem because all message handlers are
>> in-kernel but eventually we may e.g. get this exported to userspace.
>>
>> Note also, that this is not a performance critical path: messages (unlike
>> events) represent rare events so it doesn't really matter (from performance
>> point of view) if we copy too much.
>>
>> Fix the issue by taking into account the real message length. The temporary
>> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> drivers/hv/vmbus_drv.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 029378c27421..2b5572146358 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>> return;
>>
>> INIT_WORK(&ctx->work, vmbus_onmessage_work);
>> - memcpy(&ctx->msg, msg, sizeof(*msg));
>> + memcpy(&ctx->msg, msg, sizeof(msg->header) +
>> + msg->header.payload_size);
>>
>
> Hi Vitaly:
> I think we still need to check whether the payload_size passed from
> Hyper-V is valid or not here to avoid cross-border issue before doing
> copying.
Sure,
the header.payload_size must be 0 <= header.payload_size <= 240
I'll add the check.
--
Vitaly
^ permalink raw reply
* RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
From: Michael Kelley @ 2020-04-03 1:52 UTC (permalink / raw)
To: vkuznets, 163, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft)
In-Reply-To: <87a73tzwdv.fsf@vitty.brq.redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, April 2, 2020 9:27 AM
>
> 163 <freedomsky1986@163.com> writes:
>
> > On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
> >> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
> >> messages. Each message comes with a header (16 bytes) which specifies the
> >> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
> >> look at the real message length and copies the whole slot to a temporary
> >> buffer before passing it to message handlers. This is potentially dangerous
> >> as hypervisor doesn't have to clean the whole slot when putting a new
> >> message there and a message handler can get access to some data which
> >> belongs to a previous message.
> >>
> >> Note, this is not currently a problem because all message handlers are
> >> in-kernel but eventually we may e.g. get this exported to userspace.
> >>
> >> Note also, that this is not a performance critical path: messages (unlike
> >> events) represent rare events so it doesn't really matter (from performance
> >> point of view) if we copy too much.
> >>
> >> Fix the issue by taking into account the real message length. The temporary
> >> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
> >>
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >> drivers/hv/vmbus_drv.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> >> index 029378c27421..2b5572146358 100644
> >> --- a/drivers/hv/vmbus_drv.c
> >> +++ b/drivers/hv/vmbus_drv.c
> >> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
> >> return;
> >>
> >> INIT_WORK(&ctx->work, vmbus_onmessage_work);
> >> - memcpy(&ctx->msg, msg, sizeof(*msg));
> >> + memcpy(&ctx->msg, msg, sizeof(msg->header) +
> >> + msg->header.payload_size);
> >>
> >
> > Hi Vitaly:
> > I think we still need to check whether the payload_size passed from
> > Hyper-V is valid or not here to avoid cross-border issue before doing
> > copying.
>
> Sure,
>
> the header.payload_size must be 0 <= header.payload_size <= 240
>
> I'll add the check.
>
With this change,
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
FWIW, all of this VMbus code, as well as the drivers for the VMbus
synthetic devices, make the fundamental assumption that Hyper-V
is trustworthy and doesn't send any malformed messages. However,
starting this summer we will be submitting changes to "harden" all
of the interactions with Hyper-V to no longer make that assumption.
All relevant fields will be checked before being used so that incorrect
memory references aren't made. This patch is one small step in that
direction. :-)
Michael
^ permalink raw reply
* RE: [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages
From: Michael Kelley @ 2020-04-03 1:54 UTC (permalink / raw)
To: vkuznets, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-3-vkuznets@redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:37 AM
>
> When we need to pass a buffer with Hyper-V message we don't need to always
> allocate 256 bytes for the message: the real message length is known from
> the header. Change 'struct onmessage_work_context' to make it possible to
> not over-allocate.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/hv/vmbus_drv.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
From: Michael Kelley @ 2020-04-03 1:55 UTC (permalink / raw)
To: vkuznets, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-4-vkuznets@redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:37 AM
>
> vmbus_onmessage() doesn't need the header of the message, it only
> uses it to get to the payload, we can pass the pointer to the
> payload directly.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/hv/channel_mgmt.c | 7 +------
> drivers/hv/vmbus_drv.c | 3 ++-
> include/linux/hyperv.h | 2 +-
> 3 files changed, 4 insertions(+), 8 deletions(-)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly
From: Michael Kelley @ 2020-04-03 1:56 UTC (permalink / raw)
To: vkuznets, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft)
In-Reply-To: <20200401103816.1406642-1-vkuznets@redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:38 AM
>
> Strictly speaking, compiler is free to use something different from 'u32'
> for 'enum vmbus_channel_message_type' (e.g. char) but it doesn't happen in
> real life, just add a BUILD_BUG_ON() guardian.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/hv/vmbus_drv.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths
From: Michael Kelley @ 2020-04-03 2:00 UTC (permalink / raw)
To: vkuznets, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft)
In-Reply-To: <20200401103816.1406642-2-vkuznets@redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:38 AM
>
> VMBus message handlers (channel_message_table) receive a pointer to
> 'struct vmbus_channel_message_header' and cast it to a structure of their
> choice, which is sometimes longer than the header. We, however, don't check
> that the message is long enough so in case hypervisor screws up we'll be
> accessing memory beyond what was allocated for temporary buffer.
>
> Previously, we used to always allocate and copy 256 bytes from message page
> to temporary buffer but this is hardly better: in case the message is
> shorter than we expect we'll be trying to consume garbage as some real
> data and no memory guarding technique will be able to identify an issue.
>
> Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
> and check against it in vmbus_on_msg_dpc(). Note, we can't require the
> exact length as new hypervisor versions may add extra fields to messages,
> we only check that the message is not shorter than we expect.
This assumes that the current structure definitions don't already
include extra fields that were added in newer versions of Hyper-V. If they did,
the minimum length test could fail on older versions of Hyper-V. But I
looked through the structure definitions and don't see any indication that
such extra fields have been added, so this should be OK.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
> drivers/hv/hyperv_vmbus.h | 1 +
> drivers/hv/vmbus_drv.c | 6 +++++
> 3 files changed, 37 insertions(+), 24 deletions(-)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
From: Vitaly Kuznetsov @ 2020-04-03 7:03 UTC (permalink / raw)
To: Michael Kelley
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft), 163,
linux-hyperv@vger.kernel.org
In-Reply-To: <MW2PR2101MB1052836B9357770FBE0C3825D7C70@MW2PR2101MB1052.namprd21.prod.outlook.com>
Michael Kelley <mikelley@microsoft.com> writes:
> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, April 2, 2020 9:27 AM
>>
>> 163 <freedomsky1986@163.com> writes:
>>
>> > On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
>> >> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
>> >> messages. Each message comes with a header (16 bytes) which specifies the
>> >> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
>> >> look at the real message length and copies the whole slot to a temporary
>> >> buffer before passing it to message handlers. This is potentially dangerous
>> >> as hypervisor doesn't have to clean the whole slot when putting a new
>> >> message there and a message handler can get access to some data which
>> >> belongs to a previous message.
>> >>
>> >> Note, this is not currently a problem because all message handlers are
>> >> in-kernel but eventually we may e.g. get this exported to userspace.
>> >>
>> >> Note also, that this is not a performance critical path: messages (unlike
>> >> events) represent rare events so it doesn't really matter (from performance
>> >> point of view) if we copy too much.
>> >>
>> >> Fix the issue by taking into account the real message length. The temporary
>> >> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>> >>
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >> ---
>> >> drivers/hv/vmbus_drv.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> >> index 029378c27421..2b5572146358 100644
>> >> --- a/drivers/hv/vmbus_drv.c
>> >> +++ b/drivers/hv/vmbus_drv.c
>> >> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>> >> return;
>> >>
>> >> INIT_WORK(&ctx->work, vmbus_onmessage_work);
>> >> - memcpy(&ctx->msg, msg, sizeof(*msg));
>> >> + memcpy(&ctx->msg, msg, sizeof(msg->header) +
>> >> + msg->header.payload_size);
>> >>
>> >
>> > Hi Vitaly:
>> > I think we still need to check whether the payload_size passed from
>> > Hyper-V is valid or not here to avoid cross-border issue before doing
>> > copying.
>>
>> Sure,
>>
>> the header.payload_size must be 0 <= header.payload_size <= 240
>>
>> I'll add the check.
>>
>
> With this change,
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
> FWIW, all of this VMbus code, as well as the drivers for the VMbus
> synthetic devices, make the fundamental assumption that Hyper-V
> is trustworthy and doesn't send any malformed messages. However,
> starting this summer we will be submitting changes to "harden" all
> of the interactions with Hyper-V to no longer make that assumption.
> All relevant fields will be checked before being used so that incorrect
> memory references aren't made. This patch is one small step in that
> direction. :-)
What about 'alternetive' Hyper-V implementations? :-)
On a more serious note, my understanding is: if your hypervisor
misbehaves it's 'game over' for you (as a guest), the only question is
how hard is it going to be to figure out what happened. In this
particular case if hypervisor send us garbase we probably want to fail
immediately and not try to handle it as a valid message.
Thank you for your review!
--
Vitaly
^ permalink raw reply
* RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths
From: Vitaly Kuznetsov @ 2020-04-03 7:07 UTC (permalink / raw)
To: Michael Kelley, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Wei Liu, Tianyu Lan, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, Andrea Parri (Microsoft)
In-Reply-To: <MW2PR2101MB1052E7B2458F489E0123C1D9D7C70@MW2PR2101MB1052.namprd21.prod.outlook.com>
Michael Kelley <mikelley@microsoft.com> writes:
> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:38 AM
>>
>> VMBus message handlers (channel_message_table) receive a pointer to
>> 'struct vmbus_channel_message_header' and cast it to a structure of their
>> choice, which is sometimes longer than the header. We, however, don't check
>> that the message is long enough so in case hypervisor screws up we'll be
>> accessing memory beyond what was allocated for temporary buffer.
>>
>> Previously, we used to always allocate and copy 256 bytes from message page
>> to temporary buffer but this is hardly better: in case the message is
>> shorter than we expect we'll be trying to consume garbage as some real
>> data and no memory guarding technique will be able to identify an issue.
>>
>> Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
>> and check against it in vmbus_on_msg_dpc(). Note, we can't require the
>> exact length as new hypervisor versions may add extra fields to messages,
>> we only check that the message is not shorter than we expect.
>
> This assumes that the current structure definitions don't already
> include extra fields that were added in newer versions of Hyper-V. If they did,
> the minimum length test could fail on older versions of Hyper-V. But I
> looked through the structure definitions and don't see any indication that
> such extra fields have been added, so this should be OK.
>
Yes, my understanding as well. When we decide to extend some of these
structures for newer VMbus version we'll have a choice: keep the require
length minimal or implement a more somplex check (e.g. add a 'length
check' function pointer to vmbus_channel_message_table_entry() -- or
pass 'length' to all message handlers and handle it ther). But as we
currently have no such cases this will definitely look over-engineered
so I decided to go the easiest way.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
>> drivers/hv/hyperv_vmbus.h | 1 +
>> drivers/hv/vmbus_drv.c | 6 +++++
>> 3 files changed, 37 insertions(+), 24 deletions(-)
>>
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
Thanks!
--
Vitaly
^ permalink raw reply
* [PATCH -next] hv_debugfs: Make hv_debug_root static
From: YueHaibing @ 2020-04-03 8:28 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu; +Cc: linux-hyperv, linux-kernel, YueHaibing
Fix sparse warning:
drivers/hv/hv_debugfs.c:14:15: warning: symbol 'hv_debug_root' was not declared. Should it be static?
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/hv/hv_debugfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/hv_debugfs.c b/drivers/hv/hv_debugfs.c
index 8a2878573582..ccf752b6659a 100644
--- a/drivers/hv/hv_debugfs.c
+++ b/drivers/hv/hv_debugfs.c
@@ -11,7 +11,7 @@
#include "hyperv_vmbus.h"
-struct dentry *hv_debug_root;
+static struct dentry *hv_debug_root;
static int hv_debugfs_delay_get(void *data, u64 *val)
{
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH 02/11] Drivers: hv: vmbus: Don't bind the offer&rescind works to a specific CPU
From: Andrea Parri @ 2020-04-03 12:04 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Michael Kelley, Dexuan Cui, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv@vger.kernel.org,
Boqun Feng, linux-kernel@vger.kernel.org
In-Reply-To: <87o8se2fpr.fsf@vitty.brq.redhat.com>
On Mon, Mar 30, 2020 at 02:24:16PM +0200, Vitaly Kuznetsov wrote:
> Michael Kelley <mikelley@microsoft.com> writes:
>
> > From: Andrea Parri <parri.andrea@gmail.com> Sent: Saturday, March 28, 2020 10:09 AM
> >>
> >> > In case we believe that OFFER -> RESCINF sequence is always ordered
> >> > by the host AND we don't care about other offers in the queue the
> >> > suggested locking is OK: we're guaranteed to process RESCIND after we
> >> > finished processing OFFER for the same channel. However, waiting for
> >> > 'offer_in_progress == 0' looks fishy so I'd suggest we at least add a
> >> > comment explaining that the wait is only needed to serialize us with
> >> > possible OFFER for the same channel - and nothing else. I'd personally
> >> > still slightly prefer the algorythm I suggested as it guarantees we take
> >> > channel_mutex with offer_in_progress == 0 -- even if there are no issues
> >> > we can think of today (not strongly though).
> >>
> >> Does it? offer_in_progress is incremented without channel_mutex...
> >>
>
> No, it does not, you're right, by itself the change is insufficient.
>
> >> IAC, I have no objections to apply the changes you suggested. To avoid
> >> misunderstandings: vmbus_bus_suspend() presents a similar usage... Are
> >> you suggesting that I apply similar changes there?
> >>
> >> Alternatively: FWIW, the comment in vmbus_onoffer_rescind() does refer
> >> to "The offer msg and the corresponding rescind msg...". I am all ears
> >> if you have any concrete suggestions to improve these comments.
> >>
> >
> > Given that waiting for 'offer_in_progress == 0' is the current code, I think
> > there's an argument to made for not changing it if the change isn't strictly
> > necessary. This patch set introduces enough change that *is* necessary. :-)
> >
>
> Sure. I was thinking a bit more about this and it seems that over years
> we've made the synchronization of channels code too complex (every time
> for a good reason but still). Now (before this series) we have at least:
>
> vmbus_connection.channel_mutex
> vmbus_connection.offer_in_progress
> channel.probe_done
> channel.rescind
> Workqueues (vmbus_connection.work_queue,
> queue_work_on(vmbus_connection.connect_cpu),...)
> channel.lock spinlock (the least of the problems)
>
> Maybe there's room for improvement? Out of top of my head I'd suggest a
> state machine for each channel (e.g something like
> OFFERED->OPENING->OPEN->RESCIND_REQ->RESCINDED->CLOSED) + refcounting
> (subchannels, open/rescind/... requests in progress, ...) + non-blocking
> request handling like "Can we handle this rescind offer now? No,
> refcount is too big. OK, rescheduling the work". Maybe not the best
> design ever and I'd gladly support any other which improves the
> readability of the code and makes all state changes and synchronization
> between them more obvious.
>
> Note, VMBus channel handling driven my messages (unlike events for ring
> buffer) is not performance critical, we just need to ensure completeness
> (all requests are handled correctly) with forward progress guarantees
> (no deadlocks).
>
> I understand the absence of 'hot' issues in the current code is what can
> make the virtue of redesign questionable and sorry for hijacking the
> series which doesn't seem to make things worse :-)
(Back here... Sorry for the delay.)
FWIW, what you wrote above makes sense to me; and *yes*, the series in
question was not intended as "let us undertake such a redesign" (quite
the opposite in fact...)
With regard to this specific patch, it seems to me that we've reached
a certain consensus or, at least, I don't see complaints ;-). Please
let me know if I misunderstood.
Thanks,
Andrea
^ permalink raw reply
* RE: [PATCH -next] hv_debugfs: Make hv_debug_root static
From: Michael Kelley @ 2020-04-03 12:21 UTC (permalink / raw)
To: YueHaibing, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20200403082845.22740-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com> Sent: Friday, April 3, 2020 1:29 AM
>
> Fix sparse warning:
>
> drivers/hv/hv_debugfs.c:14:15: warning: symbol 'hv_debug_root' was not declared. Should
> it be static?
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/hv/hv_debugfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv_debugfs.c b/drivers/hv/hv_debugfs.c
> index 8a2878573582..ccf752b6659a 100644
> --- a/drivers/hv/hv_debugfs.c
> +++ b/drivers/hv/hv_debugfs.c
> @@ -11,7 +11,7 @@
>
> #include "hyperv_vmbus.h"
>
> -struct dentry *hv_debug_root;
> +static struct dentry *hv_debug_root;
>
> static int hv_debugfs_delay_get(void *data, u64 *val)
> {
> --
> 2.17.1
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [RFC PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels
From: Andrea Parri @ 2020-04-03 13:38 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv, Michael Kelley,
Dexuan Cui, Boqun Feng
In-Reply-To: <87imim2epp.fsf@vitty.brq.redhat.com>
On Mon, Mar 30, 2020 at 02:45:54PM +0200, Vitaly Kuznetsov wrote:
> Andrea Parri <parri.andrea@gmail.com> writes:
>
> >> Correct me if I'm wrong, but currently vmbus_chan_sched() accesses
> >> per-cpu list of channels on the same CPU so we don't need a spinlock to
> >> guarantee that during an interrupt we'll be able to see the update if it
> >> happened before the interrupt (in chronological order). With a global
> >> list of relids, who guarantees that an interrupt handler on another CPU
> >> will actually see the modified list?
> >
> > Thanks for pointing this out!
> >
> > The offer/resume path presents implicit full memory barriers, program
> > -order after the array store which should guarantee the visibility of
> > the store to *all* CPUs before the offer/resume can complete (c.f.,
> >
> > tools/memory-model/Documentation/explanation.txt, Sect. #13
> >
> > and assuming that the offer/resume for a channel must complete before
> > the corresponding handler, which seems to be the case considered that
> > some essential channel fields are initialized only later...)
> >
> > IIUC, the spin lock approach you suggested will work and be "simpler";
> > an obvious side effect would be, well, a global synchronization point
> > in vmbus_chan_sched()...
> >
> > Thoughts?
>
> This is, of course, very theoretical as if we're seeing an interrupt for
> a channel at the same time we're writing its relid we're already in
> trouble. I can, however, try to suggest one tiny improvement:
Indeed. I think the idea (still quite informal) is that:
1) the mapping of the channel relid is propagated to (visible from)
all CPUs before add_channel_work is queued (full barrier in
queue_work()),
2) add_channel_work is queued before the channel is opened (aka,
before the channel ring buffer is allocate/initalized and the
OPENCHANNEL msg is sent and acked from Hyper-V, cf. OPEN_STATE),
3) the channel is opened before Hyper-V can start sending interrupts
for the channel, and hence before vmbus_chan_sched() can find the
channel relid in recv_int_page set,
4) vmbus_chan_sched() finds the channel's relid in recv_int_page
set before it search/load from the channel array (full barrier
in sync_test_and_clear_bit()).
This is for the "normal"/not resuming from hibernation case; for the
latter, notice that:
a) vmbus_isr() (and vmbus_chan_sched()) can not run until when
vmbus_bus_resume() has finished (@resume_noirq callback),
b) vmbus_bus_resume() can not complete before nr_chan_fixup_on_resume
equals 0 in check_ready_for_resume_event().
(and check_ready_for_resume_event() does also provides a full barrier).
If makes sense to you, I'll try to add some of the above in comments.
Thanks,
Andrea
>
> vmbus_chan_sched() now clean the bit in the event page and then searches
> for a channel with this relid; in case we allow the search to
> (temporary) fail we can reverse the logic: search for the channel and
> clean the bit only if we succeed. In case we fail, next time (next IRQ)
> we'll try again and likely succeed. The only purpose is to make sure no
> interrupts are ever lost. This may be an overkill, we may want to try
> to count how many times (if ever) this happens.
>
> Just a thought though.
>
> --
> Vitaly
>
^ permalink raw reply
* Re: [RFC PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Andrea Parri @ 2020-04-03 13:41 UTC (permalink / raw)
To: Michael Kelley
Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv@vger.kernel.org,
Dexuan Cui, Boqun Feng, vkuznets, James E.J. Bottomley,
Martin K. Petersen, linux-scsi@vger.kernel.org
In-Reply-To: <DM5PR2101MB104720061795F26D892D5373D7CB0@DM5PR2101MB1047.namprd21.prod.outlook.com>
On Mon, Mar 30, 2020 at 07:49:00PM +0000, Michael Kelley wrote:
> From: Andrea Parri <parri.andrea@gmail.com> Sent: Monday, March 30, 2020 11:55 AM
> >
> > > > @@ -1721,6 +1721,10 @@ static ssize_t target_cpu_store(struct vmbus_channel
> > *channel,
> > > > * in on a CPU that is different from the channel target_cpu value.
> > > > */
> > > >
> > > > + if (channel->change_target_cpu_callback)
> > > > + (*channel->change_target_cpu_callback)(channel,
> > > > + channel->target_cpu, target_cpu);
> > > > +
> > > > channel->target_cpu = target_cpu;
> > > > channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
> > > > channel->numa_node = cpu_to_node(target_cpu);
> > >
> > > I think there's an ordering problem here. The change_target_cpu_callback
> > > will allow storvsc to flush the cache that it is keeping, but there's a window
> > > after the storvsc callback releases the spin lock and before this function
> > > changes channel->target_cpu to the new value. In that window, the cache
> > > could get refilled based on the old value of channel->target_cpu, which is
> > > exactly what we don't want. Generally with caches, you have to set the new
> > > value first, then flush the cache, and I think that works in this case. The
> > > callback function doesn't depend on the value of channel->target_cpu,
> > > and any cache filling that might happen after channel->target_cpu is set
> > > to the new value but before the callback function runs is OK. But please
> > > double-check my thinking. :-)
> >
> > Sorry, I don't see the problem. AFAICT, the "cache" gets refilled based
> > on the values of alloced_cpus and on the current state of the cache but
> > not based on the value of channel->target_cpu. The callback invocation
> > uses the value of the "old" target_cpu; I think I ended up placing the
> > callback call where it is for not having to introduce a local variable
> > "old_cpu". ;-)
> >
>
> You are right. My comment is bogus.
>
> >
> > > > @@ -621,6 +621,63 @@ static inline struct storvsc_device *get_in_stor_device(
> > > >
> > > > }
> > > >
> > > > +void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old, u32 new)
> > > > +{
> > > > + struct storvsc_device *stor_device;
> > > > + struct vmbus_channel *cur_chn;
> > > > + bool old_is_alloced = false;
> > > > + struct hv_device *device;
> > > > + unsigned long flags;
> > > > + int cpu;
> > > > +
> > > > + device = channel->primary_channel ?
> > > > + channel->primary_channel->device_obj
> > > > + : channel->device_obj;
> > > > + stor_device = get_out_stor_device(device);
> > > > + if (!stor_device)
> > > > + return;
> > > > +
> > > > + /* See storvsc_do_io() -> get_og_chn(). */
> > > > + spin_lock_irqsave(&device->channel->lock, flags);
> > > > +
> > > > + /*
> > > > + * Determines if the storvsc device has other channels assigned to
> > > > + * the "old" CPU to update the alloced_cpus mask and the stor_chns
> > > > + * array.
> > > > + */
> > > > + if (device->channel != channel && device->channel->target_cpu == old) {
> > > > + cur_chn = device->channel;
> > > > + old_is_alloced = true;
> > > > + goto old_is_alloced;
> > > > + }
> > > > + list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
> > > > + if (cur_chn == channel)
> > > > + continue;
> > > > + if (cur_chn->target_cpu == old) {
> > > > + old_is_alloced = true;
> > > > + goto old_is_alloced;
> > > > + }
> > > > + }
> > > > +
> > > > +old_is_alloced:
> > > > + if (old_is_alloced)
> > > > + WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
> > > > + else
> > > > + cpumask_clear_cpu(old, &stor_device->alloced_cpus);
> > >
> > > I think target_cpu_store() can get called in parallel on multiple CPUs for different
> > > channels on the same storvsc device, but multiple changes to a single channel are
> > > serialized by higher levels of sysfs. So this function could run after multiple
> > > channels have been changed, in which case there's not just a single "old" value,
> > > and the above algorithm might not work, especially if channel->target_cpu is
> > > updated before calling this function per my earlier comment. I can see a
> > > couple of possible ways to deal with this. One is to put the update of
> > > channel->target_cpu in this function, within the spin lock boundaries so
> > > that the cache flush and target_cpu update are atomic. Another idea is to
> > > process multiple changes in this function, by building a temp copy of
> > > alloced_cpus by walking the channel list, use XOR to create a cpumask
> > > with changes, and then process all the changes in a loop instead of
> > > just handling a single change as with the current code at the old_is_alloced
> > > label. But I haven't completely thought through this idea.
> >
> > Same here: the invocations of target_cpu_store() are serialized on the
> > per-connection channel_mutex...
>
> Agreed. My comment is not valid.
>
> >
> >
> > > > @@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct
> > storvsc_device
> > > > *stor_device,
> > > > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > > > num_channels++;
> > > > }
> > > > - if (num_channels == 0)
> > > > + if (num_channels == 0) {
> > > > + stor_device->stor_chns[q_num] = stor_device->device->channel;
> > >
> > > Is the above added line just fixing a bug in the existing code? I'm not seeing how
> > > it would derive from the other changes in this patch.
> >
> > It was rather intended as an optimization: Each time I/O for a device
> > is initiated on a CPU that have "num_channels == 0" channel, the current
> > code ends up calling get_og_chn() (in the attempt to fill the cache) and
> > returns the device's primary channel. In the current code, the cost of
> > this operations is basically the cost of parsing alloced_cpus, but with
> > the changes introduced here this also involves acquiring (and releasing)
> > the primary channel's lock. I should probably put my hands forward and
> > say that I haven't observed any measurable effects due this addition in
> > my experiments; OTOH, caching the returned/"found" value made sense...
>
> OK. That's what I thought. The existing code does not produce an incorrect
> result, but the cache isn't working as intended. This fixes it.
>
> >
> >
> > > > @@ -1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
> > > > continue;
> > > > if (tgt_cpu == q_num)
> > > > continue;
> > > > - channel = stor_device->stor_chns[tgt_cpu];
> > > > + channel = READ_ONCE(
> > > > + stor_device->stor_chns[tgt_cpu]);
> > > > + if (channel == NULL)
> > > > + continue;
> > >
> > > The channel == NULL case is new because a cache flush could be happening
> > > in parallel on another CPU. I'm wondering about the tradeoffs of
> > > continuing in the loop (as you have coded in this patch) vs. a "goto" back to
> > > the top level "if" statement. With the "continue" you might finish the
> > > loop without finding any matches, and fall through to the next approach.
> > > But it's only a single I/O operation, and if it comes up with a less than
> > > optimal channel choice, it's no big deal. So I guess it's really a wash.
> >
> > Yes, I considered both approaches; they both "worked" here. I was a
> > bit concerned about the number of "possible" gotos (again, mainly a
> > theoretical issue, since I can imagine that the cash flushes will be
> > relatively "rare" events in most cases and, in any case, they happen
> > to be serialized); the "continue" looked like a suitable and simpler
> > approach/compromise, at least for the time being.
>
> Yes, I'm OK with your patch "as is". I was just thinking about the
> alternative, and evidently you did too.
Thank you for the confirmation. I'm wondering: could take this as a
Reviewed-by: for this patch? ;-)
Thanks,
Andrea
>
> >
> >
> > >
> > > > if (hv_get_avail_to_write_percent(
> > > > &channel->outbound)
> > > > > ring_avail_percent_lowater) {
> > > > @@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
> > > > for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
> > > > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > > > continue;
> > > > - channel = stor_device->stor_chns[tgt_cpu];
> > > > + channel = READ_ONCE(
> > > > + stor_device->stor_chns[tgt_cpu]);
> > > > + if (channel == NULL)
> > > > + continue;
> > >
> > > Same comment here.
> >
> > Similarly here.
>
> Agreed.
>
> >
> > Thoughts?
> >
> > Thanks,
> > Andrea
^ permalink raw reply
* Re: [RFC PATCH 10/11] Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL message type
From: Andrea Parri @ 2020-04-03 14:55 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Dexuan Cui, Michael Kelley, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv, linux-kernel,
Boqun Feng
In-Reply-To: <20200328184821.GA12184@andrea>
> > I can only think of a 'lazy' approach to setting channel CPU affinity:
> > we actually re-assign it to the target CPU when we receive first
> > interrupt there - but it's not very different from re-assigning it there
> > but still handling interrupts on the old CPU like you do.
>
> Interesting! I'm wondering whether it'd make sense to use a similar
> approach to (lazily) "unblock" the "old" CPUs; let me think more...
So, I spent some more time thinking about this... AFAICT, one main
issue with this approach (besides the fact that we'd need to "save"
the "old" CPUs...) is that we might be blocking that CPU for "too
long" (depending on the channel/device usage); also, this approach
seemed to make the handling of target_cpu a bit more involved, and
this is a concern considered that (as mentioned before) we'd like to
keep the target_cpu of *all* channels in the system balanced.
I'm sticking to the approach implemented here for now: I'm planning
to send a new version of the series with minor changes soon.
Thanks,
Andrea
^ permalink raw reply
* Re: [RFC PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels
From: Vitaly Kuznetsov @ 2020-04-03 14:56 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv, Michael Kelley,
Dexuan Cui, Boqun Feng
In-Reply-To: <20200403133826.GA25401@andrea>
Andrea Parri <parri.andrea@gmail.com> writes:
> On Mon, Mar 30, 2020 at 02:45:54PM +0200, Vitaly Kuznetsov wrote:
>> Andrea Parri <parri.andrea@gmail.com> writes:
>>
>> >> Correct me if I'm wrong, but currently vmbus_chan_sched() accesses
>> >> per-cpu list of channels on the same CPU so we don't need a spinlock to
>> >> guarantee that during an interrupt we'll be able to see the update if it
>> >> happened before the interrupt (in chronological order). With a global
>> >> list of relids, who guarantees that an interrupt handler on another CPU
>> >> will actually see the modified list?
>> >
>> > Thanks for pointing this out!
>> >
>> > The offer/resume path presents implicit full memory barriers, program
>> > -order after the array store which should guarantee the visibility of
>> > the store to *all* CPUs before the offer/resume can complete (c.f.,
>> >
>> > tools/memory-model/Documentation/explanation.txt, Sect. #13
>> >
>> > and assuming that the offer/resume for a channel must complete before
>> > the corresponding handler, which seems to be the case considered that
>> > some essential channel fields are initialized only later...)
>> >
>> > IIUC, the spin lock approach you suggested will work and be "simpler";
>> > an obvious side effect would be, well, a global synchronization point
>> > in vmbus_chan_sched()...
>> >
>> > Thoughts?
>>
>> This is, of course, very theoretical as if we're seeing an interrupt for
>> a channel at the same time we're writing its relid we're already in
>> trouble. I can, however, try to suggest one tiny improvement:
>
> Indeed. I think the idea (still quite informal) is that:
>
> 1) the mapping of the channel relid is propagated to (visible from)
> all CPUs before add_channel_work is queued (full barrier in
> queue_work()),
>
> 2) add_channel_work is queued before the channel is opened (aka,
> before the channel ring buffer is allocate/initalized and the
> OPENCHANNEL msg is sent and acked from Hyper-V, cf. OPEN_STATE),
>
> 3) the channel is opened before Hyper-V can start sending interrupts
> for the channel, and hence before vmbus_chan_sched() can find the
> channel relid in recv_int_page set,
>
> 4) vmbus_chan_sched() finds the channel's relid in recv_int_page
> set before it search/load from the channel array (full barrier
> in sync_test_and_clear_bit()).
>
> This is for the "normal"/not resuming from hibernation case; for the
> latter, notice that:
>
> a) vmbus_isr() (and vmbus_chan_sched()) can not run until when
> vmbus_bus_resume() has finished (@resume_noirq callback),
>
> b) vmbus_bus_resume() can not complete before nr_chan_fixup_on_resume
> equals 0 in check_ready_for_resume_event().
>
> (and check_ready_for_resume_event() does also provides a full barrier).
>
> If makes sense to you, I'll try to add some of the above in comments.
>
It does, thank you!
--
Vitaly
^ permalink raw reply
* Re: [PATCH -next] hv_debugfs: Make hv_debug_root static
From: Wei Liu @ 2020-04-04 16:48 UTC (permalink / raw)
To: Michael Kelley
Cc: YueHaibing, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <MW2PR2101MB105222D949FB4944EE647F2FD7C70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Fri, Apr 03, 2020 at 12:21:47PM +0000, Michael Kelley wrote:
> From: YueHaibing <yuehaibing@huawei.com> Sent: Friday, April 3, 2020 1:29 AM
> >
> > Fix sparse warning:
> >
> > drivers/hv/hv_debugfs.c:14:15: warning: symbol 'hv_debug_root' was not declared. Should
> > it be static?
> >
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > ---
> > drivers/hv/hv_debugfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hv/hv_debugfs.c b/drivers/hv/hv_debugfs.c
> > index 8a2878573582..ccf752b6659a 100644
> > --- a/drivers/hv/hv_debugfs.c
> > +++ b/drivers/hv/hv_debugfs.c
> > @@ -11,7 +11,7 @@
> >
> > #include "hyperv_vmbus.h"
> >
> > -struct dentry *hv_debug_root;
> > +static struct dentry *hv_debug_root;
> >
> > static int hv_debugfs_delay_get(void *data, u64 *val)
> > {
> > --
> > 2.17.1
> >
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
Queued. Thanks.
^ permalink raw reply
* [PATCH 00/11] VMBus channel interrupt reassignment
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft), David S. Miller,
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
James E.J. Bottomley, Martin K. Petersen
Hi all,
This is a follow-up on the RFC submission [1]. The series introduces
changes in the VMBus drivers to reassign the CPU that a VMbus channel
will interrupt. This feature can be used for load balancing or other
purposes (e.g. CPU offlining). The submission integrates feedback in
the RFC to amend the handling of the 'array of channels' (patch #3).
Thanks,
Andrea
[1] https://lkml.kernel.org/r/20200325225505.23998-1-parri.andrea@gmail.com
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Andrew Murray <amurray@thegoodpenguin.co.uk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Andrea Parri (Microsoft) (11):
Drivers: hv: vmbus: Always handle the VMBus messages on CPU0
Drivers: hv: vmbus: Don't bind the offer&rescind works to a specific
CPU
Drivers: hv: vmbus: Replace the per-CPU channel lists with a global
array of channels
hv_netvsc: Disable NAPI before closing the VMBus channel
hv_utils: Always execute the fcopy and vss callbacks in a tasklet
Drivers: hv: vmbus: Use a spin lock for synchronizing channel
scheduling vs. channel removal
PCI: hv: Prepare hv_compose_msi_msg() for the
VMBus-channel-interrupt-to-vCPU reassignment functionality
Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
logic
Drivers: hv: vmbus: Synchronize init_vp_index() vs. CPU hotplug
Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL message
type
scsi: storvsc: Re-init stor_chns when a channel interrupt is
re-assigned
drivers/hv/channel.c | 58 +++--
drivers/hv/channel_mgmt.c | 347 +++++++++++++++-------------
drivers/hv/connection.c | 58 +----
drivers/hv/hv.c | 16 +-
drivers/hv/hv_fcopy.c | 2 +-
drivers/hv/hv_snapshot.c | 2 +-
drivers/hv/hv_trace.h | 19 ++
drivers/hv/hyperv_vmbus.h | 32 ++-
drivers/hv/vmbus_drv.c | 241 +++++++++++++++----
drivers/net/hyperv/netvsc.c | 7 +-
drivers/pci/controller/pci-hyperv.c | 44 ++--
drivers/scsi/storvsc_drv.c | 95 +++++++-
include/linux/hyperv.h | 51 ++--
13 files changed, 620 insertions(+), 352 deletions(-)
--
2.24.0
^ permalink raw reply
* [PATCH 01/11] Drivers: hv: vmbus: Always handle the VMBus messages on CPU0
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
A Linux guest have to pick a "connect CPU" to communicate with the
Hyper-V host. This CPU can not be taken offline because Hyper-V does
not provide a way to change that CPU assignment.
Current code sets the connect CPU to whatever CPU ends up running the
function vmbus_negotiate_version(), and this will generate problems if
that CPU is taken offine.
Establish CPU0 as the connect CPU, and add logics to prevents the
connect CPU from being taken offline. We could pick some other CPU,
and we could pick that "other CPU" dynamically if there was a reason to
do so at some point in the future. But for now, #defining the connect
CPU to 0 is the most straightforward and least complex solution.
While on this, add inline comments explaining "why" offer and rescind
messages should not be handled by a same serialized work queue.
Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/connection.c | 20 +-------------------
drivers/hv/hv.c | 7 +++++++
drivers/hv/hyperv_vmbus.h | 11 ++++++-----
drivers/hv/vmbus_drv.c | 20 +++++++++++++++++---
4 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 74e77de89b4f3..f4bd306d2cef9 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -69,7 +69,6 @@ MODULE_PARM_DESC(max_version,
int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
{
int ret = 0;
- unsigned int cur_cpu;
struct vmbus_channel_initiate_contact *msg;
unsigned long flags;
@@ -102,24 +101,7 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]);
msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]);
- /*
- * We want all channel messages to be delivered on CPU 0.
- * This has been the behavior pre-win8. This is not
- * perf issue and having all channel messages delivered on CPU 0
- * would be ok.
- * For post win8 hosts, we support receiving channel messagges on
- * all the CPUs. This is needed for kexec to work correctly where
- * the CPU attempting to connect may not be CPU 0.
- */
- if (version >= VERSION_WIN8_1) {
- cur_cpu = get_cpu();
- msg->target_vcpu = hv_cpu_number_to_vp_number(cur_cpu);
- vmbus_connection.connect_cpu = cur_cpu;
- put_cpu();
- } else {
- msg->target_vcpu = 0;
- vmbus_connection.connect_cpu = 0;
- }
+ msg->target_vcpu = hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
/*
* Add to list before we send the request since we may
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 6098e0cbdb4b0..e2b3310454640 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -249,6 +249,13 @@ int hv_synic_cleanup(unsigned int cpu)
bool channel_found = false;
unsigned long flags;
+ /*
+ * Hyper-V does not provide a way to change the connect CPU once
+ * it is set; we must prevent the connect CPU from going offline.
+ */
+ if (cpu == VMBUS_CONNECT_CPU)
+ return -EBUSY;
+
/*
* Search for channels which are bound to the CPU we're about to
* cleanup. In case we find one and vmbus is still connected we need to
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f5fa3b3c9baf7..ce19a4c583884 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -212,12 +212,13 @@ enum vmbus_connect_state {
#define MAX_SIZE_CHANNEL_MESSAGE HV_MESSAGE_PAYLOAD_BYTE_COUNT
-struct vmbus_connection {
- /*
- * CPU on which the initial host contact was made.
- */
- int connect_cpu;
+/*
+ * The CPU that Hyper-V will interrupt for VMBUS messages, such as
+ * CHANNELMSG_OFFERCHANNEL and CHANNELMSG_RESCIND_CHANNELOFFER.
+ */
+#define VMBUS_CONNECT_CPU 0
+struct vmbus_connection {
u32 msg_conn_id;
atomic_t offer_in_progress;
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 172ceae69abb7..a491d44362f9f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1083,14 +1083,28 @@ void vmbus_on_msg_dpc(unsigned long data)
/*
* If we are handling the rescind message;
* schedule the work on the global work queue.
+ *
+ * The OFFER message and the RESCIND message should
+ * not be handled by the same serialized work queue,
+ * because the OFFER handler may call vmbus_open(),
+ * which tries to open the channel by sending an
+ * OPEN_CHANNEL message to the host and waits for
+ * the host's response; however, if the host has
+ * rescinded the channel before it receives the
+ * OPEN_CHANNEL message, the host just silently
+ * ignores the OPEN_CHANNEL message; as a result,
+ * the guest's OFFER handler hangs for ever, if we
+ * handle the RESCIND message in the same serialized
+ * work queue: the RESCIND handler can not start to
+ * run before the OFFER handler finishes.
*/
- schedule_work_on(vmbus_connection.connect_cpu,
+ schedule_work_on(VMBUS_CONNECT_CPU,
&ctx->work);
break;
case CHANNELMSG_OFFERCHANNEL:
atomic_inc(&vmbus_connection.offer_in_progress);
- queue_work_on(vmbus_connection.connect_cpu,
+ queue_work_on(VMBUS_CONNECT_CPU,
vmbus_connection.work_queue,
&ctx->work);
break;
@@ -1137,7 +1151,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
INIT_WORK(&ctx->work, vmbus_onmessage_work);
- queue_work_on(vmbus_connection.connect_cpu,
+ queue_work_on(VMBUS_CONNECT_CPU,
vmbus_connection.work_queue,
&ctx->work);
}
--
2.24.0
^ permalink raw reply related
* [PATCH 02/11] Drivers: hv: vmbus: Don't bind the offer&rescind works to a specific CPU
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
The offer and rescind works are currently scheduled on the so called
"connect CPU". However, this is not really needed: we can synchronize
the works by relying on the usage of the offer_in_progress counter and
of the channel_mutex mutex. This synchronization is already in place.
So, remove this unnecessary "bind to the connect CPU" constraint and
update the inline comments accordingly.
Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 21 ++++++++++++++++-----
drivers/hv/vmbus_drv.c | 39 ++++++++++++++++++++++++++++-----------
2 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 501c43c5851dc..aed8db59a5ff8 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1028,11 +1028,22 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
* offer comes in first and then the rescind.
* Since we process these events in work elements,
* and with preemption, we may end up processing
- * the events out of order. Given that we handle these
- * work elements on the same CPU, this is possible only
- * in the case of preemption. In any case wait here
- * until the offer processing has moved beyond the
- * point where the channel is discoverable.
+ * the events out of order. We rely on the synchronization
+ * provided by offer_in_progress and by channel_mutex for
+ * ordering these events:
+ *
+ * { Initially: offer_in_progress = 1 }
+ *
+ * CPU1 CPU2
+ *
+ * [vmbus_process_offer()] [vmbus_onoffer_rescind()]
+ *
+ * LOCK channel_mutex WAIT_ON offer_in_progress == 0
+ * DECREMENT offer_in_progress LOCK channel_mutex
+ * INSERT chn_list SEARCH chn_list
+ * UNLOCK channel_mutex UNLOCK channel_mutex
+ *
+ * Forbids: CPU2's SEARCH from *not* seeing CPU1's INSERT
*/
while (atomic_read(&vmbus_connection.offer_in_progress) != 0) {
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index a491d44362f9f..41ec0c95df33f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1075,8 +1075,9 @@ void vmbus_on_msg_dpc(unsigned long data)
/*
* The host can generate a rescind message while we
* may still be handling the original offer. We deal with
- * this condition by ensuring the processing is done on the
- * same CPU.
+ * this condition by relying on the synchronization provided
+ * by offer_in_progress and by channel_mutex. See also the
+ * inline comments in vmbus_onoffer_rescind().
*/
switch (hdr->msgtype) {
case CHANNELMSG_RESCIND_CHANNELOFFER:
@@ -1098,16 +1099,34 @@ void vmbus_on_msg_dpc(unsigned long data)
* work queue: the RESCIND handler can not start to
* run before the OFFER handler finishes.
*/
- schedule_work_on(VMBUS_CONNECT_CPU,
- &ctx->work);
+ schedule_work(&ctx->work);
break;
case CHANNELMSG_OFFERCHANNEL:
+ /*
+ * The host sends the offer message of a given channel
+ * before sending the rescind message of the same
+ * channel. These messages are sent to the guest's
+ * connect CPU; the guest then starts processing them
+ * in the tasklet handler on this CPU:
+ *
+ * VMBUS_CONNECT_CPU
+ *
+ * [vmbus_on_msg_dpc()]
+ * atomic_inc() // CHANNELMSG_OFFERCHANNEL
+ * queue_work()
+ * ...
+ * [vmbus_on_msg_dpc()]
+ * schedule_work() // CHANNELMSG_RESCIND_CHANNELOFFER
+ *
+ * We rely on the memory-ordering properties of the
+ * queue_work() and schedule_work() primitives, which
+ * guarantee that the atomic increment will be visible
+ * to the CPUs which will execute the offer & rescind
+ * works by the time these works will start execution.
+ */
atomic_inc(&vmbus_connection.offer_in_progress);
- queue_work_on(VMBUS_CONNECT_CPU,
- vmbus_connection.work_queue,
- &ctx->work);
- break;
+ fallthrough;
default:
queue_work(vmbus_connection.work_queue, &ctx->work);
@@ -1151,9 +1170,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
INIT_WORK(&ctx->work, vmbus_onmessage_work);
- queue_work_on(VMBUS_CONNECT_CPU,
- vmbus_connection.work_queue,
- &ctx->work);
+ queue_work(vmbus_connection.work_queue, &ctx->work);
}
#endif /* CONFIG_PM_SLEEP */
--
2.24.0
^ permalink raw reply related
* [PATCH 04/11] hv_netvsc: Disable NAPI before closing the VMBus channel
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft), Stephen Hemminger,
David S. Miller, netdev
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
vmbus_chan_sched() might call the netvsc driver callback function that
ends up scheduling NAPI work. This "work" can access the channel ring
buffer, so we must ensure that any such work is completed and that the
ring buffer is no longer being accessed before freeing the ring buffer
data structure in the channel closure path. To this end, disable NAPI
before calling vmbus_close() in netvsc_device_remove().
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: <netdev@vger.kernel.org>
---
drivers/hv/channel.c | 6 ++++++
drivers/net/hyperv/netvsc.c | 7 +++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 23f358cb7f494..256ee90c74460 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -609,6 +609,12 @@ void vmbus_reset_channel_cb(struct vmbus_channel *channel)
* the former is accessing channel->inbound.ring_buffer, the latter
* could be freeing the ring_buffer pages, so here we must stop it
* first.
+ *
+ * vmbus_chan_sched() might call the netvsc driver callback function
+ * that ends up scheduling NAPI work that accesses the ring buffer.
+ * At this point, we have to ensure that any such work is completed
+ * and that the channel ring buffer is no longer being accessed, cf.
+ * the calls to napi_disable() in netvsc_device_remove().
*/
tasklet_disable(&channel->callback_event);
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1b320bcf150a4..806cc85d10033 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -635,9 +635,12 @@ void netvsc_device_remove(struct hv_device *device)
RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
- /* And disassociate NAPI context from device */
- for (i = 0; i < net_device->num_chn; i++)
+ /* Disable NAPI and disassociate its context from the device. */
+ for (i = 0; i < net_device->num_chn; i++) {
+ /* See also vmbus_reset_channel_cb(). */
+ napi_disable(&net_device->chan_table[i].napi);
netif_napi_del(&net_device->chan_table[i].napi);
+ }
/*
* At this point, no one should be accessing net_device
--
2.24.0
^ permalink raw reply related
* [PATCH 05/11] hv_utils: Always execute the fcopy and vss callbacks in a tasklet
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
The fcopy and vss callback functions could be running in a tasklet
at the same time they are called in hv_poll_channel(). Current code
serializes the invocations of these functions, and their accesses to
the channel ring buffer, by sending an IPI to the CPU that is allowed
to access the ring buffer, cf. hv_poll_channel(). This IPI mechanism
becomes infeasible if we allow changing the CPU that a channel will
interrupt. Instead modify the callback wrappers to always execute
the fcopy and vss callbacks in a tasklet, thus mirroring the solution
for the kvp callback functions adopted since commit a3ade8cc474d8
("HV: properly delay KVP packets when negotiation is in progress").
This will ensure that the callback function can't run on two CPUs at
the same time.
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/hv_fcopy.c | 2 +-
drivers/hv/hv_snapshot.c | 2 +-
drivers/hv/hyperv_vmbus.h | 7 +------
3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index bb9ba3f7c7949..5040d7e0cd9e9 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -71,7 +71,7 @@ static void fcopy_poll_wrapper(void *channel)
{
/* Transaction is finished, reset the state here to avoid races. */
fcopy_transaction.state = HVUTIL_READY;
- hv_fcopy_onchannelcallback(channel);
+ tasklet_schedule(&((struct vmbus_channel *)channel)->callback_event);
}
static void fcopy_timeout_func(struct work_struct *dummy)
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 1c75b38f0d6da..783779e4cc1a5 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -80,7 +80,7 @@ static void vss_poll_wrapper(void *channel)
{
/* Transaction is finished, reset the state here to avoid races. */
vss_transaction.state = HVUTIL_READY;
- hv_vss_onchannelcallback(channel);
+ tasklet_schedule(&((struct vmbus_channel *)channel)->callback_event);
}
/*
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 41b7267da529a..8df138f98e9e5 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -377,12 +377,7 @@ static inline void hv_poll_channel(struct vmbus_channel *channel,
{
if (!channel)
return;
-
- if (in_interrupt() && (channel->target_cpu == smp_processor_id())) {
- cb(channel);
- return;
- }
- smp_call_function_single(channel->target_cpu, cb, channel, true);
+ cb(channel);
}
enum hvutil_device_state {
--
2.24.0
^ permalink raw reply related
* [PATCH 07/11] PCI: hv: Prepare hv_compose_msi_msg() for the VMBus-channel-interrupt-to-vCPU reassignment functionality
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft), Lorenzo Pieralisi,
Andrew Murray, Bjorn Helgaas, linux-pci
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
The current implementation of hv_compose_msi_msg() is incompatible with
the new functionality that allows changing the vCPU a VMBus channel will
interrupt: if this function always calls hv_pci_onchannelcallback() in
the polling loop, the interrupt going to a different CPU could cause
hv_pci_onchannelcallback() to be running simultaneously in a tasklet,
which will break. The current code also has a problem in that it is not
synchronized with vmbus_reset_channel_cb(): hv_compose_msi_msg() could
be accessing the ring buffer via the call of hv_pci_onchannelcallback()
well after the time that vmbus_reset_channel_cb() has finished.
Fix these issues as follows. Disable the channel tasklet before
entering the polling loop in hv_compose_msi_msg() and re-enable it when
done. This will prevent hv_pci_onchannelcallback() from running in a
tasklet on a different CPU. Moreover, poll by always calling
hv_pci_onchannelcallback(), but check the channel callback function for
NULL and invoke the callback within a sched_lock critical section. This
will prevent hv_compose_msi_msg() from accessing the ring buffer after
vmbus_reset_channel_cb() has acquired the sched_lock spinlock.
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Andrew Murray <amurray@thegoodpenguin.co.uk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: <linux-pci@vger.kernel.org>
---
drivers/pci/controller/pci-hyperv.c | 44 ++++++++++++++++++-----------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 9977abff92fc5..e6020480a28b1 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1350,11 +1350,11 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
{
struct irq_cfg *cfg = irqd_cfg(data);
struct hv_pcibus_device *hbus;
+ struct vmbus_channel *channel;
struct hv_pci_dev *hpdev;
struct pci_bus *pbus;
struct pci_dev *pdev;
struct cpumask *dest;
- unsigned long flags;
struct compose_comp_ctxt comp;
struct tran_int_desc *int_desc;
struct {
@@ -1372,6 +1372,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
dest = irq_data_get_effective_affinity_mask(data);
pbus = pdev->bus;
hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
+ channel = hbus->hdev->channel;
hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn));
if (!hpdev)
goto return_null_message;
@@ -1428,43 +1429,52 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
goto free_int_desc;
}
+ /*
+ * Prevents hv_pci_onchannelcallback() from running concurrently
+ * in the tasklet.
+ */
+ tasklet_disable(&channel->callback_event);
+
/*
* Since this function is called with IRQ locks held, can't
* do normal wait for completion; instead poll.
*/
while (!try_wait_for_completion(&comp.comp_pkt.host_event)) {
+ unsigned long flags;
+
/* 0xFFFF means an invalid PCI VENDOR ID. */
if (hv_pcifront_get_vendor_id(hpdev) == 0xFFFF) {
dev_err_once(&hbus->hdev->device,
"the device has gone\n");
- goto free_int_desc;
+ goto enable_tasklet;
}
/*
- * When the higher level interrupt code calls us with
- * interrupt disabled, we must poll the channel by calling
- * the channel callback directly when channel->target_cpu is
- * the current CPU. When the higher level interrupt code
- * calls us with interrupt enabled, let's add the
- * local_irq_save()/restore() to avoid race:
- * hv_pci_onchannelcallback() can also run in tasklet.
+ * Make sure that the ring buffer data structure doesn't get
+ * freed while we dereference the ring buffer pointer. Test
+ * for the channel's onchannel_callback being NULL within a
+ * sched_lock critical section. See also the inline comments
+ * in vmbus_reset_channel_cb().
*/
- local_irq_save(flags);
-
- if (hbus->hdev->channel->target_cpu == smp_processor_id())
- hv_pci_onchannelcallback(hbus);
-
- local_irq_restore(flags);
+ spin_lock_irqsave(&channel->sched_lock, flags);
+ if (unlikely(channel->onchannel_callback == NULL)) {
+ spin_unlock_irqrestore(&channel->sched_lock, flags);
+ goto enable_tasklet;
+ }
+ hv_pci_onchannelcallback(hbus);
+ spin_unlock_irqrestore(&channel->sched_lock, flags);
if (hpdev->state == hv_pcichild_ejecting) {
dev_err_once(&hbus->hdev->device,
"the device is being ejected\n");
- goto free_int_desc;
+ goto enable_tasklet;
}
udelay(100);
}
+ tasklet_enable(&channel->callback_event);
+
if (comp.comp_pkt.completion_status < 0) {
dev_err(&hbus->hdev->device,
"Request for interrupt failed: 0x%x",
@@ -1488,6 +1498,8 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
put_pcichild(hpdev);
return;
+enable_tasklet:
+ tasklet_enable(&channel->callback_event);
free_int_desc:
kfree(int_desc);
drop_reference:
--
2.24.0
^ permalink raw reply related
* [PATCH 10/11] Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL message type
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
VMBus version 4.1 and later support the CHANNELMSG_MODIFYCHANNEL(22)
message type which can be used to request Hyper-V to change the vCPU
that a channel will interrupt.
Introduce the CHANNELMSG_MODIFYCHANNEL message type, and define the
vmbus_send_modifychannel() function to send CHANNELMSG_MODIFYCHANNEL
requests to the host via a hypercall. The function is then used to
define a sysfs "store" operation, which allows to change the (v)CPU
the channel will interrupt by using the sysfs interface. The feature
can be used for load balancing or other purposes.
One interesting catch here is that Hyper-V can *not* currently ACK
CHANNELMSG_MODIFYCHANNEL messages with the promise that (after the ACK
is sent) the channel won't send any more interrupts to the "old" CPU.
The peculiarity of the CHANNELMSG_MODIFYCHANNEL messages is problematic
if the user want to take a CPU offline, since we don't want to take a
CPU offline (and, potentially, "lose" channel interrupts on such CPU)
if the host is still processing a CHANNELMSG_MODIFYCHANNEL message
associated to that CPU.
It is worth mentioning, however, that we have been unable to observe
the above mentioned "race": in all our tests, CHANNELMSG_MODIFYCHANNEL
requests appeared *as if* they were processed synchronously by the host.
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel.c | 28 ++++++++++
drivers/hv/channel_mgmt.c | 2 +-
drivers/hv/hv_trace.h | 19 +++++++
drivers/hv/vmbus_drv.c | 108 +++++++++++++++++++++++++++++++++++++-
include/linux/hyperv.h | 10 +++-
5 files changed, 163 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 132e476f87b2e..90070b337c10d 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -289,6 +289,34 @@ int vmbus_send_tl_connect_request(const guid_t *shv_guest_servie_id,
}
EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
+/*
+ * Set/change the vCPU (@target_vp) the channel (@child_relid) will interrupt.
+ *
+ * CHANNELMSG_MODIFYCHANNEL messages are aynchronous. Also, Hyper-V does not
+ * ACK such messages. IOW we can't know when the host will stop interrupting
+ * the "old" vCPU and start interrupting the "new" vCPU for the given channel.
+ *
+ * The CHANNELMSG_MODIFYCHANNEL message type is supported since VMBus version
+ * VERSION_WIN10_V4_1.
+ */
+int vmbus_send_modifychannel(u32 child_relid, u32 target_vp)
+{
+ struct vmbus_channel_modifychannel conn_msg;
+ int ret;
+
+ memset(&conn_msg, 0, sizeof(conn_msg));
+ conn_msg.header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ conn_msg.child_relid = child_relid;
+ conn_msg.target_vp = target_vp;
+
+ ret = vmbus_post_msg(&conn_msg, sizeof(conn_msg), true);
+
+ trace_vmbus_send_modifychannel(&conn_msg, ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vmbus_send_modifychannel);
+
/*
* create_gpadl_header - Creates a gpadl for the specified buffer
*/
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 3785beead503d..1058c814ab06e 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1377,7 +1377,7 @@ channel_message_table[CHANNELMSG_COUNT] = {
{ CHANNELMSG_19, 0, NULL },
{ CHANNELMSG_20, 0, NULL },
{ CHANNELMSG_TL_CONNECT_REQUEST, 0, NULL },
- { CHANNELMSG_22, 0, NULL },
+ { CHANNELMSG_MODIFYCHANNEL, 0, NULL },
{ CHANNELMSG_TL_CONNECT_RESULT, 0, NULL },
};
diff --git a/drivers/hv/hv_trace.h b/drivers/hv/hv_trace.h
index e70783e33680f..a43bc76c2d5d0 100644
--- a/drivers/hv/hv_trace.h
+++ b/drivers/hv/hv_trace.h
@@ -296,6 +296,25 @@ TRACE_EVENT(vmbus_send_tl_connect_request,
)
);
+TRACE_EVENT(vmbus_send_modifychannel,
+ TP_PROTO(const struct vmbus_channel_modifychannel *msg,
+ int ret),
+ TP_ARGS(msg, ret),
+ TP_STRUCT__entry(
+ __field(u32, child_relid)
+ __field(u32, target_vp)
+ __field(int, ret)
+ ),
+ TP_fast_assign(
+ __entry->child_relid = msg->child_relid;
+ __entry->target_vp = msg->target_vp;
+ __entry->ret = ret;
+ ),
+ TP_printk("binding child_relid 0x%x to target_vp 0x%x, ret %d",
+ __entry->child_relid, __entry->target_vp, __entry->ret
+ )
+ );
+
DECLARE_EVENT_CLASS(vmbus_channel,
TP_PROTO(const struct vmbus_channel *channel),
TP_ARGS(channel),
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 18bd1c0e09a83..0f204640c50c2 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1579,8 +1579,24 @@ static ssize_t vmbus_chan_attr_show(struct kobject *kobj,
return attribute->show(chan, buf);
}
+static ssize_t vmbus_chan_attr_store(struct kobject *kobj,
+ struct attribute *attr, const char *buf,
+ size_t count)
+{
+ const struct vmbus_chan_attribute *attribute
+ = container_of(attr, struct vmbus_chan_attribute, attr);
+ struct vmbus_channel *chan
+ = container_of(kobj, struct vmbus_channel, kobj);
+
+ if (!attribute->store)
+ return -EIO;
+
+ return attribute->store(chan, buf, count);
+}
+
static const struct sysfs_ops vmbus_chan_sysfs_ops = {
.show = vmbus_chan_attr_show,
+ .store = vmbus_chan_attr_store,
};
static ssize_t out_mask_show(struct vmbus_channel *channel, char *buf)
@@ -1651,11 +1667,99 @@ static ssize_t write_avail_show(struct vmbus_channel *channel, char *buf)
}
static VMBUS_CHAN_ATTR_RO(write_avail);
-static ssize_t show_target_cpu(struct vmbus_channel *channel, char *buf)
+static ssize_t target_cpu_show(struct vmbus_channel *channel, char *buf)
{
return sprintf(buf, "%u\n", channel->target_cpu);
}
-static VMBUS_CHAN_ATTR(cpu, S_IRUGO, show_target_cpu, NULL);
+static ssize_t target_cpu_store(struct vmbus_channel *channel,
+ const char *buf, size_t count)
+{
+ ssize_t ret = count;
+ u32 target_cpu;
+
+ if (vmbus_proto_version < VERSION_WIN10_V4_1)
+ return -EIO;
+
+ if (sscanf(buf, "%uu", &target_cpu) != 1)
+ return -EIO;
+
+ /* Validate target_cpu for the cpumask_test_cpu() operation below. */
+ if (target_cpu >= nr_cpumask_bits)
+ return -EINVAL;
+
+ /* No CPUs should come up or down during this. */
+ cpus_read_lock();
+
+ if (!cpumask_test_cpu(target_cpu, cpu_online_mask)) {
+ cpus_read_unlock();
+ return -EINVAL;
+ }
+
+ /*
+ * Synchronizes target_cpu_store() and channel closure:
+ *
+ * { Initially: state = CHANNEL_OPENED }
+ *
+ * CPU1 CPU2
+ *
+ * [target_cpu_store()] [vmbus_disconnect_ring()]
+ *
+ * LOCK channel_mutex LOCK channel_mutex
+ * LOAD r1 = state LOAD r2 = state
+ * IF (r1 == CHANNEL_OPENED) IF (r2 == CHANNEL_OPENED)
+ * SEND MODIFYCHANNEL STORE state = CHANNEL_OPEN
+ * [...] SEND CLOSECHANNEL
+ * UNLOCK channel_mutex UNLOCK channel_mutex
+ *
+ * Forbids: r1 == r2 == CHANNEL_OPENED (i.e., CPU1's LOCK precedes
+ * CPU2's LOCK) && CPU2's SEND precedes CPU1's SEND
+ *
+ * Note. The host processes the channel messages "sequentially", in
+ * the order in which they are received on a per-partition basis.
+ */
+ mutex_lock(&vmbus_connection.channel_mutex);
+
+ /*
+ * Hyper-V will ignore MODIFYCHANNEL messages for "non-open" channels;
+ * avoid sending the message and fail here for such channels.
+ */
+ if (channel->state != CHANNEL_OPENED_STATE) {
+ ret = -EIO;
+ goto cpu_store_unlock;
+ }
+
+ if (channel->target_cpu == target_cpu)
+ goto cpu_store_unlock;
+
+ if (vmbus_send_modifychannel(channel->offermsg.child_relid,
+ hv_cpu_number_to_vp_number(target_cpu))) {
+ ret = -EIO;
+ goto cpu_store_unlock;
+ }
+
+ /*
+ * Warning. At this point, there is *no* guarantee that the host will
+ * have successfully processed the vmbus_send_modifychannel() request.
+ * See the header comment of vmbus_send_modifychannel() for more info.
+ *
+ * Lags in the processing of the above vmbus_send_modifychannel() can
+ * result in missed interrupts if the "old" target CPU is taken offline
+ * before Hyper-V starts sending interrupts to the "new" target CPU.
+ * But apart from this offlining scenario, the code tolerates such
+ * lags. It will function correctly even if a channel interrupt comes
+ * in on a CPU that is different from the channel target_cpu value.
+ */
+
+ channel->target_cpu = target_cpu;
+ channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
+ channel->numa_node = cpu_to_node(target_cpu);
+
+cpu_store_unlock:
+ mutex_unlock(&vmbus_connection.channel_mutex);
+ cpus_read_unlock();
+ return ret;
+}
+static VMBUS_CHAN_ATTR(cpu, 0644, target_cpu_show, target_cpu_store);
static ssize_t channel_pending_show(struct vmbus_channel *channel,
char *buf)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index f8e7c22d41a1a..edfcd42319ef3 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -425,7 +425,7 @@ enum vmbus_channel_message_type {
CHANNELMSG_19 = 19,
CHANNELMSG_20 = 20,
CHANNELMSG_TL_CONNECT_REQUEST = 21,
- CHANNELMSG_22 = 22,
+ CHANNELMSG_MODIFYCHANNEL = 22,
CHANNELMSG_TL_CONNECT_RESULT = 23,
CHANNELMSG_COUNT
};
@@ -620,6 +620,13 @@ struct vmbus_channel_tl_connect_request {
guid_t host_service_id;
} __packed;
+/* Modify Channel parameters, cf. vmbus_send_modifychannel() */
+struct vmbus_channel_modifychannel {
+ struct vmbus_channel_message_header header;
+ u32 child_relid;
+ u32 target_vp;
+} __packed;
+
struct vmbus_channel_version_response {
struct vmbus_channel_message_header header;
u8 version_supported;
@@ -1505,6 +1512,7 @@ extern __u32 vmbus_proto_version;
int vmbus_send_tl_connect_request(const guid_t *shv_guest_servie_id,
const guid_t *shv_host_servie_id);
+int vmbus_send_modifychannel(u32 child_relid, u32 target_vp);
void vmbus_set_event(struct vmbus_channel *channel);
/* Get the start of the ring buffer. */
--
2.24.0
^ permalink raw reply related
* [PATCH 08/11] Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity logic
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
The logic is unused since commit 509879bdb30b8 ("Drivers: hv: Introduce
a policy for controlling channel affinity").
This logic assumes that a channel target_cpu doesn't change during the
lifetime of a channel, but this assumption is incompatible with the new
functionality that allows changing the vCPU a channel will interrupt.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 105 +++++++++-----------------------------
include/linux/hyperv.h | 27 ----------
2 files changed, 25 insertions(+), 107 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 46ea978e4222a..9041e5950e367 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -433,14 +433,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
- /*
- * We need to free the bit for init_vp_index() to work in the case
- * of sub-channel, when we reload drivers like hv_netvsc.
- */
- if (channel->affinity_policy == HV_LOCALIZED)
- cpumask_clear_cpu(channel->target_cpu,
- &primary_channel->alloced_cpus_in_node);
-
/*
* Upon suspend, an in-use hv_sock channel is marked as "rescinded" and
* the relid is invalidated; after hibernation, when the user-space app
@@ -662,20 +654,21 @@ static DEFINE_SPINLOCK(bind_channel_to_cpu_lock);
/*
* Starting with Win8, we can statically distribute the incoming
* channel interrupt load by binding a channel to VCPU.
- * We distribute the interrupt loads to one or more NUMA nodes based on
- * the channel's affinity_policy.
*
* For pre-win8 hosts or non-performance critical channels we assign the
* first CPU in the first NUMA node.
+ *
+ * Starting with win8, performance critical channels will be distributed
+ * evenly among all the available NUMA nodes. Once the node is assigned,
+ * we will assign the CPU based on a simple round robin scheme.
*/
static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
{
- u32 cur_cpu;
bool perf_chn = vmbus_devs[dev_type].perf_device;
- struct vmbus_channel *primary = channel->primary_channel;
- int next_node;
cpumask_var_t available_mask;
struct cpumask *alloced_mask;
+ u32 target_cpu;
+ int numa_node;
if ((vmbus_proto_version == VERSION_WS2008) ||
(vmbus_proto_version == VERSION_WIN7) || (!perf_chn) ||
@@ -693,31 +686,27 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
return;
}
- spin_lock(&bind_channel_to_cpu_lock);
-
/*
- * Based on the channel affinity policy, we will assign the NUMA
- * nodes.
+ * Serializes the accesses to the global variable next_numa_node_id.
+ * See also the header comment of the spin lock declaration.
*/
+ spin_lock(&bind_channel_to_cpu_lock);
- if ((channel->affinity_policy == HV_BALANCED) || (!primary)) {
- while (true) {
- next_node = next_numa_node_id++;
- if (next_node == nr_node_ids) {
- next_node = next_numa_node_id = 0;
- continue;
- }
- if (cpumask_empty(cpumask_of_node(next_node)))
- continue;
- break;
+ while (true) {
+ numa_node = next_numa_node_id++;
+ if (numa_node == nr_node_ids) {
+ next_numa_node_id = 0;
+ continue;
}
- channel->numa_node = next_node;
- primary = channel;
+ if (cpumask_empty(cpumask_of_node(numa_node)))
+ continue;
+ break;
}
- alloced_mask = &hv_context.hv_numa_map[primary->numa_node];
+ channel->numa_node = numa_node;
+ alloced_mask = &hv_context.hv_numa_map[numa_node];
if (cpumask_weight(alloced_mask) ==
- cpumask_weight(cpumask_of_node(primary->numa_node))) {
+ cpumask_weight(cpumask_of_node(numa_node))) {
/*
* We have cycled through all the CPUs in the node;
* reset the alloced map.
@@ -725,57 +714,13 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
cpumask_clear(alloced_mask);
}
- cpumask_xor(available_mask, alloced_mask,
- cpumask_of_node(primary->numa_node));
+ cpumask_xor(available_mask, alloced_mask, cpumask_of_node(numa_node));
- cur_cpu = -1;
-
- if (primary->affinity_policy == HV_LOCALIZED) {
- /*
- * Normally Hyper-V host doesn't create more subchannels
- * than there are VCPUs on the node but it is possible when not
- * all present VCPUs on the node are initialized by guest.
- * Clear the alloced_cpus_in_node to start over.
- */
- if (cpumask_equal(&primary->alloced_cpus_in_node,
- cpumask_of_node(primary->numa_node)))
- cpumask_clear(&primary->alloced_cpus_in_node);
- }
-
- while (true) {
- cur_cpu = cpumask_next(cur_cpu, available_mask);
- if (cur_cpu >= nr_cpu_ids) {
- cur_cpu = -1;
- cpumask_copy(available_mask,
- cpumask_of_node(primary->numa_node));
- continue;
- }
-
- if (primary->affinity_policy == HV_LOCALIZED) {
- /*
- * NOTE: in the case of sub-channel, we clear the
- * sub-channel related bit(s) in
- * primary->alloced_cpus_in_node in
- * hv_process_channel_removal(), so when we
- * reload drivers like hv_netvsc in SMP guest, here
- * we're able to re-allocate
- * bit from primary->alloced_cpus_in_node.
- */
- if (!cpumask_test_cpu(cur_cpu,
- &primary->alloced_cpus_in_node)) {
- cpumask_set_cpu(cur_cpu,
- &primary->alloced_cpus_in_node);
- cpumask_set_cpu(cur_cpu, alloced_mask);
- break;
- }
- } else {
- cpumask_set_cpu(cur_cpu, alloced_mask);
- break;
- }
- }
+ target_cpu = cpumask_first(available_mask);
+ cpumask_set_cpu(target_cpu, alloced_mask);
- channel->target_cpu = cur_cpu;
- channel->target_vp = hv_cpu_number_to_vp_number(cur_cpu);
+ channel->target_cpu = target_cpu;
+ channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
spin_unlock(&bind_channel_to_cpu_lock);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index ce32ab186192f..f8e7c22d41a1a 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -689,11 +689,6 @@ union hv_connection_id {
} u;
};
-enum hv_numa_policy {
- HV_BALANCED = 0,
- HV_LOCALIZED,
-};
-
enum vmbus_device_type {
HV_IDE = 0,
HV_SCSI,
@@ -808,10 +803,6 @@ struct vmbus_channel {
u32 target_vp;
/* The corresponding CPUID in the guest */
u32 target_cpu;
- /*
- * State to manage the CPU affiliation of channels.
- */
- struct cpumask alloced_cpus_in_node;
int numa_node;
/*
* Support for sub-channels. For high performance devices,
@@ -898,18 +889,6 @@ struct vmbus_channel {
*/
bool low_latency;
- /*
- * NUMA distribution policy:
- * We support two policies:
- * 1) Balanced: Here all performance critical channels are
- * distributed evenly amongst all the NUMA nodes.
- * This policy will be the default policy.
- * 2) Localized: All channels of a given instance of a
- * performance critical service will be assigned CPUs
- * within a selected NUMA node.
- */
- enum hv_numa_policy affinity_policy;
-
bool probe_done;
/*
@@ -965,12 +944,6 @@ static inline bool is_sub_channel(const struct vmbus_channel *c)
return c->offermsg.offer.sub_channel_index != 0;
}
-static inline void set_channel_affinity_state(struct vmbus_channel *c,
- enum hv_numa_policy policy)
-{
- c->affinity_policy = policy;
-}
-
static inline void set_channel_read_mode(struct vmbus_channel *c,
enum hv_callback_mode mode)
{
--
2.24.0
^ permalink raw reply related
* [PATCH 09/11] Drivers: hv: vmbus: Synchronize init_vp_index() vs. CPU hotplug
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
init_vp_index() may access the cpu_online_mask mask via its calls of
cpumask_of_node(). Make sure to protect these accesses with a
cpus_read_lock() critical section.
Also, remove some (hardcoded) instances of CPU(0) from init_vp_index()
and replace them with VMBUS_CONNECT_CPU. The connect CPU can not go
offline, since Hyper-V does not provide a way to change it.
Finally, order the accesses of target_cpu from init_vp_index() and
hv_synic_cleanup() by relying on the channel_mutex; this is achieved
by moving the call of init_vp_index() into vmbus_process_offer().
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 47 ++++++++++++++++++++++++++++-----------
drivers/hv/hv.c | 7 +++---
2 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 9041e5950e367..3785beead503d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/completion.h>
#include <linux/delay.h>
+#include <linux/cpu.h>
#include <linux/hyperv.h>
#include <asm/mshyperv.h>
@@ -466,13 +467,8 @@ static void vmbus_add_channel_work(struct work_struct *work)
container_of(work, struct vmbus_channel, add_channel_work);
struct vmbus_channel *primary_channel = newchannel->primary_channel;
unsigned long flags;
- u16 dev_type;
int ret;
- dev_type = hv_get_dev_type(newchannel);
-
- init_vp_index(newchannel, dev_type);
-
/*
* This state is used to indicate a successful open
* so that when we do close the channel normally, we
@@ -504,7 +500,7 @@ static void vmbus_add_channel_work(struct work_struct *work)
if (!newchannel->device_obj)
goto err_deq_chan;
- newchannel->device_obj->device_id = dev_type;
+ newchannel->device_obj->device_id = hv_get_dev_type(newchannel);
/*
* Add the new device to the bus. This will kick off device-driver
* binding which eventually invokes the device driver's AddDevice()
@@ -560,6 +556,25 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
unsigned long flags;
bool fnew = true;
+ /*
+ * Initialize the target_CPU before inserting the channel in
+ * the chn_list and sc_list lists, within the channel_mutex
+ * critical section:
+ *
+ * CPU1 CPU2
+ *
+ * [vmbus_process_offer()] [hv_syninc_cleanup()]
+ *
+ * STORE target_cpu LOCK channel_mutex
+ * LOCK channel_mutex SEARCH chn_list
+ * INSERT chn_list LOAD target_cpu
+ * UNLOCK channel_mutex UNLOCK channel_mutex
+ *
+ * Forbids: CPU2's SEARCH from seeing CPU1's INSERT &&
+ * CPU2's LOAD from *not* seing CPU1's STORE
+ */
+ init_vp_index(newchannel, hv_get_dev_type(newchannel));
+
mutex_lock(&vmbus_connection.channel_mutex);
/* Remember the channels that should be cleaned up upon suspend. */
@@ -656,7 +671,7 @@ static DEFINE_SPINLOCK(bind_channel_to_cpu_lock);
* channel interrupt load by binding a channel to VCPU.
*
* For pre-win8 hosts or non-performance critical channels we assign the
- * first CPU in the first NUMA node.
+ * VMBUS_CONNECT_CPU.
*
* Starting with win8, performance critical channels will be distributed
* evenly among all the available NUMA nodes. Once the node is assigned,
@@ -675,17 +690,22 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
!alloc_cpumask_var(&available_mask, GFP_KERNEL)) {
/*
* Prior to win8, all channel interrupts are
- * delivered on cpu 0.
+ * delivered on VMBUS_CONNECT_CPU.
* Also if the channel is not a performance critical
- * channel, bind it to cpu 0.
- * In case alloc_cpumask_var() fails, bind it to cpu 0.
+ * channel, bind it to VMBUS_CONNECT_CPU.
+ * In case alloc_cpumask_var() fails, bind it to
+ * VMBUS_CONNECT_CPU.
*/
- channel->numa_node = 0;
- channel->target_cpu = 0;
- channel->target_vp = hv_cpu_number_to_vp_number(0);
+ channel->numa_node = cpu_to_node(VMBUS_CONNECT_CPU);
+ channel->target_cpu = VMBUS_CONNECT_CPU;
+ channel->target_vp =
+ hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
return;
}
+ /* No CPUs can come up or down during this. */
+ cpus_read_lock();
+
/*
* Serializes the accesses to the global variable next_numa_node_id.
* See also the header comment of the spin lock declaration.
@@ -723,6 +743,7 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
spin_unlock(&bind_channel_to_cpu_lock);
+ cpus_read_unlock();
free_cpumask_var(available_mask);
}
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 17bf1f229152b..188b42b07f07b 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -256,9 +256,10 @@ int hv_synic_cleanup(unsigned int cpu)
/*
* Search for channels which are bound to the CPU we're about to
- * cleanup. In case we find one and vmbus is still connected we need to
- * fail, this will effectively prevent CPU offlining. There is no way
- * we can re-bind channels to different CPUs for now.
+ * cleanup. In case we find one and vmbus is still connected, we
+ * fail; this will effectively prevent CPU offlining.
+ *
+ * TODO: Re-bind the channels to different CPUs.
*/
mutex_lock(&vmbus_connection.channel_mutex);
list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
--
2.24.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox