Re: [Xen-devel] [PATCH v3] x86/apicv: Enhance posted-interrupt processing

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Mar 02, 2017 at 02:41:55AM -0700, Jan Beulich wrote:
>>>> On 02.03.17 at 02:49, <chao.gao@xxxxxxxxx> wrote:
>
>The patch title, btw, makes it looks like this isn't a bug fix, which is
>contrary to the understanding I've gained so far.

Thanks to your patience and your changing so much description for me. Also 
to the questions you asked. I agree to the comments i don't reply to.
How about this:
Fix a wrongly IPI suppression during posted interrupt delivery

>
>> __vmx_deliver_posted_interrupt() wrongly used a softirq bit to decide whether
>> to suppress an IPI. Its logic was: the first time an IPI was sent, we set
>> the softirq bit. Next time, we would check that softirq bit before sending
>> another IPI. If the 1st IPI arrived at the pCPU which was in
>> non-root mode, the hardware would consume the IPI and sync PIR to vIRR.
>> During the process, no one (both hardware and software) will clear the
>> softirq bit. As a result, the following IPI would be wrongly suppressed.
>> 
>> This patch discards the suppression check, always sending IPI.
>> The softirq also need to be raised. But there is a little change.
>> This patch moves the place where we raise a softirq for
>> 'cpu != smp_processor_id()' case to the IPI interrupt handler.
>> Namely, don't raise a softirq for this case and set the interrupt handler
>> to pi_notification_interrupt()(in which a softirq is raised) regardless of
>> posted interrupt enabled or not.
>
>As using a PI thing even in the non-PI case is counterintuitive, this
>needs expanding on imo. Maybe not here but in a code comment.
>Or perhaps, looking at the code, this is just not precise enough a
>description: The code is inside a cpu_has_vmx_posted_intr_processing
>conditional, and what you do is move code out of the iommu_intpost
>specific section. I.e. a reference to IOMMU may simply be missing
>here.

Yes. The right description is "regardless of VT-d PI enabled or not".

>
>> The only difference is when the IPI arrives
>> at the pCPU which is happened in non-root mode, the patch will not raise a
>
>s/patch/code/ (or some such)
>
>> useless softirq since the IPI is consumed by hardware rather than raise a
>> softirq unconditionally.
>> 
>> Quan doesn't have enough time to upstream this fix patch. He asks me to do
>> this. Merge another his related patch
>> (https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg02885.html).
>
>This doesn't belong in the commit message, and hence should be after
>the first ---.
>

Will take care of this later.

