Message-ID: <4970C984.30202@sgi.com>
Date:	Fri, 16 Jan 2009 09:53:08 -0800
From:	Mike Travis <travis@....com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Rusty Russell <rusty@...tcorp.com.au>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PULL}: latest tip/cpus4096 changes

Ingo Molnar wrote:
> * 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

Hi Ingo,

I did notice that the versions all came up the same, and that the checks
were very specific.  I was trying to be as transparent and unintrusive as
possible.  Since there's so few calls, I though this was a good approach
but apparently I was wrong.

I like the idea of collapsing the array down to one and checking to
see if all apic's have the same version, but is this really the case?
Must all apics be the same?  

Thanks,
Mike
--
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/

Powered by blists - more mailing lists