[PATCH] x86, fix x86 fixup_irqs() error handling

[PATCH] x86, fix x86 fixup_irqs() error handling

[Prarit Bhargava]

fixup_irqs() calls chip->set_irq_affinity which eventually calls
__assign_irq_vector().  Errors are not propogated back from this function call
and this results in silent irq relocation failures.  This patch fixes this
issue and prints out a warning if there is a relocation failure.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Rui Wang <rui.y.wang@intel.com>
Cc: Liu Ping Fan <kernelfans@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Cc: Yang Zhang <yang.z.zhang@Intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Cc: Li Fei <fei.li@intel.com>
Cc: gong.chen@linux.intel.com
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 arch/x86/kernel/apic/io_apic.c |   28 ++++++++++++++++++----------
 arch/x86/kernel/irq.c          |    9 +++++++--
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 6ad4658..b4b21db 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2312,7 +2312,7 @@ int __ioapic_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	int err;
 
 	if (!config_enabled(CONFIG_SMP))
-		return -1;
+		return -EPERM;
 
 	if (!cpumask_intersects(mask, cpu_online_mask))
 		return -EINVAL;
@@ -2343,7 +2343,7 @@ int native_ioapic_set_affinity(struct irq_data *data,
 	int ret;
 
 	if (!config_enabled(CONFIG_SMP))
-		return -1;
+		return -EPERM;
 
 	raw_spin_lock_irqsave(&ioapic_lock, flags);
 	ret = __ioapic_set_affinity(data, mask, &dest);
@@ -3075,9 +3075,11 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 	struct irq_cfg *cfg = data->chip_data;
 	struct msi_msg msg;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	__get_cached_msi_msg(data->msi_desc, &msg);
 
@@ -3177,9 +3179,11 @@ dmar_msi_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	struct irq_cfg *cfg = data->chip_data;
 	unsigned int dest, irq = data->irq;
 	struct msi_msg msg;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	dmar_msi_read(irq, &msg);
 
@@ -3226,9 +3230,11 @@ static int hpet_msi_set_affinity(struct irq_data *data,
 	struct irq_cfg *cfg = data->chip_data;
 	struct msi_msg msg;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	hpet_msi_read(data->handler_data, &msg);
 
@@ -3295,9 +3301,11 @@ ht_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 {
 	struct irq_cfg *cfg = data->chip_data;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	target_ht_irq(data->irq, dest, cfg->vector);
 	return IRQ_SET_MASK_OK_NOCOPY;
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index d99f31d..55fab61 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -351,6 +351,7 @@ void fixup_irqs(void)
 	struct irq_desc *desc;
 	struct irq_data *data;
 	struct irq_chip *chip;
+	int ret;
 
 	for_each_irq_desc(irq, desc) {
 		int break_affinity = 0;
@@ -389,8 +390,12 @@ void fixup_irqs(void)
 		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
 			chip->irq_mask(data);
 
-		if (chip->irq_set_affinity)
-			chip->irq_set_affinity(data, affinity, true);
+		if (chip->irq_set_affinity) {
+			ret = chip->irq_set_affinity(data, affinity, true);
+			WARN(ret == -ENOSPC,
+			     "IRQ %d set affinity failed with %d.  The device assigned to this IRQ is unstable.\n",
+			     irq, ret);
+		}
 		else if (!(warned++))
 			set_affinity = 0;
 
-- 
1.7.9.3

Re: [PATCH] x86, fix x86 fixup_irqs() error handling

[David Rientjes]

On Wed, 5 Mar 2014, Prarit Bhargava wrote:

> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index d99f31d..55fab61 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -351,6 +351,7 @@ void fixup_irqs(void)
>  	struct irq_desc *desc;
>  	struct irq_data *data;
>  	struct irq_chip *chip;
> +	int ret;
>  
>  	for_each_irq_desc(irq, desc) {
>  		int break_affinity = 0;
> @@ -389,8 +390,12 @@ void fixup_irqs(void)
>  		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
>  			chip->irq_mask(data);
>  
> -		if (chip->irq_set_affinity)
> -			chip->irq_set_affinity(data, affinity, true);
> +		if (chip->irq_set_affinity) {
> +			ret = chip->irq_set_affinity(data, affinity, true);
> +			WARN(ret == -ENOSPC,
> +			     "IRQ %d set affinity failed with %d.  The device assigned to this IRQ is unstable.\n",
> +			     irq, ret);

Should this be WARN_ON_ONCE() to avoid filling the kernel log instead?

It doesn't make much sense to print out the negative return value, maybe 
you meant to print -ret instead?

> +		}
>  		else if (!(warned++))
>  			set_affinity = 0;
>  

Re: [PATCH] x86, fix x86 fixup_irqs() error handling

[Prarit Bhargava]

On 03/05/2014 04:09 PM, David Rientjes wrote:
> On Wed, 5 Mar 2014, Prarit Bhargava wrote:
> 
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index d99f31d..55fab61 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -351,6 +351,7 @@ void fixup_irqs(void)
>>  	struct irq_desc *desc;
>>  	struct irq_data *data;
>>  	struct irq_chip *chip;
>> +	int ret;
>>  
>>  	for_each_irq_desc(irq, desc) {
>>  		int break_affinity = 0;
>> @@ -389,8 +390,12 @@ void fixup_irqs(void)
>>  		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
>>  			chip->irq_mask(data);
>>  
>> -		if (chip->irq_set_affinity)
>> -			chip->irq_set_affinity(data, affinity, true);
>> +		if (chip->irq_set_affinity) {
>> +			ret = chip->irq_set_affinity(data, affinity, true);
>> +			WARN(ret == -ENOSPC,
>> +			     "IRQ %d set affinity failed with %d.  The device assigned to this IRQ is unstable.\n",
>> +			     irq, ret);
> 
> Should this be WARN_ON_ONCE() to avoid filling the kernel log instead?

The problem is that it could hit multiple IRQs ... maybe pr_crit might be better
here so we don't flood the log with an unnecessary stack trace; anyone with the
source can figure out what the call path is.


> 
> It doesn't make much sense to print out the negative return value, maybe 
> you meant to print -ret instead?

Heh :)  You're right.  I'll fix that too.

P.

> 
>> +		}
>>  		else if (!(warned++))
>>  			set_affinity = 0;
>>  

Re: [PATCH] x86, fix x86 fixup_irqs() error handling

[Thomas Gleixner]

On Wed, 5 Mar 2014, David Rientjes wrote:

> On Wed, 5 Mar 2014, Prarit Bhargava wrote:
> 
> > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> > index d99f31d..55fab61 100644
> > --- a/arch/x86/kernel/irq.c
> > +++ b/arch/x86/kernel/irq.c
> > @@ -351,6 +351,7 @@ void fixup_irqs(void)
> >  	struct irq_desc *desc;
> >  	struct irq_data *data;
> >  	struct irq_chip *chip;
> > +	int ret;
> >  
> >  	for_each_irq_desc(irq, desc) {
> >  		int break_affinity = 0;
> > @@ -389,8 +390,12 @@ void fixup_irqs(void)
> >  		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
> >  			chip->irq_mask(data);
> >  
> > -		if (chip->irq_set_affinity)
> > -			chip->irq_set_affinity(data, affinity, true);
> > +		if (chip->irq_set_affinity) {
> > +			ret = chip->irq_set_affinity(data, affinity, true);
> > +			WARN(ret == -ENOSPC,
> > +			     "IRQ %d set affinity failed with %d.  The device assigned to this IRQ is unstable.\n",
> > +			     irq, ret);
> 
> Should this be WARN_ON_ONCE() to avoid filling the kernel log instead?
> 
> It doesn't make much sense to print out the negative return value, maybe 
> you meant to print -ret instead?

Well, that does not make sense either. We only print if ret == -ENOSPC!
 
> > +		}
> >  		else if (!(warned++))
> >  			set_affinity = 0;
> >  
> 

[PATCH] x86, fix x86 fixup_irqs() error handling [v2]

[Prarit Bhargava]

fixup_irqs() calls chip->set_irq_affinity which eventually calls
__assign_irq_vector().  Errors are not propogated back from this function call
and this results in silent irq relocation failures.  This patch fixes this
issue and prints out a warning if there is a relocation failure.

[v2]: modified WARN to pr_crit and changed message

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Rui Wang <rui.y.wang@intel.com>
Cc: Liu Ping Fan <kernelfans@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Cc: Yang Zhang <yang.z.zhang@Intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Cc: Li Fei <fei.li@intel.com>
Cc: gong.chen@linux.intel.com
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 arch/x86/kernel/apic/io_apic.c |   28 ++++++++++++++++++----------
 arch/x86/kernel/irq.c          |    9 +++++++--
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 6ad4658..b4b21db 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2312,7 +2312,7 @@ int __ioapic_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	int err;
 
 	if (!config_enabled(CONFIG_SMP))
-		return -1;
+		return -EPERM;
 
 	if (!cpumask_intersects(mask, cpu_online_mask))
 		return -EINVAL;
@@ -2343,7 +2343,7 @@ int native_ioapic_set_affinity(struct irq_data *data,
 	int ret;
 
 	if (!config_enabled(CONFIG_SMP))
-		return -1;
+		return -EPERM;
 
 	raw_spin_lock_irqsave(&ioapic_lock, flags);
 	ret = __ioapic_set_affinity(data, mask, &dest);
@@ -3075,9 +3075,11 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 	struct irq_cfg *cfg = data->chip_data;
 	struct msi_msg msg;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	__get_cached_msi_msg(data->msi_desc, &msg);
 
@@ -3177,9 +3179,11 @@ dmar_msi_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	struct irq_cfg *cfg = data->chip_data;
 	unsigned int dest, irq = data->irq;
 	struct msi_msg msg;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	dmar_msi_read(irq, &msg);
 
@@ -3226,9 +3230,11 @@ static int hpet_msi_set_affinity(struct irq_data *data,
 	struct irq_cfg *cfg = data->chip_data;
 	struct msi_msg msg;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	hpet_msi_read(data->handler_data, &msg);
 
@@ -3295,9 +3301,11 @@ ht_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 {
 	struct irq_cfg *cfg = data->chip_data;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	target_ht_irq(data->irq, dest, cfg->vector);
 	return IRQ_SET_MASK_OK_NOCOPY;
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index d99f31d..32a351d 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -351,6 +351,7 @@ void fixup_irqs(void)
 	struct irq_desc *desc;
 	struct irq_data *data;
 	struct irq_chip *chip;
+	int ret;
 
 	for_each_irq_desc(irq, desc) {
 		int break_affinity = 0;
@@ -389,8 +390,12 @@ void fixup_irqs(void)
 		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
 			chip->irq_mask(data);
 
-		if (chip->irq_set_affinity)
-			chip->irq_set_affinity(data, affinity, true);
+		if (chip->irq_set_affinity) {
+			ret = chip->irq_set_affinity(data, affinity, true);
+			if (ret == -ENOSPC)
+				pr_crit("IRQ %d set affinity failed because there are no available vectors.  The device assigned to this IRQ is unstable.\n",
+					irq);
+		}
 		else if (!(warned++))
 			set_affinity = 0;
 
-- 
1.7.9.3

Re: [PATCH] x86, fix x86 fixup_irqs() error handling [v2]

[Ingo Molnar]

* Prarit Bhargava <prarit@redhat.com> wrote:

> fixup_irqs() calls chip->set_irq_affinity which eventually calls
> __assign_irq_vector().  Errors are not propogated back from this function call
> and this results in silent irq relocation failures.  This patch fixes this
> issue and prints out a warning if there is a relocation failure.
> 
> [v2]: modified WARN to pr_crit and changed message

Please fix the changelog to conform to the standard changelog style:

 - first describe the symptoms of the bug - how does a user notice? 

 - then describe how the code behaves today and how that is causing the bug

 - and then only describe how it's fixed.

The first item is the most important one - yet it's missing from this 
changelog.

Thanks,

	Ingo

[PATCH] x86, fix x86 fixup_irqs() error handling [v3]

[Prarit Bhargava]

Several patches to fix cpu hotplug and the down'd cpu's irq relocations
have been submitted in the past month or so.  The patches should resolve
the problems with cpu hotplug and irq relocation, however, there is
always a possibility that a bug still exists.  The big problem with
debugging these irq reassignments is that the cpu down completes and then
we get random stack traces from drivers for which irqs have not been properly
assigned to a new cpu.  The stack traces are a mix of storage, network,
and other kernel subsystem (I once saw the serial port stop working ...)
warnings and failures.

The problem with these failures is that they are difficult to diagnose.
There is no warning in the cpu hotplug down path to indicate that an IRQ
has failed to be assigned to a new cpu, and all we are left with is a
stack trace from a driver, or a non-functional device.  If we had some
information on the console debugging these situations would be much
easier; after all we can map an IRQ to a device by simply using lspci or
/proc/interrupts.

The current code, fixup_irqs(), which migrates IRQs from the down'd cpu
and is called close to the end of the cpu down path, calls
chip->set_irq_affinity which eventually calls __assign_irq_vector().
Errors are not propogated back from this function call and this
results in silent irq relocation failures.

This patch fixes this issue by returning the error codes up the call
stack and prints out a warning if there is a relocation failure.

[v2]: modified WARN to pr_crit and changed message
[v3]: updated changelog to conform to standard changelog style

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Rui Wang <rui.y.wang@intel.com>
Cc: Liu Ping Fan <kernelfans@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Cc: Yang Zhang <yang.z.zhang@Intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Cc: Li Fei <fei.li@intel.com>
Cc: gong.chen@linux.intel.com
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 arch/x86/kernel/apic/io_apic.c |   28 ++++++++++++++++++----------
 arch/x86/kernel/irq.c          |    9 +++++++--
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 6ad4658..b4b21db 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2312,7 +2312,7 @@ int __ioapic_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	int err;
 
 	if (!config_enabled(CONFIG_SMP))
-		return -1;
+		return -EPERM;
 
 	if (!cpumask_intersects(mask, cpu_online_mask))
 		return -EINVAL;
@@ -2343,7 +2343,7 @@ int native_ioapic_set_affinity(struct irq_data *data,
 	int ret;
 
 	if (!config_enabled(CONFIG_SMP))
-		return -1;
+		return -EPERM;
 
 	raw_spin_lock_irqsave(&ioapic_lock, flags);
 	ret = __ioapic_set_affinity(data, mask, &dest);
@@ -3075,9 +3075,11 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 	struct irq_cfg *cfg = data->chip_data;
 	struct msi_msg msg;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	__get_cached_msi_msg(data->msi_desc, &msg);
 
@@ -3177,9 +3179,11 @@ dmar_msi_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	struct irq_cfg *cfg = data->chip_data;
 	unsigned int dest, irq = data->irq;
 	struct msi_msg msg;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	dmar_msi_read(irq, &msg);
 
@@ -3226,9 +3230,11 @@ static int hpet_msi_set_affinity(struct irq_data *data,
 	struct irq_cfg *cfg = data->chip_data;
 	struct msi_msg msg;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	hpet_msi_read(data->handler_data, &msg);
 
@@ -3295,9 +3301,11 @@ ht_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force)
 {
 	struct irq_cfg *cfg = data->chip_data;
 	unsigned int dest;
+	int ret;
 
-	if (__ioapic_set_affinity(data, mask, &dest))
-		return -1;
+	ret = __ioapic_set_affinity(data, mask, &dest);
+	if (ret)
+		return ret;
 
 	target_ht_irq(data->irq, dest, cfg->vector);
 	return IRQ_SET_MASK_OK_NOCOPY;
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index d99f31d..32a351d 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -351,6 +351,7 @@ void fixup_irqs(void)
 	struct irq_desc *desc;
 	struct irq_data *data;
 	struct irq_chip *chip;
+	int ret;
 
 	for_each_irq_desc(irq, desc) {
 		int break_affinity = 0;
@@ -389,8 +390,12 @@ void fixup_irqs(void)
 		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
 			chip->irq_mask(data);
 
-		if (chip->irq_set_affinity)
-			chip->irq_set_affinity(data, affinity, true);
+		if (chip->irq_set_affinity) {
+			ret = chip->irq_set_affinity(data, affinity, true);
+			if (ret == -ENOSPC)
+				pr_crit("IRQ %d set affinity failed because there are no available vectors.  The device assigned to this IRQ is unstable.\n",
+					irq);
+		}
 		else if (!(warned++))
 			set_affinity = 0;
 
-- 
1.7.9.3