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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 07/31/17 10:03 PM >>>
>We have limited number (slightly under NR_DYNAMIC_VECTORS=192) of IRQ
>vectors that are available to each processor. Currently, when x2apic
>cluster mode is used (which is default), each vector is shared among
>all processors in the cluster. With many IRQs (as is the case on systems
>with multiple SR-IOV cards) and few clusters (e.g. single socket)
>there is a good chance that we will run out of vectors.
>
>This patch tries to decrease vector sharing between processors by
>assigning vector to a single processor if the assignment request (via
>__assign_irq_vector()) comes without explicitly specifying which
>processors are expected to share the interrupt. This typically happens
>during boot time (or possibly PCI hotplug) when create_irq(NUMA_NO_NODE)
>is called. When __assign_irq_vector() is called from
>set_desc_affinity() which provides sharing mask, vector sharing will
>continue to be performed, as before.

Wouldn't it be sufficient for people running into vector shortage due to
sharing to specify "x2apic_phys" on the command line?

>@@ -72,9 +73,13 @@ 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)
>{
>-    return per_cpu(cluster_cpus, cpu);
>+    if ( cpumask != TARGET_CPUS )

Is a pointer comparison (rather than a content one) here really correct in
the general case?

>+        return per_cpu(cluster_cpus, cpu);
>+    else
>+        return cpumask_of(cpu);

Please avoid the "else" in cases like this.

>--- a/xen/include/asm-x86/mach-generic/mach_apic.h
>+++ b/xen/include/asm-x86/mach-generic/mach_apic.h
>@@ -13,10 +13,11 @@
>#define INT_DELIVERY_MODE (genapic->int_delivery_mode)
>#define INT_DEST_MODE (genapic->int_dest_mode)
>#define TARGET_CPUS      (genapic->target_cpus())
>-#define init_apic_ldr (genapic->init_apic_ldr)
>-#define clustered_apic_check (genapic->clustered_apic_check) 
>-#define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)
>-#define vector_allocation_cpumask(cpu) 
>(genapic->vector_allocation_cpumask(cpu))
>+#define INIT_APIC_LDR (genapic->init_apic_ldr)
>+#define CLUSTERED_APIC_CHECK (genapic->clustered_apic_check) 
>+#define CPU_MASK_TO_APICID (genapic->cpu_mask_to_apicid)
>+#define VECTOR_ALLOCATION_CPUMASK(cpu, mask) \
>+    (genapic->vector_allocation_cpumask(cpu, mask))
 
I don't see the connection of the change in case of all of these to the purpose
of this patch.

Jan


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