Re: [PATCH 1/2] x86/cpu hotplug: make apicid <--> cpuid mapping persistent

[Gu Zheng <guz.fnst@cn.fujitsu.com>]

Hi Kame-san,
On 03/26/2015 11:19 AM, Kamezawa Hiroyuki wrote:

> On 2015/03/26 11:17, Gu Zheng wrote:
>> Previously, we build the apicid <--> cpuid mapping when the cpu is present, but
>> the relationship will be changed if the cpu/node hotplug happenned, because we
>> always choose the first free cpuid for the hot added cpu (whether it is new-add
>> or re-add), so this the cpuid <--> node mapping changed if node hot plug
>> occurred, and it causes the wq sub-system allocation failture:
>>    ==
>>       SLUB: Unable to allocate memory on node 2 (gfp=0x80d0)
>>        cache: kmalloc-192, object size: 192, buffer size: 192, default
>> order:
>>      1, min order: 0
>>        node 0: slabs: 6172, objs: 259224, free: 245741
>>        node 1: slabs: 3261, objs: 136962, free: 127656
>>    ==
>> So here we build the persistent [lapic id] <--> cpuid mapping when the cpu first
>> present, and never change it.
>>
>> Suggested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>   arch/x86/kernel/apic/apic.c |   31 ++++++++++++++++++++++++++++++-
>>   1 files changed, 30 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
>> index ad3639a..d539ebc 100644
>> --- a/arch/x86/kernel/apic/apic.c
>> +++ b/arch/x86/kernel/apic/apic.c
>> @@ -2038,6 +2038,30 @@ void disconnect_bsp_APIC(int virt_wire_setup)
>>   	apic_write(APIC_LVT1, value);
>>   }
>>   
>> +/*
>> + * Logic cpu number(cpuid) to local APIC id persistent mappings.
>> + * Do not clear the mapping even if cpu hot removed.
>> + * */
>> +static int apicid_to_x86_cpu[MAX_LOCAL_APIC] = {
>> +	[0 ... MAX_LOCAL_APIC - 1] = -1,
>> +};
> 
> 
> This patch cannot handle x2apic, which is 32bit.

IMO, if the apicid is too big (larger than MAX_LOCAL_APIC), we will skip
generating a logic cpu number for it, so it seems no problem here.

> 
> As far as I understand, it depends on CPU's spec and the newest cpu has 9bit apicid, at least.
> 
> But you can't create inifinit array.
> 
> If you can't allocate the array dynamically, How about adding
> 
>  static int cpuid_to_apicid[MAX_CPU] = {}
> 
> or using idr library ? (please see lib/idr.c)
> 
> I guess you can update this map after boot(after mm initialization)
> and make use of idr library.
> 
> About this patch, Nack.
> 
> -Kame
> 
> 
> 
>> +
>> +/*
>> + * Internal cpu id bits, set the bit once cpu present, and never clear it.
>> + * */
>> +static cpumask_t cpuid_mask = CPU_MASK_NONE;
>> +
>> +static int get_cpuid(int apicid)
>> +{
>> +	int cpuid;
>> +
>> +	cpuid = apicid_to_x86_cpu[apicid];
>> +	if (cpuid == -1)
>> +		cpuid = cpumask_next_zero(-1, &cpuid_mask);
>> +
>> +	return cpuid;
>> +}
>> +
>>   int generic_processor_info(int apicid, int version)
>>   {
>>   	int cpu, max = nr_cpu_ids;
>> @@ -2115,7 +2139,10 @@ int generic_processor_info(int apicid, int version)
>>   		 */
>>   		cpu = 0;
>>   	} else
>> -		cpu = cpumask_next_zero(-1, cpu_present_mask);
>> +		cpu = get_cpuid(apicid);
>> +
>> +	/* Store the mapping */
>> +	apicid_to_x86_cpu[apicid] = cpu;
>>   
>>   	/*
>>   	 * Validate version
>> @@ -2144,6 +2171,8 @@ int generic_processor_info(int apicid, int version)
>>   	early_per_cpu(x86_cpu_to_logical_apicid, cpu) =
>>   		apic->x86_32_early_logical_apicid(cpu);
>>   #endif
>> +	/* Mark this cpu id as uesed (already mapping a local apic id) */
>> +	cpumask_set_cpu(cpu, &cpuid_mask);
>>   	set_cpu_possible(cpu, true);
>>   	set_cpu_present(cpu, true);
>>   
>>
> 
> 
> .
>