Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Jiri Olsa]

On Tue, Oct 04, 2016 at 12:58:04PM +0200, Thomas Gleixner wrote:
> On Mon, 3 Oct 2016, Prarit Bhargava wrote:
> > BUG: unable to handle kernel paging request at 0000000000841f1f
> > IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
> ...
> >  [<ffffffff81015a60>] ? uncore_cpu_starting+0x130/0x130
> >  [<ffffffff81015acc>] uncore_event_cpu_online+0x6c/0x80
> >  [<ffffffff8108e819>] cpuhp_invoke_callback+0x49/0x100
> >  [<ffffffff8108ead1>] cpuhp_thread_fun+0x41/0x100
> >  [<ffffffff810b054f>] smpboot_thread_fn+0x10f/0x160
> >  [<ffffffff810b0440>] ? sort_range+0x30/0x30
> >  [<ffffffff810accd8>] kthread+0xd8/0xf0
> >  [<ffffffff816ff4bf>] ret_from_fork+0x1f/0x40
> >  [<ffffffff810acc00>] ? kthread_park+0x60/0x60
> 
> > arch/x86/events/intel/uncore.c:
> > 1137 static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_     cpu,
> > 1138                                    int new_cpu)
> > 1139 {
> > 1140         struct intel_uncore_pmu *pmu = type->pmus;
> > 1141         struct intel_uncore_box *box;
> > 1142         int i, pkg;
> > 1143
> > 1144         pkg = topology_logical_package_id(old_cpu < 0 ? new_cpu : old_cpu);
> > 1145         for (i = 0; i < type->num_boxes; i++, pmu++) {
> > 1146                 box = pmu->boxes[pkg];
> > 
> > pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
> 
> And that's what needs to be fixed in the first place.

right, I'll check on that.. but I think we need this fix as well

> 
> > This patch adds the missing generic_processor_info() to
> > prefill_possible_map() to ensure the initialization of the boot cpu is
> > correct. 
> 
> > This results in smp_init_package_map() having correct data and
> > properly setting the package map for the hotplugged boot cpu, which in
> > turn resolves the kdump kernel panic on physically hotplugged cpus.
> 
> While it is the right thing to initialize the package map in that case, it
> still papers over a robustness issue in the uncore code, which needs to be
> fixed first.
> 
> > [2] prefill_possible_map() is called before smp_store_boot_cpu_info().
> > The comment beside the call to smp_store_boot_cpu_info() states that the
> > completed call results in "Final full version of the data".
> 
> I'm not sure what that [2] here means and I cannot figure out the meaning
> of this sentence either.
> 
> This changelog is incomprehensible in general and more a "oh look how I
> decoded this problem" report than something which clearly describes the
> problem at hand, the root cause and the fix. The latter wants a
> understandable explanation why prefill_possible_map() is the right place to
> do this.

I was wondering if acpi_boot_init was a better place for that, but then
Prarit suggested in our discussion that the prefill_possible_map() call
seems to be a hotplug cleanup.. so it seemed to fit

however it's difficult to say with complex code like this,
so any ideas are welcome ;-)

thanks,
jirka