>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1842,13 +1842,59 @@ static void __vmx_deliver_posted_interrupt(struct 
>> vcpu *v)
>>      bool_t running = v->is_running;
>>  
>>      vcpu_unblock(v);
>> +    /*
>> +     * The underlying 'IF' excludes two cases which we don't need further
>
>Who or what is 'IF'?
>

I meaned the 'if sentence'.

>> +     * operation to make sure the target vCPU (@v) will sync PIR to vIRR 
>> ASAP.
>> +     * Specifically, the two cases are:
>> +     * 1. The target vCPU is not running, meaning it is blocked or runnable.
>> +     * Since we have unblocked the target vCPU above, the target vCPU will
>> +     * sync PIR to vIRR when it is chosen to run.
>> +     * 2. The target vCPU is the current vCPU and in_irq() is false. It 
>> means
>> +     * the function is called in noninterrupt context.
>
>     * 2. The target vCPU is the current vCPU and we're in
>     * non-interrupt context.
>
>> Since we don't call
>> +     * the function in noninterrupt context after the last time a vCPU syncs
>> +     * PIR to vIRR, excluding this case is also safe.
>
>It is not really clear to me what "the function" here refers to. Surely
>the function the comment is in won't call itself, no matter what
>context.

Yes, it is not clear. How about:
The target vCPU is the current vCPU and we're in non-interrupt context.
we can't be in the window between the call to vmx_intr_assist()
and interrupts getting disabled since no existed code reachs here in
non-interrupt context. Considering that, it is safe to just leave without
further operation. 

Do you think this is correct? I have an assumption here to explain why
(in_irq() || (v != current)) is reasonable. It is totally my guess.

>
>> +     */
>>      if ( running && (in_irq() || (v != current)) )
>>      {
>> +        /*
>> +         * Note: Only two cases will reach here:
>> +         * 1. The target vCPU is running on other pCPU.
>> +         * 2. The target vCPU is running on the same pCPU with the current
>> +         * vCPU
>
>This is an impossible thing: There can't be two vCPU-s running on
>the same pCPU-s at the same time. Hence ...
>
>> and the current vCPU is in interrupt context. That's to say,
>> +         * the target vCPU is the current vCPU.
>
>... just this last statement is sufficient here.
>
>> +         * Note2: Don't worry the v->processor may change since at least 
>> when
>> +         * the target vCPU is chosen to run or be blocked, there is a chance
>> +         * to sync PIR to vIRR.
>> +         */
>
>"There is a chance" reads as if it may or may not happen. How about
>"The vCPU being moved to another processor is guaranteed to sync
>PIR to vIRR, due to the involved scheduling cycle"? Of course only if
>this matches reality.

I think your description is right.

>
>>          unsigned int cpu = v->processor;
>>  
>> -        if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>> -             && (cpu != smp_processor_id()) )
>> +        /*
>> +         * For case 1, we send a IPI to the pCPU. When the IPI arrives, the
>
>... an IPI ... (also at least once more below)
>
>> +         * target vCPU maybe is running in non-root mode, running in root
>> +         * mode, runnable or blocked. If the target vCPU is running in
>> +         * non-root mode, the hardware will sync PIR to vIRR for the IPI
>> +         * vector is special to the pCPU. If the target vCPU is running in
>> +         * root-mode, the interrupt handler starts to run. In order to make
>> +         * sure the target vCPU could go back to vmx_intr_assist(), the
>> +         * interrupt handler should raise a softirq if no pending softirq.
>
>I don't understand this part: Calling vmx_intr_assist() is part of any
>VM entry, so if we're in root mode, the function will necessarily be
>called. Are you perhaps worried about the window between the call
>to vmx_intr_assist() and interrupts getting disabled (or any other
>similar one - I didn't make an attempt to collect a complete set)? If
>so, that's what needs to be mentioned here.

Yes. I only recognized this window. Will mention this window in next version.

>
>> +         * If the target vCPU is runnable, it will sync PIR to vIRR next 
>> time
>> +         * it is chose to run. In this case, a IPI and a softirq is sent to
>> +         * a wrong vCPU which we think it is not a big issue. If the target
>
>Maybe "... which will not have any adverse effect"?
>
>> +         * vCPU is blocked, since vcpu_block() checks whether there is an
>> +         * event to be delivered through local_events_need_delivery() just
>> +         * after blocking, the vCPU must have synced PIR to vIRR. Similarly,
>> +         * there is a IPI and a softirq sent to a wrong vCPU.
>> +         */
>> +        if ( cpu != smp_process_id() )
>
>Did you not even build test this patch? I don't think the construct
>above compiles.

Really sorry. I forgot to commit after I fixed this.
Acturally, I found this, fixed it and tested this patch several times( with or
without VT-d PI) to avoid some obvious regression.

>
>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>> +        /*
>> +         * For case 2, raising a softirq can cause vmx_intr_assist() where 
>> PIR
>> +         * has a chance to be synced to vIRR to be called. As an 
>> optimization,
>> +         * We only need to raise a softirq when no pending softirq.
>
>How about "As any softirq will do, as an optimization we only raise
>one if none is pending already"? Again, if this is a valid statement
>only, of course.

Your description is correct imo and better.

Thanks,
Chao

>
>Jan

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