From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 3/9] xen: sched: make locking for {insert, remove}_vcpu consistent Date: Thu, 8 Oct 2015 18:23:46 +0100 Message-ID: <5616A6A2.2090102@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> <56169E00.9060902@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZkEv2-00058h-DB for xen-devel@lists.xenproject.org; Thu, 08 Oct 2015 17:23:52 +0000 In-Reply-To: <56169E00.9060902@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: George Dunlap , Dario Faggioli , xen-devel@lists.xenproject.org Cc: George Dunlap , Meng Xu List-Id: xen-devel@lists.xenproject.org On 08/10/15 17:46, George Dunlap wrote: > 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. My original statement came from the observation that schedule() runs with interrupts disabled, and takes between 2.2 and 4 microseconds to run (as measured during the ticketlock performance analysis). It is the biggest consumer of time with interrupts disabled, next being the time calibration rendezvous. I am going to go out on a limb and say that the majority of that time does not need to be spent with interrupts disabled. I might easily be wrong, but I suspect I am not. ~Andrew