Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Michael Kelley <mikelley@microsoft.com>
To: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>
Subject: RE: [PATCH v2 3/3] Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline
Date: Wed, 14 Apr 2021 18:43:19 +0000
Message-ID: <MWHPR21MB15931F523196BCE9E23293E4D74E9@MWHPR21MB1593.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20210414150118.2843-4-parri.andrea@gmail.com>

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 14, 2021 8:01 AM
> 
> Check that enough time has passed such that the modify channel message
> has been processed before taking a CPU offline.
> 
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  drivers/hv/hv.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 3e6ff83adff42..dc9aa1130b22f 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -15,6 +15,7 @@
>  #include <linux/hyperv.h>
>  #include <linux/random.h>
>  #include <linux/clockchips.h>
> +#include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <clocksource/hyperv_timer.h>
>  #include <asm/mshyperv.h>
> @@ -292,6 +293,41 @@ void hv_synic_disable_regs(unsigned int cpu)
>  		disable_percpu_irq(vmbus_irq);
>  }
> 
> +#define HV_MAX_TRIES 3
> +/*
> + * Scan the event flags page of 'this' CPU looking for any bit that is set.  If we find one
> + * bit set, then wait for a few milliseconds.  Repeat these steps for a maximum of 3 times.
> + * Return 'true', if there is still any set bit after this operation; 'false', otherwise.
> + *
> + * If a bit is set, that means there is a pending channel interrupt.  The expectation is
> + * that the normal interrupt handling mechanism will find and process the channel
> interrupt
> + * "very soon", and in the process clear the bit.
> + */
> +static bool hv_synic_event_pending(void)
> +{
> +	struct hv_per_cpu_context *hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> +	union hv_synic_event_flags *event =
> +		(union hv_synic_event_flags *)hv_cpu->synic_event_page +
> VMBUS_MESSAGE_SINT;
> +	unsigned long *recv_int_page = event->flags; /* assumes VMBus version >=
> VERSION_WIN8 */
> +	bool pending;
> +	u32 relid;
> +	int tries = 0;
> +
> +retry:
> +	pending = false;
> +	for_each_set_bit(relid, recv_int_page, HV_EVENT_FLAGS_COUNT) {
> +		/* Special case - VMBus channel protocol messages */
> +		if (relid == 0)
> +			continue;
> +		pending = true;
> +		break;
> +	}
> +	if (pending && tries++ < HV_MAX_TRIES) {
> +		usleep_range(10000, 20000);
> +		goto retry;
> +	}
> +	return pending;
> +}
> 
>  int hv_synic_cleanup(unsigned int cpu)
>  {
> @@ -336,6 +372,19 @@ int hv_synic_cleanup(unsigned int cpu)
>  	if (channel_found && vmbus_connection.conn_state == CONNECTED)
>  		return -EBUSY;
> 
> +	if (vmbus_proto_version >= VERSION_WIN10_V4_1) {
> +		/*
> +		 * channel_found == false means that any channels that were previously
> +		 * assigned to the CPU have been reassigned elsewhere with a call of
> +		 * vmbus_send_modifychannel().  Scan the event flags page looking for
> +		 * bits that are set and waiting with a timeout for vmbus_chan_sched()
> +		 * to process such bits.  If bits are still set after this operation
> +		 * and VMBus is connected, fail the CPU offlining operation.
> +		 */
> +		if (hv_synic_event_pending() && vmbus_connection.conn_state == CONNECTED)
> +			return -EBUSY;
> +	}
> +

Perhaps the test for conn_state == CONNECTED could be factored out as follows.  If we're
not CONNECTED (i.e., in the panic or kexec path) we should be able to also skip the search
for channels that are bound to the CPU since we will ignore the result anyway.

	if (vmbus_connection.conn_state != CONNECTED)
		goto always_cleanup;

	if (cpu == VMBUS_CONNECT_CPU)
		return -EBUSY;

	[Code to search for channels that are bound to the CPU we're about to clean up]
	
	if (channel_found)
		return -EBUSY;

	/*
	 * channel_found == false means that any channels that were previously
	 * assigned to the CPU have been reassigned elsewhere with a call of
	 * vmbus_send_modifychannel().  Scan the event flags page looking for
	 * bits that are set and waiting with a timeout for vmbus_chan_sched()
	 * to process such bits.  If bits are still set after this operation
	 * and VMBus is connected, fail the CPU offlining operation.
	 */
	if (vmbus_proto_version >= VERSION_WIN10_V4_1 && hv_synic_event_pending())
		return -EBUSY;

always_cleanup:

>  	hv_stimer_legacy_cleanup(cpu);
> 
>  	hv_synic_disable_regs(cpu);
> --
> 2.25.1


  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-14 15:01 [PATCH v2 0/3] Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE Andrea Parri (Microsoft)
2021-04-14 15:01 ` [PATCH v2 1/3] Drivers: hv: vmbus: Introduce and negotiate VMBus protocol version 5.3 Andrea Parri (Microsoft)
2021-04-14 18:25   ` Michael Kelley
2021-04-15 11:08     ` Andrea Parri
2021-04-14 15:01 ` [PATCH v2 2/3] Drivers: hv: vmbus: Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE Andrea Parri (Microsoft)
2021-04-14 18:26   ` Michael Kelley
2021-04-14 15:01 ` [PATCH v2 3/3] Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline Andrea Parri (Microsoft)
2021-04-14 18:43   ` Michael Kelley [this message]
2021-04-15 11:39     ` 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=MWHPR21MB15931F523196BCE9E23293E4D74E9@MWHPR21MB1593.namprd21.prod.outlook.com \
    --to=mikelley@microsoft.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=parri.andrea@gmail.com \
    --cc=sthemmin@microsoft.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