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

[Sebastian Andrzej Siewior]

On 2016-08-15 10:46:46 [-0400], Boris Ostrovsky wrote:
> Switch to new CPU hotplug infrastructure.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
>  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().
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.

> +	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.

> +					       "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