Re: [PATCH v2] x86/apic/x2apic: Share IRQ vector between cluster members only when no cpumask is specified

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 08/25/2017 10:56 AM, Jan Beulich wrote:
>>>> On 08.08.17 at 17:59, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/genapic/delivery.c
>> +++ b/xen/arch/x86/genapic/delivery.c
>> @@ -30,7 +30,8 @@ void __init clustered_apic_check_flat(void)
>>  	printk("Enabling APIC mode:  Flat.  Using %d I/O APICs\n", nr_ioapics);
>>  }
>>  
>> -const cpumask_t *vector_allocation_cpumask_flat(int cpu)
>> +const cpumask_t *vector_allocation_cpumask_flat(int cpu,
>> +    const cpumask_t *cpumask)
>>  {
>>  	return &cpu_online_map;
>>  } 
>> @@ -58,7 +59,8 @@ void __init clustered_apic_check_phys(void)
>>  	printk("Enabling APIC mode:  Phys.  Using %d I/O APICs\n", nr_ioapics);
>>  }
>>  
>> -const cpumask_t *vector_allocation_cpumask_phys(int cpu)
>> +const cpumask_t *vector_allocation_cpumask_phys(int cpu,
>> +    const cpumask_t *cpumask)
>>  {
>>  	return cpumask_of(cpu);
>>  }
>> --- a/xen/arch/x86/genapic/x2apic.c
>> +++ b/xen/arch/x86/genapic/x2apic.c
>> @@ -72,8 +72,12 @@ static void __init clustered_apic_check_x2apic(void)
>>  {
>>  }
>>  
>> -static const cpumask_t *vector_allocation_cpumask_x2apic_cluster(int cpu)
>> +static const cpumask_t *vector_allocation_cpumask_x2apic_cluster(int cpu,
>> +    const cpumask_t *cpumask)
>>  {
>> +    if ( !cpumask )
>> +        return cpumask_of(cpu);
>> +
>>      return per_cpu(cluster_cpus, cpu);
>>  }
> It is a strange addition you're making here: None of the three
> implementations care about the passed in mask. Why is this then
> not a bool with a suitable name?

I can pass in a bool. Say, 'bool share_vectors'.

>
> Additionally, shouldn't vector_allocation_cpumask_flat() behave
> similar to vector_allocation_cpumask_x2apic_cluster() then?

Yes, I should probably do that as well.

>
> Further I'd prefer if you made it a single return statement here,
> using a conditional expression.
>
> And finally I continue to be not really happy about the change as
> a whole. Despite what was discussed on v1, I'm concerned of the
> effects of this on hosts _not_ suffering from vector shortage.
> Could you live with the new behavior requiring a command line
> option to enable?

I can add something like 'apic_share_vectors', defaulting to true,
although it will not be useful in case of a hotplug. Defaulting to false?

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel