linux-kernel - Re: [PATCH v10 2/7] x86, acpi, cpu-hotplug: Enable acpi to register all possible cpus at boot time.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 29 Jul 2016 15:36:51 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Dou Liyang <douly.fnst@...fujitsu.com>
cc:	cl@...ux.com, tj@...nel.org, mika.j.penttila@...il.com,
	mingo@...hat.com, akpm@...ux-foundation.org, rjw@...ysocki.net,
	hpa@...or.com, yasu.isimatu@...il.com,
	isimatu.yasuaki@...fujitsu.com, kamezawa.hiroyu@...fujitsu.com,
	izumi.taku@...fujitsu.com, gongzhaogang@...pur.com,
	len.brown@...el.com, lenb@...nel.org, chen.tang@...ystack.cn,
	rafael@...nel.org, x86@...nel.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Gu Zheng <guz.fnst@...fujitsu.com>,
	Tang Chen <tangchen@...fujitsu.com>,
	Zhu Guihua <zhugh.fnst@...fujitsu.com>
Subject: Re: [PATCH v10 2/7] x86, acpi, cpu-hotplug: Enable acpi to register
 all possible cpus at boot time.

On Tue, 26 Jul 2016, Dou Liyang wrote: 

> 1. Enable apic registeration flow to handle both enabled and disabled cpus.
>    This is done by introducing an extra parameter to generic_processor_info to
>    let the caller control if disabled cpus are ignored.

If I'm reading the patch correctly then the 'enabled' argument controls more
than the disabled cpus accounting. It also controls the modification of
num_processors and the present mask.
 
> -int generic_processor_info(int apicid, int version)
> +static int __generic_processor_info(int apicid, int version, bool enabled)
>  {
>  	int cpu, max = nr_cpu_ids;
>  	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
> @@ -2032,7 +2032,8 @@ int generic_processor_info(int apicid, int version)
>  			   " Processor %d/0x%x ignored.\n",
>  			   thiscpu, apicid);
>  
> -		disabled_cpus++;
> +		if (enabled)
> +			disabled_cpus++;
>  		return -ENODEV;
>  	}
>  
> @@ -2049,7 +2050,8 @@ int generic_processor_info(int apicid, int version)
>  			" reached. Keeping one slot for boot cpu."
>  			"  Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
>  
> -		disabled_cpus++;
> +		if (enabled)
> +			disabled_cpus++;

This is utterly confusing. That code path cannot be reached when enabled is
false, because num_processors is 0 as we never increment it when enabled is
false.

That said, I really do not like this 'slap some argument on it and make it
work somehow' approach.

The proper solution for this is to seperate out the functionality which you
need for the preparation run (enabled = false) and make sure that the
information you need for the real run (enabled = true) is properly cached
somewhere so we don't have to evaluate the same thing over and over.

Thanks,

	tglx