Re: [v4 16/17] vmx: Add some scheduler hooks for VT-d posted interrupts

From: "Jan Beulich" <JBeulich@suse.com>
To: Dario Faggioli <dario.faggioli@citrix.com>, Feng Wu <feng.wu@intel.com>
Cc: GeorgeDunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [v4 16/17] vmx: Add some scheduler hooks for VT-d posted interrupts
Date: Tue, 11 Aug 2015 04:23:03 -0600	[thread overview]
Message-ID: <55C9E9270200007800099787@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1438280797.16912.86.camel@citrix.com>

>>> On 30.07.15 at 20:26, <dario.faggioli@citrix.com> wrote:
> On Thu, 2015-07-30 at 02:04 +0000, Wu, Feng wrote:
>> > -----Original Message-----
>> > From: Dario Faggioli [mailto:dario.faggioli@citrix.com]
>> > Since this is one of the differences
>> > between the two, was it the cause of the issues you were seeing? If yes,
>> > can you elaborate on how and why?
>> > 
>> > In the end, I'm not too opposed to the hook being at the beginning
>> > rather than at the end, but there has to be a reason, which may well end
>> > up better be stated in a comment...
>> 
>> Here is the reason I put arch_vcpu_wake() ahead of vcpu_wake():
>> arch_vcpu_wake() does some prerequisites for a vCPU which is about
>> to run, such as, setting SN again, changing NV filed back to
>> ' posted_intr_vector ', which should be finished before the vCPU is
>> actually scheduled to run. However, if we put arch_vcpu_wake() later
>> in vcpu_wake() right before ' vcpu_schedule_unlock_irqrestore', after
>> the 'wake' hook get finished, the vcpu can run at any time (maybe in
>> another pCPU since the current pCPU is protected by the lock), if
>> this can happen, it is incorrect. Does my understanding make sense?
>> 
> It's safe in any case. In fact, the spinlock will  prevent both the
> vcpu's processor to schedule, as well as any other processors to steal
> the waking vcpu from the runqueue to run it.
> 
> That's actually why I wanted to double check you changing the position
> of the hook (wrt the draft), as it felt weird that the issue were in
> there. :-)
> 
> So, now that we know that safety is not an issue, where should we put
> the hook?
> 
> Having it before SCHED_OP(wake) may make people think that arch specific
> code is (or can, at some point) somehow influencing the scheduler
> specific wakeup code, which is not (and should not become, if possible)
> the case.
> 
> However, I kind of like the fact that the spinlock is released as soon
> as possible, after the call to SCHED_OP(wake). That will make it more
> likely, for the processors we may have sent IPIs to, during the
> scheduler specific wakeup code, to find the spinlock free. So, looking
> at things from this angle, it would be better to avoid putting stuff in
> between SCHED_OP(wake) and vcpu_schedule_unlock().
> 
> So, all in all, I'd say leave it on top, where it is in this patch. Of
> course, if others have opinions, I'm all ears. :-)

If it is kept at the beginning, the hook should be renamed to
something like arch_vcpu_wake_prepare().

Jan