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

[George Dunlap <George.Dunlap@eu.citrix.com>]

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?

> +            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?

>      }
>
>     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
>