Re: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

Re: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Srivatsa S. Bhat]

On 09/26/2012 08:02 PM, Chuansheng Liu wrote:
> 
> When one CPU is going offline, and fixup_irqs() will re-set the
> irq affinity in some cases, we should clean the offlining CPU from
> the irq affinity.
> 
> The reason is setting offlining CPU as of the affinity is useless.
> Moreover, the smp_affinity value will be confusing when the
> offlining CPU come back again.
> 
> Example:
> For irq 93 with 4 CPUS, the default affinity f(1111),
> normal cases: 4 CPUS will receive the irq93 interrupts.
> 
> When echo 0 > /sys/devices/system/cpu/cpu3/online, just CPU0,1,2 will
> receive the interrupts.
> 
> But after the CPU3 is online again, we will not set affinity,the result
> will be:
> the smp_affinity is f, but still just CPU0,1,2 can receive the interrupts.
> 
> So we should clean the offlining CPU from irq affinity mask
> in fixup_irqs().
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  arch/x86/kernel/irq.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index d44f782..671d462 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -239,6 +239,7 @@ void fixup_irqs(void)
>  	struct irq_desc *desc;
>  	struct irq_data *data;
>  	struct irq_chip *chip;
> +	int cpu = smp_processor_id();
> 
>  	for_each_irq_desc(irq, desc) {
>  		int break_affinity = 0;
> @@ -271,7 +272,8 @@ void fixup_irqs(void)
>  		if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
>  			break_affinity = 1;
>  			affinity = cpu_online_mask;
> -		}
> +		} else if (cpumask_test_cpu(cpu, data->affinity))
> +			cpumask_clear_cpu(cpu, data->affinity);
> 

You meant to use 'affinity' (instead of data->affinity) in the above 2 statements
right? Note that we do chip->irq_set_affinity(data, affinity, true); further down.

Regards,
Srivatsa S. Bhat

>  		chip = irq_data_get_irq_chip(data);
>  		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
> 

RE: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 741 bytes --]

> > +		} else if (cpumask_test_cpu(cpu, data->affinity))
> > +			cpumask_clear_cpu(cpu, data->affinity);
> >
> 
> You meant to use 'affinity' (instead of data->affinity) in the above 2 statements
> right? Note that we do chip->irq_set_affinity(data, affinity, true); further down.
> 

Yes, I have noticed it, used data->affinity here is just for avoiding compile warning.
in fact affinity == data->affinity, but affinity pointer is const type,
And cpumask_clear_cpu needs non-const type,so here I am using data->affinity,
instead of changing code "const struct cpumask *affinity;"
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Srivatsa S. Bhat]

On 09/26/2012 12:22 PM, Liu, Chuansheng wrote:
>>> +		} else if (cpumask_test_cpu(cpu, data->affinity))
>>> +			cpumask_clear_cpu(cpu, data->affinity);
>>>
>>
>> You meant to use 'affinity' (instead of data->affinity) in the above 2 statements
>> right? Note that we do chip->irq_set_affinity(data, affinity, true); further down.
>>
> 
> Yes, I have noticed it, used data->affinity here is just for avoiding compile warning.
> in fact affinity == data->affinity, but affinity pointer is const type,
> And cpumask_clear_cpu needs non-const type,so here I am using data->affinity,
> instead of changing code "const struct cpumask *affinity;"
> 

Hmm.. Then what happens to error handling in the case that you can't set
the affinity?

ie., if we take the 'else' branch in:
                if (chip->irq_set_affinity)
                        chip->irq_set_affinity(data, affinity, true);
                else if (!(warned++))
                        set_affinity = 0;

In that case, we would end up with an incorrect data->affinity right?

Btw, on a slightly different note, I'm also rather surprised that the above
code doesn't care about the return value of chip->irq_set_affinity() ..
Shouldn't we warn if that fails?

Regards,
Srivatsa S. Bhat

RE: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 691 bytes --]

> Btw, on a slightly different note, I'm also rather surprised that the above
> code doesn't care about the return value of chip->irq_set_affinity() ..
> Shouldn't we warn if that fails?

