* [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
@ 2012-09-27 14:50 Jan Beulich
2012-09-27 14:57 ` Andrew Cooper
2012-09-27 15:29 ` Keir Fraser
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2012-09-27 14:50 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3
[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]
There are two greater-than-zero checks for the old vector retrieved,
which don't work when a negative value got stashed into the respective
arch_irq_desc field. The effect of this was that for interrupts that
are intended to get their affinity adjusted the first time before the
first interrupt occurs, the affinity change would fail, because the
original vector assignment would have caused the move_in_progress flag
to get set (which causes subsequent re-assignments to fail until it
gets cleared, which only happens from the ->ack() actor, i.e. when an
interrupt actually occurred).
This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I have to admit that I don't understand why the value got changed in
the first place: 0 is as invalid a value as -1 for a vector to be used
for delivering hardware interrupts.
--- 2012-09-21.orig/xen/arch/x86/irq.c 2012-09-19 08:48:33.000000000 +0200
+++ 2012-09-21/xen/arch/x86/irq.c 2012-09-27 13:33:45.000000000 +0200
@@ -430,8 +430,7 @@ static int __assign_irq_vector(
* 0x80, because int 0x80 is hm, kind of importantish. ;)
*/
static int current_vector = FIRST_DYNAMIC_VECTOR, current_offset = 0;
- unsigned int old_vector;
- int cpu, err;
+ int cpu, err, old_vector;
cpumask_t tmp_mask;
vmask_t *irq_used_vectors = NULL;
[-- Attachment #2: x86-assign-irq-vector-old.patch --]
[-- Type: text/plain, Size: 1514 bytes --]
x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
There are two greater-than-zero checks for the old vector retrieved,
which don't work when a negative value got stashed into the respective
arch_irq_desc field. The effect of this was that for interrupts that
are intended to get their affinity adjusted the first time before the
first interrupt occurs, the affinity change would fail, because the
original vector assignment would have caused the move_in_progress flag
to get set (which causes subsequent re-assignments to fail until it
gets cleared, which only happens from the ->ack() actor, i.e. when an
interrupt actually occurred).
This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I have to admit that I don't understand why the value got changed in
the first place: 0 is as invalid a value as -1 for a vector to be used
for delivering hardware interrupts.
--- 2012-09-21.orig/xen/arch/x86/irq.c 2012-09-19 08:48:33.000000000 +0200
+++ 2012-09-21/xen/arch/x86/irq.c 2012-09-27 13:33:45.000000000 +0200
@@ -430,8 +430,7 @@ static int __assign_irq_vector(
* 0x80, because int 0x80 is hm, kind of importantish. ;)
*/
static int current_vector = FIRST_DYNAMIC_VECTOR, current_offset = 0;
- unsigned int old_vector;
- int cpu, err;
+ int cpu, err, old_vector;
cpumask_t tmp_mask;
vmask_t *irq_used_vectors = NULL;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
2012-09-27 14:50 [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector() Jan Beulich
@ 2012-09-27 14:57 ` Andrew Cooper
2012-09-27 15:04 ` Jan Beulich
2012-09-27 15:29 ` Keir Fraser
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2012-09-27 14:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 27/09/12 15:50, Jan Beulich wrote:
> There are two greater-than-zero checks for the old vector retrieved,
> which don't work when a negative value got stashed into the respective
> arch_irq_desc field. The effect of this was that for interrupts that
> are intended to get their affinity adjusted the first time before the
> first interrupt occurs, the affinity change would fail, because the
> original vector assignment would have caused the move_in_progress flag
> to get set (which causes subsequent re-assignments to fail until it
> gets cleared, which only happens from the ->ack() actor, i.e. when an
> interrupt actually occurred).
>
> This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
> changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I have to admit that I don't understand why the value got changed in
> the first place: 0 is as invalid a value as -1 for a vector to be used
> for delivering hardware interrupts.
http://lists.xen.org/archives/html/xen-devel/2011-09/msg00193.html
It was a suggestion for consistency with using -1 elsewhere in the irq
code to mean unassigned.
~Andrew
>
> --- 2012-09-21.orig/xen/arch/x86/irq.c 2012-09-19 08:48:33.000000000 +0200
> +++ 2012-09-21/xen/arch/x86/irq.c 2012-09-27 13:33:45.000000000 +0200
> @@ -430,8 +430,7 @@ static int __assign_irq_vector(
> * 0x80, because int 0x80 is hm, kind of importantish. ;)
> */
> static int current_vector = FIRST_DYNAMIC_VECTOR, current_offset = 0;
> - unsigned int old_vector;
> - int cpu, err;
> + int cpu, err, old_vector;
> cpumask_t tmp_mask;
> vmask_t *irq_used_vectors = NULL;
>
>
>
>
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
2012-09-27 14:57 ` Andrew Cooper
@ 2012-09-27 15:04 ` Jan Beulich
2012-09-27 15:33 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2012-09-27 15:04 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 27.09.12 at 16:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 27/09/12 15:50, Jan Beulich wrote:
>> There are two greater-than-zero checks for the old vector retrieved,
>> which don't work when a negative value got stashed into the respective
>> arch_irq_desc field. The effect of this was that for interrupts that
>> are intended to get their affinity adjusted the first time before the
>> first interrupt occurs, the affinity change would fail, because the
>> original vector assignment would have caused the move_in_progress flag
>> to get set (which causes subsequent re-assignments to fail until it
>> gets cleared, which only happens from the ->ack() actor, i.e. when an
>> interrupt actually occurred).
>>
>> This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
>> changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I have to admit that I don't understand why the value got changed in
>> the first place: 0 is as invalid a value as -1 for a vector to be used
>> for delivering hardware interrupts.
>
> http://lists.xen.org/archives/html/xen-devel/2011-09/msg00193.html
>
> It was a suggestion for consistency with using -1 elsewhere in the irq
> code to mean unassigned.
Not really - there George suggested to use IRQ_VECTOR_UNASSIGNED,
but not to make that resolve to -1. My claim is that this manifest
constant could easily resolve to zero instead.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
2012-09-27 14:50 [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector() Jan Beulich
2012-09-27 14:57 ` Andrew Cooper
@ 2012-09-27 15:29 ` Keir Fraser
1 sibling, 0 replies; 6+ messages in thread
From: Keir Fraser @ 2012-09-27 15:29 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper
On 27/09/2012 15:50, "Jan Beulich" <JBeulich@suse.com> wrote:
> There are two greater-than-zero checks for the old vector retrieved,
> which don't work when a negative value got stashed into the respective
> arch_irq_desc field. The effect of this was that for interrupts that
> are intended to get their affinity adjusted the first time before the
> first interrupt occurs, the affinity change would fail, because the
> original vector assignment would have caused the move_in_progress flag
> to get set (which causes subsequent re-assignments to fail until it
> gets cleared, which only happens from the ->ack() actor, i.e. when an
> interrupt actually occurred).
>
> This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
> changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> ---
> I have to admit that I don't understand why the value got changed in
> the first place: 0 is as invalid a value as -1 for a vector to be used
> for delivering hardware interrupts.
>
> --- 2012-09-21.orig/xen/arch/x86/irq.c 2012-09-19 08:48:33.000000000 +0200
> +++ 2012-09-21/xen/arch/x86/irq.c 2012-09-27 13:33:45.000000000 +0200
> @@ -430,8 +430,7 @@ static int __assign_irq_vector(
> * 0x80, because int 0x80 is hm, kind of importantish. ;)
> */
> static int current_vector = FIRST_DYNAMIC_VECTOR, current_offset = 0;
> - unsigned int old_vector;
> - int cpu, err;
> + int cpu, err, old_vector;
> cpumask_t tmp_mask;
> vmask_t *irq_used_vectors = NULL;
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
2012-09-27 15:04 ` Jan Beulich
@ 2012-09-27 15:33 ` Andrew Cooper
2012-09-27 16:03 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2012-09-27 15:33 UTC (permalink / raw)
To: xen-devel
On 27/09/12 16:04, Jan Beulich wrote:
>>>> On 27.09.12 at 16:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 27/09/12 15:50, Jan Beulich wrote:
>>> There are two greater-than-zero checks for the old vector retrieved,
>>> which don't work when a negative value got stashed into the respective
>>> arch_irq_desc field. The effect of this was that for interrupts that
>>> are intended to get their affinity adjusted the first time before the
>>> first interrupt occurs, the affinity change would fail, because the
>>> original vector assignment would have caused the move_in_progress flag
>>> to get set (which causes subsequent re-assignments to fail until it
>>> gets cleared, which only happens from the ->ack() actor, i.e. when an
>>> interrupt actually occurred).
>>>
>>> This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
>>> changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I have to admit that I don't understand why the value got changed in
>>> the first place: 0 is as invalid a value as -1 for a vector to be used
>>> for delivering hardware interrupts.
>> http://lists.xen.org/archives/html/xen-devel/2011-09/msg00193.html
>>
>> It was a suggestion for consistency with using -1 elsewhere in the irq
>> code to mean unassigned.
> Not really - there George suggested to use IRQ_VECTOR_UNASSIGNED,
> but not to make that resolve to -1. My claim is that this manifest
> constant could easily resolve to zero instead.
>
> Jan
Ah - it was in the following email.
"Yes - I missed that. However, IRQ_VECTOR_UNASSIGNED should be -1
instead of 0, as the first 32 entries of irq_vector have 0 entries which
are not unassigned."
Which was my justification of using -1 as opposed to 0.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector()
2012-09-27 15:33 ` Andrew Cooper
@ 2012-09-27 16:03 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2012-09-27 16:03 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 27.09.12 at 17:33, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 27/09/12 16:04, Jan Beulich wrote:
>>>>> On 27.09.12 at 16:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 27/09/12 15:50, Jan Beulich wrote:
>>>> There are two greater-than-zero checks for the old vector retrieved,
>>>> which don't work when a negative value got stashed into the respective
>>>> arch_irq_desc field. The effect of this was that for interrupts that
>>>> are intended to get their affinity adjusted the first time before the
>>>> first interrupt occurs, the affinity change would fail, because the
>>>> original vector assignment would have caused the move_in_progress flag
>>>> to get set (which causes subsequent re-assignments to fail until it
>>>> gets cleared, which only happens from the ->ack() actor, i.e. when an
>>>> interrupt actually occurred).
>>>>
>>>> This addresses a problem introduced in c/s 23816:7f357e1ef60a (by
>>>> changing IRQ_VECTOR_UNASSIGNED from 0 to -1).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I have to admit that I don't understand why the value got changed in
>>>> the first place: 0 is as invalid a value as -1 for a vector to be used
>>>> for delivering hardware interrupts.
>>> http://lists.xen.org/archives/html/xen-devel/2011-09/msg00193.html
>>>
>>> It was a suggestion for consistency with using -1 elsewhere in the irq
>>> code to mean unassigned.
>> Not really - there George suggested to use IRQ_VECTOR_UNASSIGNED,
>> but not to make that resolve to -1. My claim is that this manifest
>> constant could easily resolve to zero instead.
>>
>> Jan
>
> Ah - it was in the following email.
>
> "Yes - I missed that. However, IRQ_VECTOR_UNASSIGNED should be -1
> instead of 0, as the first 32 entries of irq_vector have 0 entries which
> are not unassigned."
>
> Which was my justification of using -1 as opposed to 0.
With irq_vector[] not even in existence anymore, I wonder
whether we shouldn't go back to zero.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-27 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-27 14:50 [PATCH] x86/IRQ: fix valid-old-vector checks in __assign_irq_vector() Jan Beulich
2012-09-27 14:57 ` Andrew Cooper
2012-09-27 15:04 ` Jan Beulich
2012-09-27 15:33 ` Andrew Cooper
2012-09-27 16:03 ` Jan Beulich
2012-09-27 15:29 ` Keir Fraser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).