Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 27 Apr 2018 00:08:44 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "K. Y. Srinivasan" <kys@microsoft.com>
cc:     x86@kernel.org, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
        olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com,
        hpa@zytor.com, sthemmin@microsoft.com,
        Michael.H.Kelley@microsoft.com, vkuznets@redhat.com
Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
In-Reply-To: <20180425181250.8740-2-kys@linuxonhyperv.com>
Message-ID: <alpine.DEB.2.21.1804262350040.1596@nanos.tec.linutronix.de>
References: <20180425181110.8683-1-kys@linuxonhyperv.com> <20180425181250.8740-1-kys@linuxonhyperv.com> <20180425181250.8740-2-kys@linuxonhyperv.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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