linux-kernel - Re: [PULL}: latest tip/cpus4096 changes

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20090116092540.GC4305@elte.hu>
Date:	Fri, 16 Jan 2009 10:25:40 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Mike Travis <travis@....com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PULL}: latest tip/cpus4096 changes

* Mike Travis <travis@....com> wrote:

> 
> Hi Ingo,
> 
> Please pull the following 'fairly lightweight' changes for tip/cpus4096.

> --- a/arch/x86/kernel/apic.c
> +++ b/arch/x86/kernel/apic.c
> @@ -1562,8 +1562,69 @@ void __init init_apic_mappings(void)
>   * This initializes the IO-APIC and APIC hardware if this is
>   * a UP kernel.
>   */
> +
> +#if MAX_APICS < 256
>  int apic_version[MAX_APICS];
>  
> +#else
> +struct apic_version_info {
> +	unsigned int apicid;
> +	int version;
> +};
> +
> +struct apic_version_info _apic_version_info[CONFIG_NR_CPUS] __initdata;
> +struct apic_version_info *apic_version_info __refdata = _apic_version_info;
> +int nr_apic_version_info;
> +
> +/* can be called either during init or cpu hotplug add */
> +int __cpuinit add_apic_version(unsigned int apicid, int version)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_apic_version_info; i++)
> +		if (apicid == apic_version_info[i].apicid) {
> +			apic_version_info[i].version = version;
> +			return 0;
> +		}
> +
> +	if (likely(nr_apic_version_info < nr_cpu_ids)) {
> +		i = nr_apic_version_info++;
> +		apic_version_info[i].apicid = apicid;
> +		apic_version_info[i].version = version;
> +		return 0;
> +	}
> +	return -ENOMEM;
> +}
> +
> +/* lookup version for apic, usually first one (boot cpu) */
> +int get_apic_version(unsigned int apicid)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_apic_version_info; i++)
> +		if (apicid == apic_version_info[i].apicid)
> +			return apic_version_info[i].version;
> +
> +	return 0;
> +}
> +
> +/* allocate permanent apic_version structure */
> +void __init cleanup_apic_version(void)
> +{
> +	size_t size;
> +	int i;
> +
> +	/* allows disabled_cpus to be brought online */
> +	size = nr_cpu_ids * sizeof(*apic_version_info);
> +	apic_version_info = alloc_bootmem(size);
> +
> +	/* copy version info from initial array to permanent array */
> +	for (i = 0; i < nr_apic_version_info; i++)
> +		apic_version_info[i] = _apic_version_info[i];
> +}
> +
> +#endif /* MAX_APICS >= 256 */

this is all but 'lightweight'. A 'lightweight' patch is that which either 
is less than say a dozen lines or one that changes a provably trivial 
aspect of the kernel. This patch goes to the guts of the APIC code and is 
not only complex but also very ugly:

 - it's riddled with #ifdefs

 - it splits the testing space between <256 apics and large system - guess 
   which one will get 99% of the testing?

And why is it all done, a hundred lines of very ugly code in a fragile 
area of the x86 architecture? Because you want to shrink this array:

   int apic_version[MAX_APICS];

which can take 128K of RAM if MAX_APICs is 32K ...

Firstly, if you want to shrink the APIC version array, you might want to 
shrink it the most obvious way: by changing the 'int' to 'char' via a 
oneliner patch. APIC version values tend to be significantly below 256. 
That gives 75% of space savings already.

Secondly, you might even observe the fact that the set of systems with 
assymetric APIC versions approximates that of the nil set. Assymetric SMP 
never took off and probably never will - vendors have hard enough time 
getting the very same type of CPU working across the board.

And _even_ if there existed such systems, we already have a lot of other 
places where symmetry is assumed and _material_ APIC version assymetry to 
the boot CPU's APIC version will very likely not work anyway, on the 
physical signalling level.

So the simplest approach might as well be to turn apic_version into a 
single __read_mostly boot_apic_version variable. Maybe also a 
WARN_ONCE("whee") message if an APIC is seen with a version different from 
that of the boot CPU.

_Please_ think these changes through because these kinds of mindless 
complication patches are not acceptable at all.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/