From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v7 15/17] vmx: VT-d posted-interrupt core logic handling Date: Thu, 17 Sep 2015 10:39:49 +0100 Message-ID: <55FA8A65.4060309@citrix.com> References: <1441960146-10569-1-git-send-email-feng.wu@intel.com> <1441960146-10569-16-git-send-email-feng.wu@intel.com> <1442423887.15327.29.camel@citrix.com> <1442479704.15327.65.camel@citrix.com> <55FA8A02.30705@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55FA8A02.30705@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli , "Wu, Feng" Cc: "Tian, Kevin" , Keir Fraser , George Dunlap , Andrew Cooper , "xen-devel@lists.xen.org" , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 09/17/2015 10:38 AM, George Dunlap wrote: > On 09/17/2015 09:48 AM, Dario Faggioli wrote: >> On Thu, 2015-09-17 at 08:00 +0000, Wu, Feng wrote: >> >>>> -----Original Message----- >>>> From: Dario Faggioli [mailto:dario.faggioli@citrix.com] >> >>>> 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). >> >>>> - 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 . :-) > > So just to clarify the situation... Er, and to clarify something else -- Technically I'm responding to Dario here, but my mail is actually addressed to Wu Feng. This was just a good point to "put my oar in" to the conversation. :-) -George