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

[Chao Gao <chao.gao@xxxxxxxxx>]

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