Re: [Xen-devel] [PATCH v2] x86/apicv: enhance posted-interrupt processing

["Xuquan (Quan Xu)" <xuquan8@xxxxxxxxxx>]

On March 01, 2017 2:24 PM, wrote:
>On Wed, Mar 01, 2017 at 12:38:39AM -0700, Jan Beulich wrote:
>>>>> On 01.03.17 at 04:23, <xuquan8@xxxxxxxxxx> wrote:
>>> On February 28, 2017 11:08 PM, Jan Beulich wrote:
>>>>>>> On 27.02.17 at 11:53, <xuquan8@xxxxxxxxxx> wrote:
>>>>> If guest is already in non-root mode, an posted interrupt will be
>>>>> directly delivered to guest (leaving softirq being set without
>>>>> actually incurring a VM-Exit - breaking desired softirq behavior).
>>>>
>>>>This is irritating - are you describing a problem you mean to fix
>>>>with this patch, without making clear what the fix is? Or are you
>>>>describing state after this patch (which the code below suggests), in
>>>>which case I have to say no, we certainly don't want this.
>>>>
>>>
>>> Sorry. I knew you would not like any assumption in patch description..
>>> But I think this one really help community understand what the
>>> problem is( this is also valuable).
>>> Also, as you know, I am a clumsy in patch description and always open
>>> to your suggestion.
>>> :) please don't be irritated.. if you have a better description, I am
>>> always open to it.
>>
>>Well, the problem here is that a suitable patch description is vital to
>>understand what was wrong and why the new behavior is what we want.
>>Hence you really need to view me as the consumer of it, i.e.
>>you can't rely on me to describe your change and/or findings.
>>
>>>>> Then further posted interrupts will skip the IPI, stay in PIR and
>>>>> not noted until another VM-Exit happens.
>>>>>
>>>>> When target vCPU is in non-root mode and running on remote CPU,
>>>>> posted way is triggered to inject interrupt without VM-Exit.
>>>>> Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until
>>>>> another VM-Entry as a best effort.
>>>>
>>>>Furthermore you talking about non-root mode here doesn't fit with the
>>>>code change, as you can't find out whether the guest is in non-root mode.
>>>>
>>>
>>> Reply in below..
>>>
>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>>> @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct
>>>>> vcpu *v)
>>>>>
>>>>>  static void __vmx_deliver_posted_interrupt(struct vcpu *v)  {
>>>>> -    bool_t running = v->is_running;
>>>>> +    unsigned int cpu = v->processor;
>>>>>
>>>>>      vcpu_unblock(v);
>>>>> -    if ( running && (in_irq() || (v != current)) )
>>>>> -    {
>>>>> -        unsigned int cpu = v->processor;
>>>>> -
>>>>> -        if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ,
>>>>&softirq_pending(cpu))
>>>>> -             && (cpu != smp_processor_id()) )
>>>>> -            send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>>> -    }
>>>>> +    if ( v->is_running && (in_irq() || (v != current)) &&
>>>>> +         (cpu != smp_processor_id()) )
>>>>> +        send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>>> +    else
>>>>> +        set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu));
>>>>
>>>>Why don't you simply call raise_softirq() here?
>>>>
>>>
>>> raise_softirq() is a better wrapper, but I didn't get it until you told me.
>>>
>>>
>>>>Plus, if we already want to fix this and _eliminate_ (rather than
>>>>shrink) any time windows, is namely looking at v->is_running really
>>>>useful here? By the time you do the next part of the conditional (or
>>>>call into
>>>>send_IPI_mask()) that may already have changed.
>>>
>>> So far, I can't find a better one to check whether the vcpu is in non-root 
>>> or
>not.
>>> Any suggestion?
>>
>>As said before - you can't find out easily, and if you managed to, by
>>the time you look at the result that result might be stale. Hence the
>>problem and/or patch description should be written in different terms.
>>You may, for example, explain (if that's the case, of course) ...
>>
>>> I am afraid we could not eliminate any time windows, but try our best.
>>
>>... that v->is_running set covers a superset of the time the vCPU
>>spends in non-root mode, with it being acceptable to _also_ do the same
>>actions if the flag is set but the vCPU is in root mode. In such a
>>scenario there would indeed be no time window left. But as I continue
>>to not fully understand you intentions, I can't judge whether this
>>applies here.
>>
>>>>Similarly, while this isn't
>>>>something you change, I don't really understand the relevance of
>>>>using in_irq() here. Hence at the very least a code comment should be
>>>>added imo, clarifying what all this magic is about.
>>>>
>>>
>>> Gone through the code, in_irq() means that the cpu is dispatching an
>>> interrupt..
>>> I am really hesitated whether to drop ' (in_irq() || (v != current))
>>> ' or not in v2, but I found there is a same one in vcpu_kick()..
>>> So I leave it as is for further discussion. Now I tend to drop it.
>>
>>Well, it being that way in vcpu_kick() rather suggests that you also
>>want the same thing here - after all this looks to be an open-coded
>>slight variation of that function. _That's_ likely what is missing to
>>be said here in a comment (and you wouldn't even need to repeat any of
>>the commentary there [which sadly looks to be somewhat stale], but
>>simply refer to it). However, this also points out that your local
>>variable are likely wrong:
>>v->is_running wants evaluating before calling vcpu_pause(),
>>while v->processor wants to be evaluated only after the call.
>>
>>The main thing to explain then is why v->processor changing after
>>having evaluated it is not a problem. While relatively obvious in
>>vcpu_kick() - the field changing means the vCPU is running, and getting
>>it to run is all vcpu_kick() wants to achieve -, the goal here -
>>avoiding to miss delivering an interrupt - is different, and so is
>>rationalizing the correctness of the code.
>
>Good point. I ignore v->processor maybe change. I have thought over
> __vmx_deliver_posted_interrupt() again and want to share you my opinion.
>First of all, __vmx_deliver_posted_interrupt() is to let the target vCPU sync
>PIR to vIRR ASAP.
>different strategies we will used to deal with different cases.
>One is we just unblock the target vCPU when the vCPU is blocked. This can
>make sure the vCPU will go to  vmx_intr_assist() where we achieve the goal.
>The second one is the vCPU is runnable, we will achieve the goal automatically
>when the vCPU is chosen to run.
>The third one is the vCPU is running and running on the same pCPU with the
>source vCPU. It just wants to notify itself. Just raise a softirq is enough.
>The fourth one is the vCPU is running on other pCPU. To notify the target
>vCPU, we send a IPI to the target PCPU which the vCPU is on. Note that when
>the notification arrives to the target PCPU, the target vCPU maybe is blocked,
>runnable, running in root mode, or running in non-root mode. If the target
>vCPU is running in non-root mode, hardware will sync PIR to vIRR. If the target
>vCPU is in non-root mode, the Interrupt handler starts to run. To make sure,
>we can go back to vmx_intr_assist(), I have suggested that the interrupt
>handler should raise a softirq.

Does the interrupt handler refer to pi_notification_interrupt() or 
event_check_interrupt()?

Quan


> If the target vCPU is runnable, we will raise a
>softirq to a wrong vCPU. Maybe it isn't a big issue. If the target vCPU is
>blocked, since before we block a vCPU we will check events through
>local_events_need_delivery() , the goal must has been achieved. Also incur a
>IPI and softirq to a wrong vCPU.
>
>Combining with Jan's explaination about why v->processor changing is not a
>problem, I think we handle all the possible cases. Please let me know if there
>is something wrong or not clear.
>
>Thanks,
>Chao
>
>>
>>Jan

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