Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

["Liuqiming (John)" <john.liuqiming@huawei.com>]

On 2015/9/23 21:41, Zhang, Yang Z wrote:
> Hanweidong (Randy) wrote on 2015-09-23:
>>
>> Zhang, Yang Z wrote on ent: 2015年9月23日 11:51:
>>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>>>
>>> Zhang, Yang Z wrote on 2015-09-08:
>>>> Liuqiming (John) wrote on 2015-09-08:
>>>>> Ok, I will try to explain, correct me if I got anything wrong:
>>>>>
>>>>> The problem here is not interrupts lost but interrupts not
>>>>> delivered in time.
>>>>>
>>>>> there are basically two path to inject an interrupt into VM  (or
>>>>> vCPU to another vCPU):
>>>>> Path 1, the traditional way:
>>>>>         1) set bit  in vlapic IRR field which represent an interrupt,
>>>>>         then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>>>>         VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>>>> return
>>> and
>>>>>         do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target
>> vcpu
>>> 5)
>>>>>         target vcpu will VMEXIT due to
>>>>> EXIT_REASON_EXTERNAL_INTERRUPT
>>> 6)
>>>>>         the interrupt handler basically do nothing 7) interrupt in IRR
>>>>>         will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>>>>         do_softirq 9) there will be an interrupt inject into vcpu when
>>>>>         VMENTRY Path 2, the Posted-interrupt way (current logic):
>>>>> 1)
>>> set
>>>>>         bit in posted-interrupt descriptor which represent an
>>>>>         interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>>>>>         otherwise return and do nothing 3) send an
>>>>>         POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if
>>>>>         target vcpu in non-ROOT mode it will receive the interrupt
>>>>> immediately otherwise interrupt will be injected when VMENTRY
>>>>>
>>>>> As the first operation in both path is setting a interrupt
>>>>> represent bit, so no interrupts will lost.
>>>>>
>>>>> The issue is:
>>>>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set
>>>>> to 1, and unless a VMEXIT occured or somewhere called do_softirq
>>>>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>>>>> later interrupts injection  ignored at step 2), which will delay
>>>>> irq handler process in VM.
>>>>>
>>>>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu
>>>>> logic in path 1 will also return in step 3), which make this vcpu
>>>>> only can handle interrupt when some other reason cause VMEXIT.
>>>> Nice catch! But without set the softirq, the interrupt delivery also
>>>> will be delay. Look at the code in vmx_do_vmentry:
>>>>
>>>> cli
>>>>          <---------------the virtual interrupt occurs here will be
>>>> delay. Because there is no softirq pending and the following
>>>> posted interrupt may consumed by another running VCPU, so the
>>>> target VCPU will not aware there is pending virtual interrupt before next vmexit.
>>>> cmp  %ecx,(%rdx,%rax,1)
>>>> jnz  .Lvmx_process_softirqs
>>>>
>>>> I need more time to think it.
>>> Hi Qiming,
>>>
>>> Can you help to take a look at this patch? Also, if possible, please
>>> help to do a testing since I don't have machine to test it. Thanks
>>> very much.
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/entry.S
>>> b/xen/arch/x86/hvm/vmx/entry.S index 664ed83..1ebb5d0 100644
>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>>>           call vmx_enter_realmode
>>>   UNLIKELY_END(realmode)
>>> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
>>> +        jc   .Lvmx_do_vmentry
>>> +
>>>           mov  %rsp,%rdi
>>>           call vmx_vmenter_helper
>>>           mov  VCPU_hvm_guest_cr2(%rbx),%rax diff --git
>>> a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index
>>> e0e9e75..315ce65 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1678,8 +1678,9 @@ static void
>>> __vmx_deliver_posted_interrupt(struct
>>> vcpu *v)
>>>       {
>>>           unsigned int cpu = v->processor;
>>> -        if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ,
>>> &softirq_pending(cpu))
>>> -             && (cpu != smp_processor_id()) )
>>> +        if ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>> Why need pi_test_on() here?
> on bit is cleared means the interrupt is consumed by target VCPU already. So there is no need to send the PI.
>
> Best regards,
> Yang
>
Hi Yang,

I had a question about the "outstanding-notification" bit 
(POSTED_INTR_ON)  handling.
It's not very clear in Intel manual. From what I learned, this bit is 
set by software
when send an interrupt to vcpu and cleared by hardware when interrupt 
delivered, right?

from the source code, there is a test_and_set op for this bit in 
function vmx_deliver_posted_intr,

else if ( !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
     {
         __vmx_deliver_posted_interrupt(v);
         return;
     }

so when we enter __vmx_deliver_posted_interrupt to send ipi, this bit is 
already set on, the "pi_test_on" code is meaningless.

And another thought, the clear bit action performed only when you send a 
ipi to physical cpu or sync_pir_to_irr,
there has a chance the bit is set on and physical cpu not receive a ipi, 
for example

1) [CPU1] vcpu kicked and VCPU_KICK_SOFTIRQ is set
2) [CPU1] find the highest irr where call sync_pir_to_irr to clear 
POSTED_INTR_ON
3) [CPU2] a posted interrupt delivered to CPU1, it will set 
POSTED_INTR_ON on
4) [CPU2] in __vmx_deliver_posted_interrupt will test VCPU_KICK_SOFTIRQ 
on CPU1 which is set by 1), so no ipi sent

At this point, CPU1's POSTED_INTR_ON is on but not interrupt is 
received. and until next kick on CPU1,
CPU1 will not allow posted interrupt get in.

In my opinion, if we want best performance, it should just remove the 
VCPU_KICK_SOFTIRQ test in posted-interrupt function,
or at least we should change the order of test VCPU_KICK_SOFTIRQ and 
test POSTED_INTR_ON





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel