Re: [PATCH] IRQ: Group IRQ_MOVE_CLEANUP_VECTOR with other hypervisor IPIs

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

On 08/09/11 09:15, Keir Fraser wrote:
> On 08/09/2011 08:39, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>>>>> On 07.09.11 at 18:56, Keir Fraser <keir.xen@gmail.com> wrote:
>>> On 07/09/2011 17:03, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>
>>>>>>> On 07.09.11 at 17:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> Are you sure this is correct? I'm suspicious that this may intentionally
>>>> have been the lowest priority vector...
>>> I can't see why?
>> Perhaps to get all "real" interrupts serviced first, and then do a single,
>> consolidated run through everything that needs cleaning up? All the
>> more since smp_irq_move_cleanup_interrupt() may re-issue the
>> interrupt to the local CPU.
> Ah, hm, that's a good point. We obviously livelock if we make
> IRQ_MOVE_CLEANUP_VECTOR higher priority than the vector that
> smp_irq_move_cleanup_interrupt() is attempting to retry.
>
> Andrew: I think we have to leave this vector where it is, but you could add
> a comment explaining why it is so, in your cleanup patchset.
>
>  -- Keir 

Wow I was having a slow day - I was thinking that
IRQ_MOVE_CLEANUP_VECTOR was the first high priority vector.

In which case it should probably stay at its current vector, but
FIRST_DYNAMIC_VECTOR should probably be bumped up, as it is no longer a
vector dynamically allocated to guests.


>> Cc-ing the original author in the hope that
>> he's still at Intel.
>>
>> And btw., if that change is correct, then the setting of
>> bit IRQ_MOVE_CLEANUP_VECTOR in used_vectors should actually be
>> removed - while used_vectors currently is dimensioned as NR_VECTORS,
>> none of the other "special" vectors get their bits set there, and the bit
>> array could therefore easily be shrunk to just NR_DYNAMIC_VECTORS.
>>
>> Jan
>>
>>>>> Also, rename to MOVE_CLEANUP_VECTOR to be in line with the other IPI
>>>>> names.
>>>> Why would the removal of part of the descriptive name be in line with
>>>> the other names? We're dealing with the cleanup after an IRQ move
>>>> here, so let the name state this. The IRQ_ prefix here has nothing to
>>>> do with this being the vector for a specific IRQ.
>>> Agreed.
>>>
>>>  -- Keir
>>>
>>>> Jan
>>>>
>>>>> This requires bumping LAST_HIPRIORITY_VECTOR, but does mean that the
>>>>> range FIRST-LAST_HIPRIORITY_VECTORs are free once again.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> diff -r 0268e7380953 -r c7884dbb6f7d xen/arch/x86/apic.c
>>>>> --- a/xen/arch/x86/apic.c Mon Sep 05 15:10:28 2011 +0100
>>>>> +++ b/xen/arch/x86/apic.c Wed Sep 07 16:00:55 2011 +0100
>>>>> @@ -90,7 +90,7 @@ bool_t __read_mostly directed_eoi_enable
>>>>>   * through the ICC by us (IPIs)
>>>>>   */
>>>>>  __asm__(".section .text");
>>>>> -BUILD_SMP_INTERRUPT(irq_move_cleanup_interrupt,IRQ_MOVE_CLEANUP_VECTOR)
>>>>> +BUILD_SMP_INTERRUPT(irq_move_cleanup_interrupt,MOVE_CLEANUP_VECTOR)
>>>>>  BUILD_SMP_INTERRUPT(event_check_interrupt,EVENT_CHECK_VECTOR)
>>>>>  BUILD_SMP_INTERRUPT(invalidate_interrupt,INVALIDATE_TLB_VECTOR)
>>>>>  BUILD_SMP_INTERRUPT(call_function_interrupt,CALL_FUNCTION_VECTOR)
>>>>> diff -r 0268e7380953 -r c7884dbb6f7d xen/arch/x86/hvm/vmx/vmx.c
>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c Mon Sep 05 15:10:28 2011 +0100
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Wed Sep 07 16:00:55 2011 +0100
>>>>> @@ -1986,7 +1986,7 @@ static void vmx_do_extint(struct cpu_use
>>>>>  
>>>>>      switch ( vector )
>>>>>      {
>>>>> -    case IRQ_MOVE_CLEANUP_VECTOR:
>>>>> +    case MOVE_CLEANUP_VECTOR:
>>>>>          smp_irq_move_cleanup_interrupt(regs);
>>>>>          break;
>>>>>      case LOCAL_TIMER_VECTOR:
>>>>> diff -r 0268e7380953 -r c7884dbb6f7d xen/arch/x86/io_apic.c
>>>>> --- a/xen/arch/x86/io_apic.c Mon Sep 05 15:10:28 2011 +0100
>>>>> +++ b/xen/arch/x86/io_apic.c Wed Sep 07 16:00:55 2011 +0100
>>>>> @@ -476,7 +476,7 @@ fastcall void smp_irq_move_cleanup_inter
>>>>>           * to myself.
>>>>>           */
>>>>>          if (irr  & (1 << (vector % 32))) {
>>>>> -            genapic->send_IPI_self(IRQ_MOVE_CLEANUP_VECTOR);
>>>>> +            genapic->send_IPI_self(MOVE_CLEANUP_VECTOR);
>>>>>              TRACE_3D(TRC_HW_IRQ_MOVE_CLEANUP_DELAY,
>>>>>                       irq, vector, smp_processor_id());
>>>>>              goto unlock;
>>>>> @@ -513,7 +513,7 @@ static void send_cleanup_vector(struct i
>>>>>  
>>>>>      cpus_and(cleanup_mask, cfg->old_cpu_mask, cpu_online_map);
>>>>>      cfg->move_cleanup_count = cpus_weight(cleanup_mask);
>>>>> -    genapic->send_IPI_mask(&cleanup_mask, IRQ_MOVE_CLEANUP_VECTOR);
>>>>> +    genapic->send_IPI_mask(&cleanup_mask, MOVE_CLEANUP_VECTOR);
>>>>>  
>>>>>      cfg->move_in_progress = 0;
>>>>>  }
>>>>> diff -r 0268e7380953 -r c7884dbb6f7d xen/arch/x86/irq.c
>>>>> --- a/xen/arch/x86/irq.c Mon Sep 05 15:10:28 2011 +0100
>>>>> +++ b/xen/arch/x86/irq.c Wed Sep 07 16:00:55 2011 +0100
>>>>> @@ -338,7 +338,7 @@ int __init init_irq_data(void)
>>>>>      set_bit(HYPERCALL_VECTOR, used_vectors);
>>>>>      
>>>>>      /* IRQ_MOVE_CLEANUP_VECTOR used for clean up vectors */
>>>>> -    set_bit(IRQ_MOVE_CLEANUP_VECTOR, used_vectors);
>>>>> +    set_bit(MOVE_CLEANUP_VECTOR, used_vectors);
>>>>>  
>>>>>      return 0;
>>>>>  }
>>>>> diff -r 0268e7380953 -r c7884dbb6f7d xen/arch/x86/smpboot.c
>>>>> --- a/xen/arch/x86/smpboot.c Mon Sep 05 15:10:28 2011 +0100
>>>>> +++ b/xen/arch/x86/smpboot.c Wed Sep 07 16:00:55 2011 +0100
>>>>> @@ -1027,7 +1027,7 @@ void __init smp_intr_init(void)
>>>>>      }
>>>>>  
>>>>>      /* IPI for cleanuping vectors after irq move */
>>>>> -    set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
>>>>> +    set_intr_gate(MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
>>>>>  
>>>>>      /* IPI for event checking. */
>>>>>      set_intr_gate(EVENT_CHECK_VECTOR, event_check_interrupt);
>>>>> diff -r 0268e7380953 -r c7884dbb6f7d
>>>>> xen/include/asm-x86/mach-default/irq_vectors.h
>>>>> --- a/xen/include/asm-x86/mach-default/irq_vectors.h Mon Sep 05 15:10:28
>>>>> 2011
>>>>> +0100
>>>>> +++ b/xen/include/asm-x86/mach-default/irq_vectors.h Wed Sep 07 16:00:55
>>>>> 2011
>>>>> +0100
>>>>> @@ -11,12 +11,14 @@
>>>>>  #define LOCAL_TIMER_VECTOR 0xf9
>>>>>  #define PMU_APIC_VECTOR  0xf8
>>>>>  #define CMCI_APIC_VECTOR 0xf7
>>>>> +#define MOVE_CLEANUP_VECTOR 0xf6
>>>>> +
>>>>>  /*
>>>>>   * High-priority dynamically-allocated vectors. For interrupts that
>>>>>   * must be higher priority than any guest-bound interrupt.
>>>>>   */
>>>>>  #define FIRST_HIPRIORITY_VECTOR 0xf0
>>>>> -#define LAST_HIPRIORITY_VECTOR  0xf6
>>>>> +#define LAST_HIPRIORITY_VECTOR 0xf5
>>>>>  
>>>>>  /* Legacy PIC uses vectors 0xe0-0xef. */
>>>>>  #define FIRST_LEGACY_VECTOR 0xe0
>>>>> @@ -30,8 +32,6 @@
>>>>>  #define LAST_DYNAMIC_VECTOR 0xdf
>>>>>  #define NR_DYNAMIC_VECTORS (LAST_DYNAMIC_VECTOR - FIRST_DYNAMIC_VECTOR +
>>>>> 1)
>>>>>  
>>>>> -#define IRQ_MOVE_CLEANUP_VECTOR FIRST_DYNAMIC_VECTOR
>>>>> -
>>>>>  #define NR_VECTORS 256
>>>>>  
>>>>>  #endif /* _ASM_IRQ_VECTORS_H */
>>>>>
>>>>> _______________________________________________
>>>>> Xen-devel mailing list
>>>>> Xen-devel@lists.xensource.com
>>>>> http://lists.xensource.com/xen-devel
>>>>
>>>>
>>>> _______________________________________________
>>>> 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