It seems another case when irq_set_affinity is NULL whenever affinity is changed or not before that,
For this case, I suppose the chip is not supporting set_affinity, then the chip should set all
related irqs into just CPU0, otherwise, it will bring some trouble, do you agree?

I guess this case should be covered outside fixup_irqs() code.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 290 bytes --]

> Shouldn't we warn if that fails?
printk("Cannot set affinity for irq %i\n", irq);
This is the warning when set affinity failed.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 400 bytes --]

> In that case, we would end up with an incorrect data->affinity right?
> 
Moving the clean cpu mask code into if (chip->irq_set_affinity)?
Will resend the patch and will judge the chip->irq_set_affinity(data, affinity, true)
return value.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Srivatsa S. Bhat]

On 09/26/2012 01:47 PM, Liu, Chuansheng wrote:
>> Shouldn't we warn if that fails?
> printk("Cannot set affinity for irq %i\n", irq);
> This is the warning when set affinity failed.
> 

I know.. What I meant is, the code warns only if chip->irq_set_affinity
is NULL and doesn't care if chip->irq_set_affinity was not NULL and
the function failed to set the affinity (ie., when chip->irq_set_affinity()
returns error). In other words, I meant to say that this is one more
case where we need to warn about our failure to set the irq affinity. 
 
Regards,
Srivatsa S. Bhat

RE: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 592 bytes --]

> I know.. What I meant is, the code warns only if chip->irq_set_affinity
> is NULL and doesn't care if chip->irq_set_affinity was not NULL and
> the function failed to set the affinity (ie., when chip->irq_set_affinity()
> returns error). In other words, I meant to say that this is one more
> case where we need to warn about our failure to set the irq affinity.
> 
Got it, has resent the patch to include it, thanks your review.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Srivatsa S. Bhat]

On 09/26/2012 01:40 PM, Liu, Chuansheng wrote:
>> Btw, on a slightly different note, I'm also rather surprised that the above
>> code doesn't care about the return value of chip->irq_set_affinity() ..
>> Shouldn't we warn if that fails?
> 
> It seems another case when irq_set_affinity is NULL whenever affinity is changed or not before that,
> For this case, I suppose the chip is not supporting set_affinity, then the chip should set all
> related irqs into just CPU0, otherwise, it will bring some trouble, do you agree?
> 

Hmm.. no, I wouldn't jump to do that. Moreover, note that there are patches in -tip to
enable CPU 0 hotplug. So doing the above might not be terribly helpful going forward.

> I guess this case should be covered outside fixup_irqs() code.
> 
 
Regards,
Srivatsa S. Bhat

[PATCH] x86/fixup_irq: Clean the offlining CPU from the irq affinity mask

[Chuansheng Liu]

When one CPU is going offline, and fixup_irqs() will re-set the
irq affinity in some cases, we should clean the offlining CPU from
the irq affinity.

The reason is setting offlining CPU as of the affinity is useless.
Moreover, the smp_affinity value will be confusing when the
offlining CPU come back again.

Example:
For irq 93 with 4 CPUS, the default affinity f(1111),
normal cases: 4 CPUS will receive the irq93 interrupts.

When echo 0 > /sys/devices/system/cpu/cpu3/online, just CPU0,1,2 will
receive the interrupts.

But after the CPU3 is online again, we will not set affinity,the result
will be:
the smp_affinity is f, but still just CPU0,1,2 can receive the interrupts.

So we should clean the offlining CPU from irq affinity mask
in fixup_irqs().

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 arch/x86/kernel/irq.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index d44f782..671d462 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -239,6 +239,7 @@ void fixup_irqs(void)
 	struct irq_desc *desc;
 	struct irq_data *data;
 	struct irq_chip *chip;
+	int cpu = smp_processor_id();
 
 	for_each_irq_desc(irq, desc) {
 		int break_affinity = 0;
@@ -271,7 +272,8 @@ void fixup_irqs(void)
 		if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
 			break_affinity = 1;
 			affinity = cpu_online_mask;
-		}
+		} else if (cpumask_test_cpu(cpu, data->affinity))
+			cpumask_clear_cpu(cpu, data->affinity);
 
 		chip = irq_data_get_irq_chip(data);
 		if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
-- 
1.7.0.4