Re: [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/09/11 11:14, George Dunlap wrote:
> Comments inline.
>
> On Fri, Sep 2, 2011 at 5:35 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Introduce old_vector to irq_cfg with the same principle as
>> old_cpu_mask.  This removes a brute force loop from
>> __clear_irq_vector(), and paves the way to correct bitrotten logic
>> elsewhere in the irq code.
>>
>> Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c
>> --- a/xen/arch/x86/io_apic.c    Fri Sep 02 17:33:17 2011 +0100
>> +++ b/xen/arch/x86/io_apic.c    Fri Sep 02 17:33:17 2011 +0100
>> @@ -487,11 +487,16 @@ fastcall void smp_irq_move_cleanup_inter
>>         __get_cpu_var(vector_irq)[vector] = -1;
>>         cfg->move_cleanup_count--;
>>
>> -        if ( cfg->move_cleanup_count == 0
>> -             &&  cfg->used_vectors )
>> +        if ( cfg->move_cleanup_count == 0 )
>>         {
>> -            ASSERT(test_bit(vector, cfg->used_vectors));
>> -            clear_bit(vector, cfg->used_vectors);
>> +            cfg->old_vector = -1;
> Just for consistency, should this be IRQ_VECTOR_UNASSIGNED instead of -1?

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.
 
>> +            cpus_clear(cfg->old_cpu_mask);
>> +
>> +            if ( cfg->used_vectors )
>> +            {
>> +                ASSERT(test_bit(vector, cfg->used_vectors));
>> +                clear_bit(vector, cfg->used_vectors);
>> +            }
>>         }
>>  unlock:
>>         spin_unlock(&desc->lock);
>> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c
>> --- a/xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100
>> +++ b/xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100
>> @@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq)
>>
>>     cpus_and(tmp_mask, cfg->old_cpu_mask, cpu_online_map);
>>     for_each_cpu_mask(cpu, tmp_mask) {
>> -        for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR;
>> -                                vector++) {
>> -            if (per_cpu(vector_irq, cpu)[vector] != irq)
>> -                continue;
>> -            TRACE_3D(TRC_HW_IRQ_MOVE_FINISH,
>> -                     irq, vector, cpu);
>> -            per_cpu(vector_irq, cpu)[vector] = -1;
>> -             break;
>> -        }
>> +        ASSERT( per_cpu(vector_irq, cpu)[cfg->old_vector] == irq );
>> +        TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, vector, cpu);
>> +        per_cpu(vector_irq, cpu)[vector] = -1;
> Do you mean cfg->old_vector here, instead of vector?

No - the TRACE_3D and per_cpu lines are only diffs because of the change
in whitespace when removing the loop (and this is the code which should
actually remove the vector mapping).  You are correct however that
cfg->old_vector should be set to IRQ_VECTOR_UNASSIGNED at the end of the
for_each for consistency.  (In reality, you cant get to this bit of code
without having a valid cfg->old_vector)

>>      }
>>
>>     if ( cfg->used_vectors )
>> @@ -279,6 +273,7 @@ static void __init init_one_irq_desc(str
>>  static void __init init_one_irq_cfg(struct irq_cfg *cfg)
>>  {
>>     cfg->vector = IRQ_VECTOR_UNASSIGNED;
>> +    cfg->old_vector = IRQ_VECTOR_UNASSIGNED;
>>     cpus_clear(cfg->cpu_mask);
>>     cpus_clear(cfg->old_cpu_mask);
>>     cfg->used_vectors = NULL;
>> @@ -418,6 +413,7 @@ next:
>>         if (old_vector) {
>>             cfg->move_in_progress = 1;
>>             cpus_copy(cfg->old_cpu_mask, cfg->cpu_mask);
>> +            cfg->old_vector = cfg->vector;
>>         }
>>         trace_irq_mask(TRC_HW_IRQ_ASSIGN_VECTOR, irq, vector, &tmp_mask);
>>         for_each_cpu_mask(new_cpu, tmp_mask)
>> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/include/asm-x86/irq.h
>> --- a/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
>> +++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
>> @@ -28,7 +28,8 @@ typedef struct {
>>  } vmask_t;
>>
>>  struct irq_cfg {
>> -        int  vector;
>> +        s16 vector;                  /* vector itself is only 8 bits, */
>> +        s16 old_vector;              /* but we use -1 for unassigned  */
>>         cpumask_t cpu_mask;
>>         cpumask_t old_cpu_mask;
>>         unsigned move_cleanup_count;
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com