Re: [Xen-devel] [PATCH v7 15/17] vmx: VT-d posted-interrupt core logic handling

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> Sent: Thursday, September 17, 2015 1:18 AM
> To: Wu, Feng
> Cc: xen-devel@xxxxxxxxxxxxx; Tian, Kevin; Keir Fraser; George Dunlap; Andrew
> Cooper; Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v7 15/17] vmx: VT-d posted-interrupt core 
> logic
> handling
> 
> On Fri, 2015-09-11 at 16:29 +0800, Feng Wu wrote:
> > This patch includes the following aspects:
> > - Handling logic when vCPU is blocked:
> >     * Add a global vector to wake up the blocked vCPU
> >       when an interrupt is being posted to it (This part
> >       was sugguested by Yang Zhang <yang.z.zhang@xxxxxxxxx>).
> >     * Define two per-cpu variables:
> >           1. pi_blocked_vcpu:
> >             A list storing the vCPUs which were blocked
> >             on this pCPU.
> >
> >           2. pi_blocked_vcpu_lock:
> >             The spinlock to protect pi_blocked_vcpu.
> >
> > - Add some scheduler hooks, this part was suggested
> >   by Dario Faggioli <dario.faggioli@xxxxxxxxxx>.
> >     * vmx_pre_ctx_switch_pi()
> >       It is called before context switch, we update the
> >       posted interrupt descriptor when the vCPU is preempted,
> >       go to sleep, or is blocked.
> >
> >     * vmx_post_ctx_switch_pi()
> >       It is called after context switch, we update the posted
> >       interrupt descriptor when the vCPU is going to run.
> >
> >     * arch_vcpu_wake_prepare()
> >       It will be called when waking up the vCPU, we update
> >       the posted interrupt descriptor when the vCPU is
> >       unblocked.
> >
> > CC: Keir Fraser <keir@xxxxxxx>
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> > CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > CC: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> > Sugguested-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> > ---
> > v7:
> > - Merge [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> >   and "[PATCH v6 14/18] vmx: posted-interrupt handling when vCPU is
> blocked"
> >   into this patch, so it is self-contained and more convenient
> >   for code review.
> > - Make 'pi_blocked_vcpu' and 'pi_blocked_vcpu_lock' static
> > - Coding style
> > - Use per_cpu() instead of this_cpu() in pi_wakeup_interrupt()
> > - Move ack_APIC_irq() to the beginning of pi_wakeup_interrupt()
> > - Rename 'pi_ctxt_switch_from' to 'ctxt_switch_prepare'
> > - Rename 'pi_ctxt_switch_to' to 'ctxt_switch_cancel'
> > - Use 'has_hvm_container_vcpu' instead of 'is_hvm_vcpu'
> > - Use 'spin_lock' and 'spin_unlock' when the interrupt has been
> >   already disabled.
> > - Rename arch_vcpu_wake_prepare to vmx_vcpu_wake_prepare
> > - Define vmx_vcpu_wake_prepare in xen/arch/x86/hvm/hvm.c
> > - Call .pi_ctxt_switch_to() __context_switch() instead of directly
> >   calling vmx_post_ctx_switch_pi() in vmx_ctxt_switch_to()
> > - Make .pi_block_cpu unsigned int
> > - Use list_del() instead of list_del_init()
> > - Coding style
> >
> > One remaining item:
> > Jan has concern about calling vcpu_unblock() in vmx_pre_ctx_switch_pi(),
> > need Dario or George's input about this.
> >
> Hi,
> 
> Sorry for the delay in replying, I was on PTO for a few time.

That's fine. Thanks for your reply!

> 
> Coming to the issue, well, it's a though call.
> 
> First of all, Feng, have you tested this with a debug build of Xen? I'm
> asking because it looks to me that you're ending up calling vcpu_wake()
> with IRQ disabled which, if my brain is not too rusty after a few weeks
> of vacation, should result in check_lock() (in xen/common/spinlock.c)
> complaining, doesn't it?
> 
> In fact, in principle this is not too much different from what happens
> in other places. More specifically, what we have is a vcpu being
> re-inserted in  a runqueue, and the need for re-running the scheduler on
> a(some) PCPU(s) is evaluated. That is similar to what happens in Credit2
> (and in RTDS) in csched2_context_saved(), which is called from within
> context_saved(), still from the context switch code (if
> __CSFLAG_delayed_runq_add is true).
> 
> So it's not the thing per se that is that terrible, IMO. The differences
> between that and your case are:
>  - in the Credit2 case, it happens later down in the context switch
>    path (which would look already better to me) and, more important,
>    with IRQs already re-enabled;
>  - in the Credit2 case, the effect that something like that can have on
>    the scheduler is much more evident, as it happens inside a scheduler
>    hook, rather than buried down in arch specific code, which makes me a
>    lot less concerned about the possibility of latent issues Jan was
>    hinting at, with which I concur.
> 
> So, I guess, first of all, can you confirm whether or not it's exploding
> in debug builds?

Does the following information in Config.mk mean it is a debug build?

# A debug build of Xen and tools?
debug ?= y
debug_symbols ?= $(debug)

> And in either case (just tossing out ideas) would it be
> possible to deal with the "interrupt already raised when blocking" case:

Thanks for the suggestions below!

>  - later in the context switching function ?
In this case, we might need to set a flag in vmx_pre_ctx_switch_pi() instead
of calling vcpu_unblock() directly, then when it returns to context_switch(),
we can check the flag and don't really block the vCPU. But I don't have a clear
picture about how to archive this, here are some questions from me:
- When we are in context_switch(), we have done the following changes to
vcpu's state:
        * sd->curr is set to next
        * vCPU's running state (both prev and next ) is changed by
          vcpu_runstate_change()
        * next->is_running is set to 1
        * periodic timer for prev is stopped
        * periodic timer for next is setup
        ......

So what point should we perform the action to _unblock_ the vCPU? We
Need to roll back the formal changes to the vCPU's state, right?

>  - with another hook, perhaps in vcpu_block() ?

We could check this in vcpu_block(), however, the logic here is that before
vCPU is blocked, we need to change the posted-interrupt descriptor,
and during changing it, if 'ON' bit is set, which means VT-d hardware
issues a notification event because interrupts from the assigned devices
is coming, we don't need to block the vCPU and hence no need to update
the PI descriptor in this case. Checking in vcpu_block() might not cover this
logic.

Thanks,
Feng

> 
> Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel