Re: [PATCH 2/4] x86: fix cpu_mask_to_apicid_and to include cpu_online_mask

[Rusty Russell <rusty@rustcorp.com.au>]

On Saturday 13 December 2008 03:07:06 Mike Travis wrote:
> Rusty Russell wrote:
> > On Thursday 11 December 2008 21:58:08 Mike Travis wrote:
> >> Impact: fix potential problem.
> >>
> >> In determining the destination apicid, there are usually three cpumasks
> >> that are considered: the incoming cpumask arg, cfg->domain and the
> >> cpu_online_mask.  Since we are just introducing the cpu_mask_to_apicid_and
> >> function, make sure it includes the cpu_online_mask in it's evaluation.
> > 
> > Yerk.  Can we really "fail" cpu_mask_to_apicid_and with no repercussions?
> > And does it make sense to try to fix this there?
> 
> Fail?  The only failure is if there is not a cpu that satisfies the conjunction
> of the three masks, in which case it returns BAD_APICID.

That patch showed cpumask_alloc_var in some implementations of
cpu_mask_to_apicid_and.  This is bad.

> The old procedure was to:
> 
> function(..., cpumask_t mask)
> {
> 	cpumask_t tmp;
> 
> 	cpus_and(tmp, mask, cfg->domain);
> 	...
> 	cpus_and(tmp, tmp, cpu_online_map);
> 	dest = cpu_mask_to_apicid(tmp);
> 	...
> }
> 
> So making cpu_mask_to_apicid_and return:
> 
> 	dest = cpu_mask_to_apicid(mask1 & mask2 & cpu_online_mask);
> 
> maintains this compatibility.

But that was the purpose of x86:set_desc_affinity.patch.  It centralized
those conventions, and other than being a nice cleanup, it got that right.

See below.

Now, there are some places which call cpu_mask_to_apicid_and() and don't
include the online mask, but I checked: they were that way before.  Possibly
an existing bug?

Otherwise, it could be that setting the desc->affinity earlier (as this patch
does) has caused a problem.  Which of these code paths were you running?

Cheers,
Rusty.

===
x86: centralize common code for set_affinity variants.

Impact: cleanup, remove on-stack cpumasks

Everyone checks that a cpu is online, calls assign_irq_vector, and
also uses a temporary cpumask.

I *think* it's legal to set the desc->affinity in place in all cases,
so I avoid the temporary cpumask.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Ingo Molnar <mingo@redhat.com>
---
 arch/x86/kernel/io_apic.c |  112 ++++++++++++++--------------------------------
 1 file changed, 36 insertions(+), 76 deletions(-)

diff -r 07e2723b2a5d arch/x86/kernel/io_apic.c
--- a/arch/x86/kernel/io_apic.c	Tue Nov 18 11:20:08 2008 +1030
+++ b/arch/x86/kernel/io_apic.c	Tue Nov 18 11:40:35 2008 +1030
@@ -361,33 +361,38 @@
 
 static int assign_irq_vector(int irq, const struct cpumask *mask);
 
+/* Either sets desc->affinity to a valid value, and returns cpu_mask_to_apicid
+ * of that, or returns BAD_APICID and leaves desc->affinity untouched. */
+static unsigned int set_desc_affinity(unsigned irq, const struct cpumask *mask)
+{
+	struct irq_cfg *cfg;
+	struct irq_desc *desc;
+
+	if (!cpumask_intersects(mask, cpu_online_mask))
+		return BAD_APICID;
+
+	cfg = irq_cfg(irq);
+	if (assign_irq_vector(irq, mask))
+		return BAD_APICID;
+
+	desc = irq_to_desc(irq);
+	cpumask_and(&desc->affinity, to_cpumask(cfg->domain), mask);
+	return cpu_mask_to_apicid(&desc->affinity);
+}
+
 static void set_ioapic_affinity_irq(unsigned int irq,
 				    const struct cpumask *mask)
 {
-	struct irq_cfg *cfg;
 	unsigned long flags;
 	unsigned int dest;
-	cpumask_t tmp;
-	struct irq_desc *desc;
 
-	if (!cpumask_intersects(mask, cpu_online_mask))
-		return;
-
-	cfg = irq_cfg(irq);
-	if (assign_irq_vector(irq, mask))
-		return;
-
-	cpumask_and(&tmp, &cfg->domain, mask);
-	dest = cpu_mask_to_apicid(&tmp);
-	/*
-	 * Only the high 8 bits are valid.
-	 */
-	dest = SET_APIC_LOGICAL_ID(dest);
-
-	desc = irq_to_desc(irq);
 	spin_lock_irqsave(&ioapic_lock, flags);
-	__target_IO_APIC_irq(irq, dest, cfg->vector);
-	cpumask_copy(&desc->affinity, mask);
+	dest = set_desc_affinity(irq, mask);
+	if (dest != BAD_APICID) {
+		/* Only the high 8 bits are valid. */
+		dest = SET_APIC_LOGICAL_ID(dest);
+		__target_IO_APIC_irq(irq, dest, irq_cfg(irq)->vector);
+	}
 	spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 #endif /* CONFIG_SMP */
@@ -3017,32 +3022,21 @@
 #ifdef CONFIG_SMP
 static void set_msi_irq_affinity(unsigned int irq, const struct cpumask *mask)
 {
-	struct irq_cfg *cfg;
 	struct msi_msg msg;
 	unsigned int dest;
-	cpumask_t tmp;
-	struct irq_desc *desc;
 
-	if (!cpumask_intersects(mask, cpu_online_mask))
+	dest = set_desc_affinity(irq, mask);
+	if (dest == BAD_APICID)
 		return;
-
-	if (assign_irq_vector(irq, mask))
-		return;
-
-	cfg = irq_cfg(irq);
-	cpumask_and(&tmp, &cfg->domain, mask);
-	dest = cpu_mask_to_apicid(&tmp);
 
 	read_msi_msg(irq, &msg);
 
 	msg.data &= ~MSI_DATA_VECTOR_MASK;
-	msg.data |= MSI_DATA_VECTOR(cfg->vector);
+	msg.data |= MSI_DATA_VECTOR(irq_cfg(irq)->vector);
 	msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
-	desc = irq_to_desc(irq);
-	cpumask_copy(&desc->affinity, mask);
 }
 
 #ifdef CONFIG_INTR_REMAP
@@ -3055,23 +3049,17 @@
 {
 	struct irq_cfg *cfg;
 	unsigned int dest;
-	cpumask_t tmp;
 	struct irte irte;
 	struct irq_desc *desc;
-
-	if (!cpumask_intersects(mask, cpu_online_mask))
-		return;
 
 	if (get_irte(irq, &irte))
 		return;
 
-	if (assign_irq_vector(irq, mask))
+	dest = set_desc_affinity(irq, mask);
+	if (dest == BAD_APICID)
 		return;
 
 	cfg = irq_cfg(irq);
-	cpumask_and(&tmp, to_cpumask(cfg->domain), mask);
-	dest = cpu_mask_to_apicid(&tmp);
-
 	irte.vector = cfg->vector;
 	irte.dest_id = IRTE_DEST(dest);
 
@@ -3106,9 +3094,6 @@
 		}
 		cfg->move_in_progress = 0;
 	}
-
-	desc = irq_to_desc(irq);
-	cpumask_copy(&desc->affinity, mask);
 }
 #endif
 #endif /* CONFIG_SMP */
@@ -3315,19 +3300,12 @@
 	struct irq_cfg *cfg;
 	struct msi_msg msg;
 	unsigned int dest;
-	cpumask_t tmp;
-	struct irq_desc *desc;
 
-	if (!cpumask_intersects(mask, cpu_online_mask))
-		return;
-
-	if (assign_irq_vector(irq, mask))
+	dest = set_desc_affinity(irq, mask);
+	if (dest == BAD_APICID)
 		return;
 
 	cfg = irq_cfg(irq);
-	cpumask_and(&tmp, &cfg->domain, mask);
-	dest = cpu_mask_to_apicid(&tmp);
-
 	dmar_msi_read(irq, &msg);
 
 	msg.data &= ~MSI_DATA_VECTOR_MASK;
@@ -3336,8 +3314,6 @@
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	dmar_msi_write(irq, &msg);
-	desc = irq_to_desc(irq);
-	cpumask_copy(&desc->affinity, mask);
 }
 #endif /* CONFIG_SMP */
 
@@ -3373,20 +3349,14 @@
 static void hpet_msi_set_affinity(unsigned int irq, const struct cpumask *mask)
 {
 	struct irq_cfg *cfg;
-	struct irq_desc *desc;
 	struct msi_msg msg;
 	unsigned int dest;
-	cpumask_t tmp;
 
-	if (!cpumask_intersects(mask, cpu_online_mask))
-		return;
-
-	if (assign_irq_vector(irq, mask))
+	dest = set_desc_affinity(irq, mask);
+	if (dest == BAD_APICID)
 		return;
 
 	cfg = irq_cfg(irq);
-	cpumask_and(&tmp, to_cpumask(cfg->domain), mask);
-	dest = cpu_mask_to_apicid(&tmp);
 
 	hpet_msi_read(irq, &msg);
 
@@ -3396,8 +3366,6 @@
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	hpet_msi_write(irq, &msg);
-	desc = irq_to_desc(irq);
-	cpumask_copy(&desc->affinity, mask);
 }
 #endif /* CONFIG_SMP */
 
@@ -3455,22 +3423,14 @@
 {
 	struct irq_cfg *cfg;
 	unsigned int dest;
-	cpumask_t tmp;
-	struct irq_desc *desc;
 
-	if (!cpumask_intersects(mask, cpu_online_mask))
-		return;
-
-	if (assign_irq_vector(irq, mask))
+	dest = set_desc_affinity(irq, mask);
+	if (dest == BAD_APICID)
 		return;
 
 	cfg = irq_cfg(irq);
-	cpumask_and(&tmp, to_cpumask(cfg->domain), mask);
-	dest = cpu_mask_to_apicid(&tmp);
 
 	target_ht_irq(irq, dest, cfg->vector);
-	desc = irq_to_desc(irq);
-	cpumask_copy(&desc->affinity, mask);
 }
 #endif