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 4:48 PM
> 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 Thu, 2015-09-17 at 08:00 +0000, Wu, Feng wrote:
> 
> > > -----Original Message-----
> > > From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> 
> > > 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)
> >
> I think so. But as I said in my other email, I was wrong, and this is
> probably not an issue.
> 
> > > 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.
> >
> Yeah, and that would still be rather hard to understand and maintain,
> IMO.
> 
> > 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?
> >
> Mmm... not really. Not blocking prev does not mean that prev would be
> kept running on the pCPU, and that's true for your current solution as
> well! As you say yourself, you're already in the process of switching
> between prev and next, at a point where it's already a thing that next
> will be the vCPU that will run. Not blocking means that prev is
> reinserted to the runqueue, and a new invocation to the scheduler is
> (potentially) queued as well (via raising SCHEDULE_SOFTIRQ, in
> __runq_tickle()), but it's only when such new scheduling happens that
> prev will (potentially) be selected to run again.
> 
> So, no, unless I'm fully missing your point, there wouldn't be no
> rollback required. However, I still would like the other solution (doing
> stuff in vcpu_block()) better (see below).

Thanks for the detailed clarification. Yes, maybe my description above
is a little vague. Yes, the non-blocking vCPU should be put into the
runqueue. I shouldn't use the term "roll back". :)

> 
> > >  - 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.
> >
> Yep, I saw that. But could it be possible to do *everything* related to
> blocking, including the update of the descriptor, in vcpu_block(), if no
> interrupt have been raised yet at that time? I mean, would you, if
> updating the descriptor in there, still get the event that allows you to
> call vcpu_wake(), and hence vmx_vcpu_wake_prepare(), which would undo
> the blocking, no matter whether that resulted in an actual context
> switch already or not?
> 
> I appreciate that this narrows the window for such an event to happen by
> quite a bit, making the logic itself a little less useful (it makes
> things more similar to "regular" blocking vs. event delivery, though,
> AFAICT), but if it's correct, ad if it allows us to save the ugly
> invocation of vcpu_unblock from context switch context, I'd give it a
> try.
> 
> After all, this PI thing requires actions to be taken when a vCPU is
> scheduled or descheduled because of blocking, unblocking and
> preemptions, and it would seem natural to me to:
>  - deal with blockings in vcpu_block()
>  - deal with unblockings in vcpu_wake()
>  - deal with preemptions in context_switch()
> 
> This does not mean being able to consolidate some of the cases (like
> blockings and preemptions, in the current version of the code) were not
> a nice thing... But we don't want it at all costs . :-)

Yes, doing this in vcpu_block() is indeed an alternative, In fact, I also 
thought
about it before. I need to think it more to see whether it meets all the
requirements. I will get back once I have a clear picture. Thank you!

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