Re: [PATCH 4/7] xen/arm: When CPU dies, free percpu area immediatelly

From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: edgar.iglesias@xilinx.com, dm@aggios.com,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org, Jan Beulich <JBeulich@suse.com>,
	Mirela Simonovic <mirela.simonovic@aggios.com>
Subject: Re: [PATCH 4/7] xen/arm: When CPU dies, free percpu area immediatelly
Date: Mon, 16 Apr 2018 14:14:38 +0100	[thread overview]
Message-ID: <d826b63c-9cf4-3a7d-7b8c-f285289b2840@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1804121314410.3304@sstabellini-ThinkPad-X260>

Hi,

On 12/04/18 22:31, Stefano Stabellini wrote:
> On Thu, 12 Apr 2018, Julien Grall wrote:
>> On 12/04/18 00:46, Stefano Stabellini wrote:
>>> On Wed, 11 Apr 2018, Julien Grall wrote:
>>>> On 11/04/18 14:19, Mirela Simonovic wrote:
>>>>> Freeing percpu area is done when a non-boot CPU is disabled upon
>>>>> suspend.
>>>>> This use to be scheduled for execution after a period of time, what
>>>>> caused
>>>>> the following racing issues. If CPU is enabled after it is disabled and
>>>>> before the freeing of percpu area is performed, Xen would crash upon
>>>>> initializing percpu area because per cpu offset is not marked as
>>>>> INVALID_PERCPU_AREA (this suppose to happen when cpu area is freed).
>>>>> To resolve the racing issue, free percpu area right away instead
>>>>> scheduling it for later.
>>>>
>>>> The reason of using the RCU is you want to make sure that none of the
>>>> other
>>>> CPUs will access that percpu data before freeing it. So I don't think this
>>>> patch is valid.
>>>>
>>>> It looks like to me a rcu barrier is missing after calling cpu_down
>>>> somewhere
>>>> in the CPU off path. I am not entirely sure where.
>>>
>>> We need a rcu_barrier(). Perhaps, it could be added on cpu_on before
>>> initializing the percpu area?
>>
>> Do you mind giving a bit more details on your thought? cpu_up looks a strange
>> place as no one should access the percpu area after the CPU is down. So it
>> feels the rcu_barrier should be somewhere before PSCI_cpu_off is called.
> 
> Yes, it feels strange to do it on cpu_on, it would be more obvious on
> cpu_off, but we don't actually need to _free_percpu_area on cpu_off,
> right? We only need to make sure it is done before cpu_percpu_callback
> is called on cpu_on.
> 
> My suggestion would be to evaluate if it is possible to introduce the
> rcu_barrier() on the resume path before cpu_percpu_callback, maybe in
> start_secondary.

Well, cpu_percpu_callback is not called by start_secondary. It is called 
when preparing the CPU from another CPU. So anything in start_secondary 
will not work.

> 
> I was also looking at xen/arch/x86/acpi/power.c:enter_state and noticed
> that they chose to call rcu_barrier() on enable_cpu before
> enable_nonboot_cpus().

I guess the rcu_barrier() in the function handling suspend/resume works. 
But that doesn't cover the hotplug case. Looking at x86, suspend/resume 
case. For the hotplug case, there are an rcu_barrier in 
cpu_{up,down}_helper but they are only present in the case of 
cpu_{up,down} failed. I am not entirely sure how this is handled in x86

Andrew, Jan, do you know when the percpu will be free on hotplug? It is 
call to call_rcu(...) but I am not sure when this is going to be executed.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel