Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Andrea Parri <parri.andrea@gmail.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dexuan Cui <decui@microsoft.com>,
	"K . Y . 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,
	Michael Kelley <mikelley@microsoft.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 01/11] Drivers: hv: vmbus: Always handle the VMBus messages on CPU0
Date: Sat, 28 Mar 2020 19:50:35 +0100
Message-ID: <20200328185035.GB12184@andrea> (raw)
In-Reply-To: <874kub5i02.fsf@vitty.brq.redhat.com>

On Thu, Mar 26, 2020 at 03:05:17PM +0100, Vitaly Kuznetsov wrote:
> "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> writes:
> 
> > 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>
> > ---
> >  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 70b30e223a578..67fb1edcbf527 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 029378c27421d..7600615e13754 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1056,14 +1056,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;
> > @@ -1110,7 +1124,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);
> >  }
> 
> I tried to refresh my memory on why 'connect_cpu' was introduced and it
> all comes down to the following commit:
> 
> commit 7268644734f6a300353a4c4ff8bf3e013ba80f89
> Author: Alex Ng <alexng@microsoft.com>
> Date:   Fri Feb 26 15:13:22 2016 -0800
> 
>     Drivers: hv: vmbus: Support kexec on ws2012 r2 and above
> 
> which for some unknown reason kept hardcoding '0' for pre-win2012-r2 (
> hv_context.vp_index[smp_processor_id()] in all cases would do exactly
> the same I guess ). Later, 'connect_cpu' appeared just to remember our
> choice, I can't see why we didn't go with CPU0 for simplicity.
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Applied!  Thanks again for the review and all the suggestions,

  Andrea

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 22:54 [RFC PATCH 00/11] VMBus channel interrupt reassignment Andrea Parri (Microsoft)
2020-03-25 22:54 ` [RFC PATCH 01/11] Drivers: hv: vmbus: Always handle the VMBus messages on CPU0 Andrea Parri (Microsoft)
2020-03-26 14:05   ` Vitaly Kuznetsov
2020-03-28 18:50     ` Andrea Parri [this message]
2020-03-25 22:54 ` [RFC PATCH 02/11] Drivers: hv: vmbus: Don't bind the offer&rescind works to a specific CPU Andrea Parri (Microsoft)
2020-03-26 14:16   ` Vitaly Kuznetsov
2020-03-26 15:47     ` Andrea Parri
2020-03-26 17:26       ` Vitaly Kuznetsov
2020-03-28 17:08         ` Andrea Parri
2020-03-29  3:43           ` Michael Kelley
2020-03-30 12:24             ` Vitaly Kuznetsov
2020-04-03 12:04               ` Andrea Parri
2020-03-25 22:54 ` [RFC PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels Andrea Parri (Microsoft)
2020-03-26 14:31   ` Vitaly Kuznetsov
2020-03-26 17:05     ` Andrea Parri
2020-03-26 17:43       ` Vitaly Kuznetsov
2020-03-28 18:21         ` Andrea Parri
2020-03-29  3:49           ` Michael Kelley
2020-03-30 12:45           ` Vitaly Kuznetsov
2020-04-03 13:38             ` Andrea Parri
2020-04-03 14:56               ` Vitaly Kuznetsov
2020-03-25 22:54 ` [RFC PATCH 04/11] hv_netvsc: Disable NAPI before closing the VMBus channel Andrea Parri (Microsoft)
2020-03-26 15:26   ` Stephen Hemminger
2020-03-26 17:55     ` Andrea Parri
2020-03-25 22:54 ` [RFC PATCH 05/11] hv_utils: Always execute the fcopy and vss callbacks in a tasklet Andrea Parri (Microsoft)
2020-03-25 22:55 ` [RFC PATCH 06/11] Drivers: hv: vmbus: Use a spin lock for synchronizing channel scheduling vs. channel removal Andrea Parri (Microsoft)
2020-03-25 22:55 ` [RFC PATCH 07/11] PCI: hv: Prepare hv_compose_msi_msg() for the VMBus-channel-interrupt-to-vCPU reassignment functionality Andrea Parri (Microsoft)
2020-03-25 22:55 ` [RFC PATCH 08/11] Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity logic Andrea Parri (Microsoft)
2020-03-25 22:55 ` [RFC PATCH 09/11] Drivers: hv: vmbus: Synchronize init_vp_index() vs. CPU hotplug Andrea Parri (Microsoft)
2020-03-25 22:55 ` [RFC PATCH 10/11] Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL message type Andrea Parri (Microsoft)
2020-03-26 14:46   ` Vitaly Kuznetsov
2020-03-28 18:48     ` Andrea Parri
2020-04-03 14:55       ` Andrea Parri
2020-03-25 22:55 ` [RFC PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned Andrea Parri (Microsoft)
2020-03-30 16:42   ` Michael Kelley
2020-03-30 18:55     ` Andrea Parri
2020-03-30 19:49       ` Michael Kelley
2020-04-03 13:41         ` Andrea Parri

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200328185035.GB12184@andrea \
    --to=parri.andrea@gmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=decui@microsoft.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=sthemmin@microsoft.com \
    --cc=vkuznets@redhat.com \
    --cc=wei.liu@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git