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

[Julien Grall]

On 18/04/18 23:52, Stefano Stabellini wrote:

On Tue, 17 Apr 2018, Julien Grall wrote:

On 17/04/18 11:52, Mirela Simonovic wrote:

Hi Julien,


Hi Mirela,


On Mon, Apr 16, 2018 at 5:21 PM, Julien Grall  wrote:



On 16/04/18 14:41, Mirela Simonovic wrote:


On Mon, Apr 16, 2018 at 3:14 PM, Julien Grall 
wrote:


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:


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.



AFAIK disable/enable_nonboot_cpus() is the only way to do the hotplug
and rcu_barrier() is not included in the flow.



That's not the only way. I clearly specified one in my previous answer
(see
cpu_{up,down}_helper) and there are other place (look for cpu_up).



I've looked at cpu_{up,down}_helper and cpu_up and I'm convinced now
that adding rcu_barrier() prior to calling enable_nonboot_cpus() is
the right approch.

cpu_{up,down}_helper functions exist only for x86.


They have nothing very x86 specific AFAICT so they could potentially be used
for Arm when XEN_SYSCTL_hotplug will be implemented.


cpu_up_helper()
does call rcu_barrier() prior to calling cpu_up().


That's not true. Below the code for cpu_up_helper():

 int ret = cpu_up(cpu); <- First call
 if ( ret == -EBUSY )
 {
 rcu_barrier();<- RCU barrier
 ret = cpu_up(cpu); <- Second call
 }
 return ret;

So the rcu_barrier is called after cpu_up() in case it returns -EBUSY.


So calling rcu_barrier() is expected to be done prior to calling
cpu_up() (or enable_nonboot_cpus(), which is just a wrapper for
cpu_up()).

I believe this is right way to do because cpu_up() is used for
enabling non-boot CPUs in both boot and suspend/hotplug scenarios,
while rcu_barrier() is not required in boot scenario.
Therefore, I'll add rcu_barrier() prior to calling
enable_nonboot_cpus(). If I missed something please let me know.


See above, this is exactly why I asked Andrew & Jan input on how rcu work is
flushed when using cpu_up_helper/cpu_down_helper. Because I don't understand
if it is meant to work.

So I would like to see whether it would make sense to put the rcu_barrier()
somewhere else to cover every call of cpu_up().


I thought that for the specific problem we are talking about, we only
need a rcu_barrier() after a cpu has been brought offline (and before
it is brought online again). I don't understand what other use-cases you
are thinking about that might require an rcu_barrier().

Is this the question you are asking Andrew and Jan?


There are 2 use cases:
- suspend/resume
- XEN_SYSCTL_hotplug (not implemented yet on Arm)

Mirela suggestion only apply for suspend/resume. However, there are 
clearly something undocumented in the code about the expectation on 
calling cpu_up/cpu_down.


I would like to understand where is the rcu_barrier when the CPU is 
offlined using the hypercall XEN_SYSCTL_hotplug. As I pointed out there 
the only rcu_barrier call I can find in that path is when cpu_down() fails.


The answer may be: "It is missing and would need to be fixed on x86" and 
that would be fine by me.


In any case, we should probably document the expectation if we rely on 
the caller of cpu_{up,down} to call rcu_barrier(). Otherwise, this is 
yet another way to rediscover the problem with a different path.


Cheers,

--
Julien Grall

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

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

[Julien Grall]

On 17/04/18 11:52, Mirela Simonovic wrote:

Hi Julien,


Hi Mirela,


On Mon, Apr 16, 2018 at 5:21 PM, Julien Grall  wrote:



On 16/04/18 14:41, Mirela Simonovic wrote:


On Mon, Apr 16, 2018 at 3:14 PM, Julien Grall 
wrote:


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:


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.



AFAIK disable/enable_nonboot_cpus() is the only way to do the hotplug
and rcu_barrier() is not included in the flow.



That's not the only way. I clearly specified one in my previous answer (see
cpu_{up,down}_helper) and there are other place (look for cpu_up).



I've looked at cpu_{up,down}_helper and cpu_up and I'm convinced now
that adding rcu_barrier() prior to calling enable_nonboot_cpus() is
the right approch.

cpu_{up,down}_helper functions exist only for x86.


They have nothing very x86 specific AFAICT so they could potentially be 
used for Arm when XEN_SYSCTL_hotplug will be implemented.



cpu_up_helper()
does call rcu_barrier() prior to calling cpu_up().


That's not true. Below the code for cpu_up_helper():

int ret = cpu_up(cpu); <- First call
if ( ret == -EBUSY )
{
rcu_barrier(); <- RCU barrier
ret = cpu_up(cpu); <- Second call
}
return ret;

So the rcu_barrier is called after cpu_up() in case it returns -EBUSY.


So calling rcu_barrier() is expected to be done prior to calling
cpu_up() (or enable_nonboot_cpus(), which is just a wrapper for
cpu_up()).

I believe this is right way to do because cpu_up() is used for
enabling non-boot CPUs in both boot and suspend/hotplug scenarios,
while rcu_barrier() is not required in boot scenario.
Therefore, I'll add rcu_barrier() prior to calling
enable_nonboot_cpus(). If I missed something please let me know.


See above, this is exactly why I asked Andrew & Jan input on how rcu 
work is flushed when using cpu_up_helper/cpu_down_helper. Because I 
don't understand if it is meant to work.


So I would like to see whether it would make sense to put the 
rcu_barrier() somewhere else to cover every call of cpu_up().


Cheers,

--
Julien Grall

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

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

[Mirela Simonovic]

Hi Julien,

