From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 3/9] xen: sched: make locking for {insert, remove}_vcpu consistent Date: Thu, 8 Oct 2015 17:46:56 +0100 Message-ID: <56169E00.9060902@citrix.com> References: <20150929164726.17589.96920.stgit@Solace.station> <20150929165549.17589.76223.stgit@Solace.station> <560ACAD5.8040405@citrix.com> <56168479.60703@citrix.com> <561689B6.4020306@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZkELN-0001hC-Au for xen-devel@lists.xenproject.org; Thu, 08 Oct 2015 16:47:01 +0000 In-Reply-To: <561689B6.4020306@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: Andrew Cooper , Dario Faggioli , xen-devel@lists.xenproject.org Cc: George Dunlap , Meng Xu List-Id: xen-devel@lists.xenproject.org On 08/10/15 16:20, Andrew Cooper wrote: > On 08/10/15 15:58, George Dunlap wrote: >> On 29/09/15 18:31, Andrew Cooper wrote: >>> On 29/09/15 17:55, Dario Faggioli wrote: >>>> The insert_vcpu() scheduler hook is called with an >>>> inconsistent locking strategy. In fact, it is sometimes >>>> invoked while holding the runqueue lock and sometimes >>>> when that is not the case. >>>> >>>> In other words, some call sites seems to imply that >>>> locking should be handled in the callers, in schedule.c >>>> --e.g., in schedule_cpu_switch(), which acquires the >>>> runqueue lock before calling the hook; others that >>>> specific schedulers should be responsible for locking >>>> themselves --e.g., in sched_move_domain(), which does >>>> not acquire any lock for calling the hook. >>>> >>>> The right thing to do seems to always defer locking to >>>> the specific schedulers, as it's them that know what, how >>>> and when it is best to lock (as in: runqueue locks, vs. >>>> private scheduler locks, vs. both, etc.) >>>> >>>> This patch, therefore: >>>> - removes any locking around insert_vcpu() from >>>> generic code (schedule.c); >>>> - add the _proper_ locking in the hook implementations, >>>> depending on the scheduler (for instance, credit2 >>>> does that already, credit1 and RTDS need to grab >>>> the runqueue lock while manipulating runqueues). >>>> >>>> In case of credit1, remove_vcpu() handling needs some >>>> fixing remove_vcpu() too, i.e.: >>>> - it manipulates runqueues, so the runqueue lock must >>>> be acquired; >>>> - *_lock_irq() is enough, there is no need to do >>>> _irqsave() >>> Nothing in any of generic scheduling code should need interrupts >>> disabled at all. >>> >>> One of the problem-areas identified by Jenny during the ticketlock >>> performance work was that the SCHEDULE_SOFTIRQ was a large consumer of >>> time with interrupts disabled. (The other large one being the time >>> calibration rendezvous, but that is a wildly different can of worms to fix.) >> Generic scheduling code is called from interrupt contexts -- namely, >> vcpu_wake() > > There are a lot of codepaths, but I cant see one which is definitely > called with interrupts disables. (OTOH, I can see several where > interrupts are definitely enabled). Oh, I think I misunderstood you. You meant, "No codepaths *calling into* generic scheduling code should need interrupts disabled at all". I can certainly believe that to be true in most cases; there's no sense in saving the flags if we don't need to. -George