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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 <mirela.simonovic@xxxxxxxxxx>
> > > > ---
> > > >    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 = &per_cpu(free_info, cpu);
> > > >        info->cpu = cpu;
> > > > -    call_rcu(&info->rcu, _free_percpu_area);
> > > > +    _free_percpu_area(&info->rcu);
> > > >    }
> > > >      static int cpu_percpu_callback(
> > > > 
> > > 
> > > -- 
> > > Julien Grall
> > > 
> 
> -- 
> Julien Grall
> 

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