On Mon, Apr 16, 2018 at 5:21 PM, Julien Grall  wrote:
>
>
> On 16/04/18 14:41, Mirela Simonovic wrote:
>>
>> On Mon, Apr 16, 2018 at 3:14 PM, Julien Grall 
>> wrote:
>>>
>>> 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:
>>>
>>> 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.
>>>
>>
>> AFAIK disable/enable_nonboot_cpus() is the only way to do the hotplug
>> and rcu_barrier() is not included in the flow.
>
>
> That's not the only way. I clearly specified one in my previous answer (see
> cpu_{up,down}_helper) and there are other place (look for cpu_up).
>

I've looked at cpu_{up,down}_helper and cpu_up and I'm convinced now
that adding rcu_barrier() prior to calling enable_nonboot_cpus() is
the right approch.

cpu_{up,down}_helper functions exist only for x86. cpu_up_helper()
does call rcu_barrier() prior to calling cpu_up().
So calling rcu_barrier() is expected to be done prior to calling
cpu_up() (or enable_nonboot_cpus(), which is just a wrapper for
cpu_up()).

I believe this is right way to do because cpu_up() is used for
enabling non-boot CPUs in both boot and suspend/hotplug scenarios,
while rcu_barrier() is not required in boot scenario.
Therefore, I'll add rcu_barrier() prior to calling
enable_nonboot_cpus(). If I missed something please let me know.

Thanks,
Mirela

> Cheers,
>
> --
> Julien Grall

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

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

[Julien Grall]

On 16/04/18 14:41, Mirela Simonovic wrote:

On Mon, Apr 16, 2018 at 3:14 PM, Julien Grall  wrote:

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:

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.



AFAIK disable/enable_nonboot_cpus() is the only way to do the hotplug
and rcu_barrier() is not included in the flow.


That's not the only way. I clearly specified one in my previous answer 
(see cpu_{up,down}_helper) and there are other place (look for cpu_up).


Cheers,

--
Julien Grall

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

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

[Mirela Simonovic]

Hi Julien, Stefano,

Thanks for the feedback and suggestions.

On Mon, Apr 16, 2018 at 3:14 PM, Julien Grall  wrote:
> 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 have also confirmed that in start_secondary it doesn't work, it's too late.

>>
>> 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().
>

Before the enable_nonboot_cpus() gets invoked seems to be a good place
for rcu_barrier(), as it's done for x86.

>
> 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.
>

AFAIK disable/enable_nonboot_cpus() is the only way to do the hotplug
and rcu_barrier() is not included in the flow.
I suggest to invoke rcu_barrier() before enable_nonboot_cpus() and
eventually this could be moved into enable_nonboot_cpus() itself.

Thanks,
Mirela

> Cheers,
>
> --
> Julien Grall

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

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

[Julien Grall]

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

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

[Stefano Stabellini]

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.

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().


 
> > > > 
> > > > Signed-off-by: Mirela Simonovic 
> > > > ---
> > > >xen/arch/arm/percpu.c | 2 +-
> > > >1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
> > > > index 25442c48fe..e4e8405f43 100644
> > > > --- a/xen/arch/arm/percpu.c
> > > > +++ b/xen/arch/arm/percpu.c
> > > > @@ -46,7 +46,7 @@ static void free_percpu_area(unsigned int cpu)
> > > >{
> > > >struct free_info *info = _cpu(free_info, cpu);
> > > >info->cpu = cpu;
> > > > -call_rcu(>rcu, _free_percpu_area);
> > > > +_free_percpu_area(>rcu);
> > > >}
> > > >  static int cpu_percpu_callback(
> > > > 
> > > 
> > > -- 
> > > Julien Grall
> > > 
> 
> -- 
> Julien Grall
> 

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

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

[Julien Grall]

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.


Cheers,






Signed-off-by: Mirela Simonovic 
---
   xen/arch/arm/percpu.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
index 25442c48fe..e4e8405f43 100644
--- a/xen/arch/arm/percpu.c
+++ b/xen/arch/arm/percpu.c
@@ -46,7 +46,7 @@ static void free_percpu_area(unsigned int cpu)
   {
   struct free_info *info = _cpu(free_info, cpu);
   info->cpu = cpu;
-call_rcu(>rcu, _free_percpu_area);
+_free_percpu_area(>rcu);
   }
 static int cpu_percpu_callback(



--
Julien Grall



--
Julien Grall

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

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

[Stefano Stabellini]

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?


> > 
> > Signed-off-by: Mirela Simonovic 
> > ---
> >   xen/arch/arm/percpu.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
> > index 25442c48fe..e4e8405f43 100644
> > --- a/xen/arch/arm/percpu.c
> > +++ b/xen/arch/arm/percpu.c
> > @@ -46,7 +46,7 @@ static void free_percpu_area(unsigned int cpu)
> >   {
> >   struct free_info *info = _cpu(free_info, cpu);
> >   info->cpu = cpu;
> > -call_rcu(>rcu, _free_percpu_area);
> > +_free_percpu_area(>rcu);
> >   }
> > static int cpu_percpu_callback(
> > 
> 
> -- 
> Julien Grall
> 

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

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

[Julien Grall]

Hi,

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.


Cheers,



Signed-off-by: Mirela Simonovic 
---
  xen/arch/arm/percpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
index 25442c48fe..e4e8405f43 100644
--- a/xen/arch/arm/percpu.c
+++ b/xen/arch/arm/percpu.c
@@ -46,7 +46,7 @@ static void free_percpu_area(unsigned int cpu)
  {
  struct free_info *info = _cpu(free_info, cpu);
  info->cpu = cpu;
-call_rcu(>rcu, _free_percpu_area);
+_free_percpu_area(>rcu);
  }
  
  static int cpu_percpu_callback(




--
Julien Grall

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