Date:   Fri, 29 Mar 2019 02:54:43 -0600
From:   "Jan Beulich" <JBeulich@...e.com>
To:     "Boris Ostrovsky" <boris.ostrovsky@...cle.com>
Cc:     "Stefano Stabellini" <sstabellini@...nel.org>,
        "xen-devel" <xen-devel@...ts.xenproject.org>,
        "Juergen Gross" <jgross@...e.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/Xen: streamline (and fix) PV CPU enumeration

>>> On 28.03.19 at 17:50, <boris.ostrovsky@...cle.com> wrote:
> On 3/28/19 5:03 AM, Jan Beulich wrote:
>>>>> On 27.03.19 at 18:07, <boris.ostrovsky@...cle.com> wrote:
>>> On 3/27/19 11:12 AM, Jan Beulich wrote:
>>>> -
>>>> -static void __init xen_filter_cpu_maps(void)
>>>> +static void __init _get_smp_config(unsigned int early)
>>>>  {
>>>>  	int i, rc;
>>>>  	unsigned int subtract = 0;
>>>>  
>>>> -	if (!xen_initial_domain())
>>>> +	if (early)
>>>>  		return;
>>>>  
>>>>  	num_processors = 0;
>>>
>>> Is there a reason to set_cpu_possible() here (not in the diff, but in
>>> this routine)? This will be called from setup_arch() before
>>> prefill_possible_map(), which will clear and then re-initialize
>>> __cpu_possible_mask.
>> I don't think it's needed before my change either, so I'd call
>> removing this an orthogonal change. As said in the commit
>> message, the goal was to leave this function alone as far as
>> possible.
>>
>> Before my patch, xen_filter_cpu_maps() gets called long after
>> prefill_possible_map(), and by the purpose of the latter function
>> the possible map shouldn't be altered anymore once that has
>> run. Adding bits to it is surely not going to have the intended
>> effect (setup_per_cpu_areas() has already run), while removing
>> bits may have some effect, but comes too late at least for
>> setup_per_cpu_areas().
> 
> OK. Then it looks like even though your patch changes behavior slightly
> (so technically I guess it's not purely a cleanup) this shouldn't makes
> any difference at least as far as possible cpu mask is concerned: if we
> don't have percpu areas set up we can't do much for that vcpu since it
> seems to me xen_vcpu_setup(), for example, won't do well.
> 
>> And if we were to remove this, I think the CONFIG_HOTPLUG_CPU
>> section should go away as well. After all prefill_possible_map()
>> also sets nr_cpu_ids. To be honest, it was largely this code
>> fragment which made me want not touch the function more than
>> necessary: The comment there makes not clear to me at all why
>> all of this needs to be in an #ifdef in the first place.
> 
> This was introduced by cf405ae612b0 ("xen/smp: Fix crash when booting
> with ACPI hotplug CPUs.").
> 
> I am not sure this is still relevant. The ACPI hotplug code had changed,
> not significantly but sufficiently enough to alter behavior.
> acpi_processor_hotadd_init() now fails before it gets a chance to call
> arch_register_cpu() for vcpu>dom0_max_vcpus.
> 
>> Let me know whether you really want me to fold this extra
>> cleanup into this patch. If so, I'd then wonder whether the
>> set_cpu_present() from xen_pv_smp_prepare_cpus() shouldn't
>> be moved here, too. And the fiddling with the possible map
>> there looks bogus as well: Bring-up of CPUs past the command
>> line option should be avoided at boot time, but they shouldn't
>> be excluded from getting brought up later on. Note how
>> native_smp_prepare_cpus() ignores its parameter altogether.
> 
> Yes, that does look strange.
> 
> Given especially xen_pv_smp_prepare_cpus(), I think re-working proper
> setting of present/possible masks is well beyond the scope of your
> original patch.

Well, then the question is, what (if any) changes are you
expecting me to make for this change to be acceptable? Or do
you perhaps want me to add a 2nd patch on top addressing
the other outlined anomalies?

Jan

Powered by blists - more mailing lists