Re: [Xen-devel] [PATCH RFC 01/49] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 1 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 4/1/19 10:40 AM, Juergen Gross wrote:
> > On 01/04/2019 11:21, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 3/29/19 3:08 PM, Juergen Gross wrote:
> > > > cpu_disable_scheduler() is being called from __cpu_disable() today.
> > > > There is no need to execute it on the cpu just being disabled, so use
> > > > the CPU_DEAD case of the cpu notifier chain. Moving the call out of
> > > > stop_machine() context is fine, as we just need to hold the domain RCU
> > > > lock and need the scheduler percpu data to be still allocated.
> > > > 
> > > > Add another hook for CPU_DOWN_PREPARE to bail out early in case
> > > > cpu_disable_scheduler() would fail. This will avoid crashes in rare
> > > > cases for cpu hotplug or suspend.
> > > > 
> > > > While at it remove a superfluous smp_mb() in the ARM __cpu_disable()
> > > > incarnation.
> > > 
> > > This is not obvious why the smp_mb() is superfluous. Can you please
> > > provide more details on why this is not necessary?
> > 
> > cpumask_clear_cpu() should already have the needed semantics, no?
> > It is based on clear_bit() which is defined to be atomic.
> 
> atomicity does not mean the store/load cannot be re-ordered by the CPU. You
> would need a barrier to prevent re-ordering.
> 
> cpumask_clear_cpu() and clear_bit() does not contain any barrier, so
> store/load can be re-ordered.
> 
> I see we have similar smp_mb() barrier in __cpu_die(). Sadly, there are no
> documentation in the code why the barrier is here. The logs don't help either.
> 
> The barrier here will ensure that the load/store related to disabling the CPU
> are seen before any load/store happening after the return. Although, I am not
> sure why this is necessary.
> 
> Stefano, Do you remember the rationale?

/me doing some archeology

I am pretty sure it was meant to accompany the cpumask_clear_cpu call. I
think we should keep it in __cpu_disable right after cpumask_clear_cpu.

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