Re: [PATCH 12/12] parisc: Implement __cpu_die() and __cpu_disable() for CPU hotplugging

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

Am Freitag, 25. März 2022, 15:38:33 CET schrieb Helge Deller:
> Add relevant code to __cpu_die() and __cpu_disable() to finally enable
> the CPU hotplugging features. Reset the irq count values in smp_callin()
> to zero before bringing up the CPU.
> 
> Use "chcpu -d 1" to bring CPU1 down, and "chcpu -e 1" to bring it up.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>  arch/parisc/Kconfig           |  1 +
>  arch/parisc/include/asm/smp.h |  9 +---
>  arch/parisc/kernel/smp.c      | 80 +++++++++++++++++++++++++++++++++--
>  3 files changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
> index a32a882a2d58..60cc33fd345c 100644
> --- a/arch/parisc/kernel/smp.c
> +++ b/arch/parisc/kernel/smp.c
> @@ -430,10 +444,68 @@ void smp_cpus_done(unsigned int cpu_max)
> 
>  int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>  {
> -	if (cpu != 0 && cpu < parisc_max_cpus && smp_boot_one_cpu(cpu, 
tidle))
> -		return -ENOSYS;
> +	if (cpu_online(cpu))
> +		return 0;
> +
> +	if (num_online_cpus() < parisc_max_cpus && smp_boot_one_cpu(cpu, 
tidle))
> +		return -EIO;

I had to look up parisc_max_cpus, and found this:

> static int parisc_max_cpus = 1;

Hm, signed?

> parisc_max_cpus = max_cpus;
>        if (!max_cpus)
>                printk(KERN_INFO "SMP mode deactivated.\n");

So parisc_max_cpus is now 0, which seems wrong. Shouldn't the check be before 
the assignment? This would have avoided the "cpu != 0" in the old code 
completely.

> +
> +	return cpu_online(cpu) ? 0 : -EIO;
> +}
> +
> +/*
> + * __cpu_disable runs on the processor to be shutdown.
> + */
> +int __cpu_disable(void)
> +{
> +#ifdef CONFIG_HOTPLUG_CPU
> +	unsigned int cpu = smp_processor_id();
> +
> +	remove_cpu_topology(cpu);
> +
> +	/*
> +	 * Take this CPU offline.  Once we clear this, we can't return,
> +	 * and we must not schedule until we're ready to give up the cpu.
> +	 */
> +	set_cpu_online(cpu, false);
> +
> +	/*
> +	 * disable IPI interrupt
> +	 */
> +	disable_percpu_irq(IPI_IRQ);
> +
> +	/*
> +	 * migrate IRQs away from this CPU
> +	 */
> +	irq_migrate_all_off_this_cpu();

While I really enjoy good code comments the last 2 seem a t bit wasteful, 
given that the code is basically exactly the same as the text.

> +	/*
> +	 * Flush user cache and TLB mappings, and then remove this CPU
> +	 * from the vm mask set of all processes.
> +	 *
> +	 * Caches are flushed to the Level of Unification Inner Shareable
> +	 * to write-back dirty lines to unified caches shared by all CPUs.
> +	 */
> +	flush_cache_all_local();
> +	flush_tlb_all_local(NULL);
> 
> -	return cpu_online(cpu) ? 0 : -ENOSYS;
> +	/* disable all irqs, including timer irq */
> +	local_irq_disable();
> +#endif
> +	return 0;
> +}
> +
> +/*
> + * called on the thread which is asking for a CPU to be shutdown -
> + * waits until shutdown has completed, or it is timed out.
> + */
> +void __cpu_die(unsigned int cpu)
> +{
> +	if (!cpu_wait_death(cpu, 5)) {
> +		pr_crit("CPU%u: cpu didn't die\n", cpu);
> +		return;
> +	}
> +	pr_debug("CPU%u: shutdown\n", cpu);
>  }
> 
>  #ifdef CONFIG_PROC_FS
> --
> 2.35.1


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]