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

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

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?

Jan


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