Re: [PATCH] smpboot: Add smpboot state variables instead of reusing CPU hotplug states
From: Daniel Wagner
Date: Thu Nov 05 2015 - 03:10:13 EST
Hi Paul,
I guess this patch got the summer conference period treatment. ACK,
NACK, completely STUPID idea?
cheers,
daniel
On 10/15/2015 01:32 PM, Daniel Wagner wrote:
> The cpu hotplug state machine in smpboot.c is reusing the states from
> cpu.h. That is confusing when it comes to the CPU_DEAD_FROZEN usage.
> Paul explained to me that he was in need of an additional state
> for destinguishing between a CPU error states. For this he just
> picked CPU_DEAD_FROZEN.
>
> 8038dad7e888581266c76df15d70ca457a3c5910 smpboot: Add common code for notification from dying CPU
> 2a442c9c6453d3d043dfd89f2e03a1deff8a6f06 x86: Use common outgoing-CPU-notification code
>
> Instead of reusing the states, let's add new definition inside
> the smpboot.c file with explenation what those states
> mean. Thanks Paul for providing them.
>
> Signed-off-by: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> arch/x86/xen/smp.c | 4 +--
> include/linux/cpu.h | 3 +-
> kernel/smpboot.c | 82 ++++++++++++++++++++++++++++++++++++++++-------------
> 3 files changed, 67 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 3f4ebf0..804bf5c 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -495,7 +495,7 @@ static int xen_cpu_up(unsigned int cpu, struct task_struct *idle)
> rc = HYPERVISOR_vcpu_op(VCPUOP_up, cpu, NULL);
> BUG_ON(rc);
>
> - while (cpu_report_state(cpu) != CPU_ONLINE)
> + while (!cpu_check_online(cpu))
> HYPERVISOR_sched_op(SCHEDOP_yield, NULL);
>
> return 0;
> @@ -767,7 +767,7 @@ static int xen_hvm_cpu_up(unsigned int cpu, struct task_struct *tidle)
> * This can happen if CPU was offlined earlier and
> * offlining timed out in common_cpu_die().
> */
> - if (cpu_report_state(cpu) == CPU_DEAD_FROZEN) {
> + if (cpu_check_timeout(cpu)) {
> xen_smp_intr_free(cpu);
> xen_uninit_lock_cpu(cpu);
> }
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 23c30bd..f78ab46 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -284,7 +284,8 @@ void arch_cpu_idle_dead(void);
>
> DECLARE_PER_CPU(bool, cpu_dead_idle);
>
> -int cpu_report_state(int cpu);
> +int cpu_check_online(int cpu);
> +int cpu_check_timeout(int cpu);
> int cpu_check_up_prepare(int cpu);
> void cpu_set_state_online(int cpu);
> #ifdef CONFIG_HOTPLUG_CPU
> diff --git a/kernel/smpboot.c b/kernel/smpboot.c
> index a818cbc..75e5724 100644
> --- a/kernel/smpboot.c
> +++ b/kernel/smpboot.c
> @@ -371,19 +371,63 @@ int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread,
> }
> EXPORT_SYMBOL_GPL(smpboot_update_cpumask_percpu_thread);
>
> +/* The CPU is offline, and its last offline operation was
> + * successful and proceeded normally. (Or, alternatively, the
> + * CPU never has come online, as this is the initial state.)
> + */
> +#define CPUHP_POST_DEAD 0x01
> +
> +/* The CPU is in the process of coming online.
> + * Simple architectures can skip this state, and just invoke
> + * cpu_set_state_online() unconditionally instead.
> + */
> +#define CPUHP_UP_PREPARE 0x02
> +
> +/* The CPU is now online. Simple architectures can skip this
> + * state, and just invoke cpu_wait_death() and cpu_report_death()
> + * unconditionally instead.
> + */
> +#define CPUHP_ONLINE 0x03
> +
> +/* The CPU has gone offline, so that it may now be safely
> + * powered off (or whatever the architecture needs to do to it).
> + */
> +#define CPUHP_DEAD 0x04
> +
> +/* The CPU did not go offline in a timely fashion, if at all,
> + * so it might need special processing at the next online (for
> + * example, simply refusing to bring it online).
> + */
> +#define CPUHP_BROKEN 0x05
> +
> +/* The CPU eventually did go offline, but not in a timely
> + * fashion. If some sort of reset operation is required before it
> + * can be brought online, that reset operation needs to be carried
> + * out at online time. (Or, again, the architecture might simply
> + * refuse to bring it online.)
> + */
> +#define CPUHP_TIMEOUT 0x06
> +
> static DEFINE_PER_CPU(atomic_t, cpu_hotplug_state) = ATOMIC_INIT(CPU_POST_DEAD);
>
> /*
> * Called to poll specified CPU's state, for example, when waiting for
> * a CPU to come online.
> */
> -int cpu_report_state(int cpu)
> +int cpu_check_online(int cpu)
> +{
> + return atomic_read(&per_cpu(cpu_hotplug_state, cpu)) ==
> + CPUHP_ONLINE;
> +}
> +
> +int cpu_check_timeout(int cpu)
> {
> - return atomic_read(&per_cpu(cpu_hotplug_state, cpu));
> + return atomic_read(&per_cpu(cpu_hotplug_state, cpu)) ==
> + CPUHP_TIMEOUT;
> }
>
> /*
> - * If CPU has died properly, set its state to CPU_UP_PREPARE and
> + * If CPU has died properly, set its state to CPUHP_UP_PREPARE and
> * return success. Otherwise, return -EBUSY if the CPU died after
> * cpu_wait_death() timed out. And yet otherwise again, return -EAGAIN
> * if cpu_wait_death() timed out and the CPU still hasn't gotten around
> @@ -397,19 +441,19 @@ int cpu_report_state(int cpu)
> int cpu_check_up_prepare(int cpu)
> {
> if (!IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
> - atomic_set(&per_cpu(cpu_hotplug_state, cpu), CPU_UP_PREPARE);
> + atomic_set(&per_cpu(cpu_hotplug_state, cpu), CPUHP_UP_PREPARE);
> return 0;
> }
>
> switch (atomic_read(&per_cpu(cpu_hotplug_state, cpu))) {
>
> - case CPU_POST_DEAD:
> + case CPUHP_POST_DEAD:
>
> /* The CPU died properly, so just start it up again. */
> - atomic_set(&per_cpu(cpu_hotplug_state, cpu), CPU_UP_PREPARE);
> + atomic_set(&per_cpu(cpu_hotplug_state, cpu), CPUHP_UP_PREPARE);
> return 0;
>
> - case CPU_DEAD_FROZEN:
> + case CPUHP_TIMEOUT:
>
> /*
> * Timeout during CPU death, so let caller know.
> @@ -424,7 +468,7 @@ int cpu_check_up_prepare(int cpu)
> */
> return -EBUSY;
>
> - case CPU_BROKEN:
> + case CPUHP_BROKEN:
>
> /*
> * The most likely reason we got here is that there was
> @@ -452,7 +496,7 @@ int cpu_check_up_prepare(int cpu)
> */
> void cpu_set_state_online(int cpu)
> {
> - (void)atomic_xchg(&per_cpu(cpu_hotplug_state, cpu), CPU_ONLINE);
> + (void)atomic_xchg(&per_cpu(cpu_hotplug_state, cpu), CPUHP_ONLINE);
> }
>
> #ifdef CONFIG_HOTPLUG_CPU
> @@ -470,12 +514,12 @@ bool cpu_wait_death(unsigned int cpu, int seconds)
> might_sleep();
>
> /* The outgoing CPU will normally get done quite quickly. */
> - if (atomic_read(&per_cpu(cpu_hotplug_state, cpu)) == CPU_DEAD)
> + if (atomic_read(&per_cpu(cpu_hotplug_state, cpu)) == CPUHP_DEAD)
> goto update_state;
> udelay(5);
>
> /* But if the outgoing CPU dawdles, wait increasingly long times. */
> - while (atomic_read(&per_cpu(cpu_hotplug_state, cpu)) != CPU_DEAD) {
> + while (atomic_read(&per_cpu(cpu_hotplug_state, cpu)) != CPUHP_DEAD) {
> schedule_timeout_uninterruptible(sleep_jf);
> jf_left -= sleep_jf;
> if (jf_left <= 0)
> @@ -484,14 +528,14 @@ bool cpu_wait_death(unsigned int cpu, int seconds)
> }
> update_state:
> oldstate = atomic_read(&per_cpu(cpu_hotplug_state, cpu));
> - if (oldstate == CPU_DEAD) {
> + if (oldstate == CPUHP_DEAD) {
> /* Outgoing CPU died normally, update state. */
> smp_mb(); /* atomic_read() before update. */
> - atomic_set(&per_cpu(cpu_hotplug_state, cpu), CPU_POST_DEAD);
> + atomic_set(&per_cpu(cpu_hotplug_state, cpu), CPUHP_POST_DEAD);
> } else {
> /* Outgoing CPU still hasn't died, set state accordingly. */
> if (atomic_cmpxchg(&per_cpu(cpu_hotplug_state, cpu),
> - oldstate, CPU_BROKEN) != oldstate)
> + oldstate, CPUHP_BROKEN) != oldstate)
> goto update_state;
> ret = false;
> }
> @@ -502,7 +546,7 @@ update_state:
> * Called by the outgoing CPU to report its successful death. Return
> * false if this report follows the surviving CPU's timing out.
> *
> - * A separate "CPU_DEAD_FROZEN" is used when the surviving CPU
> + * A separate "CPUHP_TIMEOUT" is used when the surviving CPU
> * timed out. This approach allows architectures to omit calls to
> * cpu_check_up_prepare() and cpu_set_state_online() without defeating
> * the next cpu_wait_death()'s polling loop.
> @@ -515,13 +559,13 @@ bool cpu_report_death(void)
>
> do {
> oldstate = atomic_read(&per_cpu(cpu_hotplug_state, cpu));
> - if (oldstate != CPU_BROKEN)
> - newstate = CPU_DEAD;
> + if (oldstate != CPUHP_BROKEN)
> + newstate = CPUHP_DEAD;
> else
> - newstate = CPU_DEAD_FROZEN;
> + newstate = CPUHP_TIMEOUT;
> } while (atomic_cmpxchg(&per_cpu(cpu_hotplug_state, cpu),
> oldstate, newstate) != oldstate);
> - return newstate == CPU_DEAD;
> + return newstate == CPUHP_DEAD;
> }
>
> #endif /* #ifdef CONFIG_HOTPLUG_CPU */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/