Re: [PATCH v3] x86: correct socket_cpumask allocation

[Chao Peng <chao.p.peng@linux.intel.com>]

On Fri, Jul 10, 2015 at 06:03:54PM +0200, Dario Faggioli wrote:
> On Fri, 2015-07-10 at 16:25 +0100, Jan Beulich wrote:
> > >>> On 10.07.15 at 17:13, <JBeulich@suse.com> wrote:
> 
> > > cpu_down()
> > >   stop_machine_run(take_cpu_down, ...)
> > >     notifier_call_chain(&cpu_chain, CPU_DYING, ...)
> > >     __cpu_disable()
> > >       remove_siblinginfo()
> > >   __cpu_die()
> > > notifier_call_chain(&cpu_chain, CPU_DEAD, ...)
> > >   cpu_smpboot_free()
> > > 
> > > I.e. a clear use-after-invalidate.
> > 
> > And I can't see a reason why we shouldn't be able to defer invoking
> > remove_siblinginfo() until cpu_smpboot_free(): Other than its
> > counterpart (set_cpu_sibling_map()) it doesn't require to be run on
> > the subject CPU (that function itself doesn't appear to depend on
> > that either, but it depends on identify_cpu() having run). Which
> > would at once allow reducing code: The clearing of
> > cpu_{core,sibling}_mask then becomes redundant with the freeing
> > of these masks.
> > 
> > Of course there may be hidden dependencies, so maybe a safer
> > approach would be to just move the zapping of the three IDs
> > (and maybe the clearing of the CPU's cpu_sibling_setup_map bit)
> > into cpu_smpboot_free().
> > cpu_smpboot_free
> FWIW, I've tried the patch below (call remove_siblinginfo() from
> cpu_smpboot_free()), and it works for me.
> 
> I've tested both shutdown and ACPI S3 suspend.
> 
> I've got to go now, so I guess I'm leaving it to Chao whether to pick it
> up, or go with the other approach Jan suggested (or something else).

I have no problem if Jan agreed and perhaps especially the OSS test can
pass that (as it already looks tricky for this part of code).

I can pick this change and send a patch, with the additional change of
moving "cpumask_clear_cpu(cpu, socket_cpumask[cpu_to_socket(cpu)])"
from remove_siblinginfo() to cpu_smpboot_free().

Chao