Re: [PATCH 1/2] xen/x86: Convert to hotplug state machine

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 08/17/2016 04:33 AM, Sebastian Andrzej Siewior wrote:
> On 2016-08-15 10:46:46 [-0400], Boris Ostrovsky wrote:
>> Switch to new CPU hotplug infrastructure.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>>  arch/x86/xen/enlighten.c   |  115 +++++++++++++++++++++++++-------------------
>>  include/linux/cpuhotplug.h |    2 +
>>  2 files changed, 67 insertions(+), 50 deletions(-)
>>
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index c7f6b1f..2283976 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -1541,6 +1543,24 @@ static void __init xen_dom0_set_legacy_features(void)
>>  	x86_platform.legacy.rtc = 1;
>>  }
>>  
>> +static int xen_cpuhp_setup(void)
>> +{
>> +	int rc;
>> +
>> +	rc = cpuhp_setup_state_nocalls(CPUHP_XEN_PREPARE,
>> +				       "XEN_HVM_GUEST_PREPARE",
>> +				       xen_cpu_up_prepare, xen_cpu_up_cancel);
> The old states UP_CANCEL is different from UP_PREPARE. The latter was
> invoked only in the error case while your new callback is always
> invoked. From looking at the code you free memory in
> xen_cpu_up_cancel() which was allocated in xen_cpu_up_prepare() and
> therefore I would name it xen_cpu_dead().

Yes, "cancel" is wrong term to use here.

> If you do find the time, you might manage to rework the code to avoid
> using the _nocalls() function. If see this right, you use
> xen_setup_vcpu_info_placement() for the init in the first place. This
> uses for_each_possible_cpu macro. The cpuhp_setup_state() function would
> perform the init for all CPUs before they come up.

I am not sure I see what this would buy us.

Besides, cpuhp_setup_state() uses for_each_present_cpu().

>
>> +	if (!rc) {
>> +		rc = cpuhp_setup_state_nocalls(CPUHP_AP_XEN_ONLINE,
> If there is no need to run this after KVM's CLK callback please use
> CPUHP_AP_ONLINE_DYN. If there is such a need then please document it.

OK.

-boris

>
>> +					       "XEN_HVM_GUEST_PREPARE",
>> +					       xen_cpu_up_online, NULL);
>> +		if (rc)
>> +			cpuhp_remove_state_nocalls(CPUHP_XEN_PREPARE);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>>  /* First C function to be called on Xen boot */
>>  asmlinkage __visible void __init xen_start_kernel(void)
>>  {
> Sebastian