LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: olaf@aepfle.de, sthemmin@microsoft.com,
	gregkh@linuxfoundation.org, jasowang@redhat.com, x86@kernel.org,
	linux-kernel@vger.kernel.org, Michael.H.Kelley@microsoft.com,
	hpa@zytor.com, apw@canonical.com, devel@linuxdriverproject.org,
	vkuznets@redhat.com
Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
Date: Fri, 27 Apr 2018 00:08:44 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.21.1804262350040.1596@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20180425181250.8740-2-kys@linuxonhyperv.com>

On Wed, 25 Apr 2018, kys@linuxonhyperv.com wrote:
> +static int __send_ipi_mask(const struct cpumask *mask, int vector)
> +{
> +	int cur_cpu, vcpu;
> +	struct ipi_arg_non_ex **arg;
> +	struct ipi_arg_non_ex *ipi_arg;
> +	int ret = 1;

So this indicates whether __send_ipi_mask() can send to @mask or not. So
please make it a bool and let it return false when it does not work, true
otherwise. If you had used -Exxxx then it would have been more obvious, but
this is really a boolean decision.

> +	unsigned long flags;
> +
> +	if (cpumask_empty(mask))
> +		return 0;
> +
> +	if (!hv_hypercall_pg)
> +		return ret;
> +
> +	if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
> +		return ret;
> +
> +	local_irq_save(flags);
> +	arg = (struct ipi_arg_non_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +
> +	ipi_arg = *arg;
> +	if (unlikely(!ipi_arg))
> +		goto ipi_mask_done;
> +
> +

Stray newline

> +	ipi_arg->vector = vector;
> +	ipi_arg->reserved = 0;
> +	ipi_arg->cpu_mask = 0;
> +
> +	for_each_cpu(cur_cpu, mask) {
> +		vcpu = hv_cpu_number_to_vp_number(cur_cpu);
> +		if (vcpu >= 64)
> +			goto ipi_mask_done;

This is completely magic and deserves a comment.

> +
> +		__set_bit(vcpu, (unsigned long *)&ipi_arg->cpu_mask);
> +	}
> +
> +	ret = hv_do_hypercall(HVCALL_SEND_IPI, ipi_arg, NULL);
> +
> +ipi_mask_done:
> +	local_irq_restore(flags);
> +	return ret;
> +}

....

>  static int hv_cpu_init(unsigned int cpu)
>  {
>  	u64 msr_vp_index;
>  	struct hv_vp_assist_page **hvp = &hv_vp_assist_page[smp_processor_id()];
> +	void **input_arg;
> +
> +	input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +	*input_arg = page_address(alloc_page(GFP_ATOMIC));

This is called from the cpu hotplug thread and there is no need for an
atomic allocation. Please use GFP_KERNEL.

>  	hv_get_vp_index(msr_vp_index);
>  
> @@ -217,6 +224,10 @@ static int hv_cpu_die(unsigned int cpu)
>  {
>  	struct hv_reenlightenment_control re_ctrl;
>  	unsigned int new_cpu;
> +	void **input_arg;
> +
> +	input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +	free_page((unsigned long)*input_arg);

Hrm. Again this is called from the CPU hotplug thread when the cou is about
to go down. But you can be scheduled out after free() and before disabling
the assist thing below and the pointer persist. There is no guarantee that
nothing sends an IPI anymore after this point.

So you have two options here:

  1) Disable interrupts, get the pointer, set the per cpu pointer to NULL,
     reenable interruots and free the page

  2) Keep the page around and check for it in the CPU UP path and avoid the
     allocation when the CPU comes online again.

>  	if (hv_vp_assist_page && hv_vp_assist_page[cpu])
>  		wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
> @@ -260,6 +271,12 @@ void __init hyperv_init(void)
>  	if ((ms_hyperv.features & required_msrs) != required_msrs)
>  		return;
>  
> +	/* Allocate the per-CPU state for the hypercall input arg */
> +	hyperv_pcpu_input_arg = alloc_percpu(void  *);
> +
> +	if (hyperv_pcpu_input_arg == NULL)
> +		return;

Huch. When that allocation fails, you return and ignore the rest of the
function which has been there before. Weird decision.

Thanks,

	tglx
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  parent reply	other threads:[~2018-04-26 22:08 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25 18:11 [PATCH 0/5] X86: Hyper-V: APIC enlightenments kys
2018-04-25 18:12 ` [PATCH 1/5] X86: Hyper-V: Enlighten APIC access kys
2018-04-25 18:12   ` [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments kys
2018-04-26 21:31     ` Dan Carpenter
2018-04-27  6:34       ` KY Srinivasan
2018-04-26 22:08     ` Thomas Gleixner [this message]
2018-04-27  6:11       ` KY Srinivasan
2018-04-26 22:54     ` Michael Kelley (EOSG)
2018-04-27  6:31       ` KY Srinivasan
2018-04-27  6:24     ` kbuild test robot
2018-04-27 11:21     ` kbuild test robot
2018-04-27 11:55     ` kbuild test robot
2018-04-25 18:12   ` [PATCH 3/5] X86: Hyper-V: Enhanced IPI enlightenment kys
2018-04-26 22:16     ` Thomas Gleixner
2018-04-27  6:24       ` KY Srinivasan
2018-04-26 23:25     ` Michael Kelley (EOSG)
2018-04-25 18:12   ` [PATCH 4/5] X86: Hyper-V: Consolidate code for converting cpumask to vpset kys
2018-04-26 22:21     ` Thomas Gleixner
2018-04-25 18:12   ` [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the hypercall input page kys
2018-04-26 22:23     ` Thomas Gleixner
2018-04-27  6:29       ` KY Srinivasan
2018-04-27 10:58         ` Thomas Gleixner
2018-04-27 11:50     ` kbuild test robot
2018-04-26 21:49   ` [PATCH 1/5] X86: Hyper-V: Enlighten APIC access Thomas Gleixner
2018-04-27  5:52     ` KY Srinivasan
2018-04-26 22:15   ` Michael Kelley (EOSG)
2018-04-26 22:55     ` Thomas Gleixner
2018-04-27  5:44   ` kbuild test robot

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=alpine.DEB.2.21.1804262350040.1596@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=Michael.H.Kelley@microsoft.com \
    --cc=apw@canonical.com \
    --cc=devel@linuxdriverproject.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hpa@zytor.com \
    --cc=jasowang@redhat.com \
    --cc=kys@microsoft.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=olaf@aepfle.de \
    --cc=sthemmin@microsoft.com \
    --cc=vkuznets@redhat.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).