Re: [PATCH 1/2] x86, irq: update irq_cfg domain unless the new affinity is a subset of the current domain

[Suresh Siddha <suresh.b.siddha@intel.com>]

On Mon, 2012-06-18 at 17:51 -0700, Suresh Siddha wrote:
> > -		more_domains = apic->vector_allocation_domain(cpu, tmp_mask);
> > +		more = apic->vector_allocation_domain(cpu, mask, tmp_mask);
> > +		cmp = apic->compare_domains(tmp_mask, cfg->domain);
> 
> I think we should be able to consolidate both of them into one
> apic_driver specific routine. Also I think we should be able to use the
> tmp_mask in each round and minimize the number of unnecessary
> for_each_cpu iterations. And that should clean up the more_domains bool
> logic we did earlier.

Just posted couple of patches which did the above. Please review and
ack.

On Mon, 2012-06-18 at 11:17 +0200, Alexander Gordeev wrote:
> Also, this would fix a previous subtle condition introduced with previous
> commit 332afa6 ("x86/irq: Update irq_cfg domain unless the new affinity is a
> subset of the current domain") when a change of affinity mask could trigger
> unnecessary move of vector in case new and old masks intersect, but the new
> mask is not a subset of the old one.

So, I gave some thought to this. And I don't like/didn't address it for
the reasons I mentioned earlier. I don't want to add intelligence to the
apic drivers. That belongs to a higher level entity like 'irqbalance'.
Also when irqbalance asks for a specific mask, I don't want its behavior
to depend on the previous configuration. And I want the behavior to be
some what consistent across different apic drivers whose underlying HW
behavior is similar. For example logical-flat with no HW round-robin
will route the interrupt to the first cpu specified in the user-mask.

Also this is a not common case worth caring about.

BTW, there is still one open that I would like to address. How to handle
the vector pressure during boot etc (as the default vector assignment
specifies all online cpus) when there are lot interrupt sources but
fewer x2apic clusters (like one or two socket server case).

We should be able to do something like the appended. Any better
suggestions? I don't want to add boot parameters to limit the x2apic
cluster membership etc (to fewer than 16 logical cpu's) if possible.
---

 arch/x86/kernel/apic/x2apic_cluster.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index 4d49512..0e3d659 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -217,7 +217,20 @@ static void cluster_vector_allocation_domain(const struct cpumask *mask,
 					     const struct cpumask *prevmask)
 {
 	int cpu = cpumask_first(retmask);
-	cpumask_and(retmask, mask, per_cpu(cpus_in_cluster, cpu));
+
+	/*
+	 * By default during boot, device bringup etc interrupt
+	 * will be routed to a specific cpu.
+	 *
+	 * On irq migration requests coming from irqbalance etc,
+	 * interrupts will be routed to the x2apic cluster (cluster-id
+	 * derived from the first cpu in the mask) members specified
+	 * in the mask.
+	 */
+	if (mask == cpu_online_mask)
+		cpumask_copy(retmask, cpumask_of(cpu));
+	else
+		cpumask_and(retmask, mask, per_cpu(cpus_in_cluster, cpu));
 }
 
 static struct apic apic_x2apic_cluster = {


thanks,
suresh