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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.17 at 18:00, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 08/25/2017 10:56 AM, Jan Beulich wrote:
>>>>> On 08.08.17 at 17:59, <boris.ostrovsky@xxxxxxxxxx> 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'.

How about the opposite, 'isolate_vectors'? To me that would seem
to fit better with the intention of the change.

>> 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?

Along the lines of the above plus our desire to limit the number
of top level options, how about "apic=isolate-vectors"?

Also I don't understand your reference to hotplug, nor why you
suggest two opposite default values.

But finally, you agreeing to a command line option here makes
me come back to an earlier suggestion: Didn't we agree that
"x2apic_phys" would be sufficient to eliminate the issue? In which
case no patch would be needed at all.

Jan


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