From patchwork Fri Jun 16 10:44:01 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Dario Faggioli X-Patchwork-Id: 9791083 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4E74960325 for ; Fri, 16 Jun 2017 10:46:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 382CD285C7 for ; Fri, 16 Jun 2017 10:46:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2C347285D1; Fri, 16 Jun 2017 10:46:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 426E3285C7 for ; Fri, 16 Jun 2017 10:46:50 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dLojk-0003Nd-7g; Fri, 16 Jun 2017 10:44:20 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dLojj-0003NX-31 for xen-devel@lists.xenproject.org; Fri, 16 Jun 2017 10:44:19 +0000 Received: from [193.109.254.147] by server-11.bemta-6.messagelabs.com id 0D/E6-03587-286B3495; Fri, 16 Jun 2017 10:44:18 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrBIsWRWlGSWpSXmKPExsXitHSDvW7jNud Ig6+7jCy+b5nM5MDocfjDFZYAxijWzLyk/IoE1owpH+UKznhU/Hv3m7GB8Y1NFyMnh4RAiMSv Gx/Zuxg5OHgFDCW2vDcBMYUFwiR+rhMEqWATMJB4s2MvK4gtIqAs0fvrN0sXIxcHs8BlRokPs 6cxgSRYBFQlZv56ygZicwrYS/y/NIUJpEhI4DCjxMHr88C6+QUkJW59+cgMYjMLVEvsn7+BHe IGbYkj5/rBbF4BQYmTM5+wgNhCAmoSM+ZeZp3AyDcLScssJGUQcU2J1u2/2SFsbYllC18zQ9i 2EuvWvYeqsZHYdHUBI4QtL7H97RzmBYzsqxg1ilOLylKLdI0s9JKKMtMzSnITM3N0DQ3M9HJT i4sT01NzEpOK9ZLzczcxAkOZAQh2MJ5fG3iIUZKDSUmUl1/OKVKILyk/pTIjsTgjvqg0J7X4E KMMB4eSBO+irc6RQoJFqempFWmZOcCogklLcPAoifAybQJK8xYXJOYWZ6ZDpE4x6nK03d/yhU mIJS8/L1VKnFcTZIYASFFGaR7cCFiEX2KUlRLmZQQ6SoinILUoN7MEVf4VozgHo5IwxBSezLw SuE2vgI5gAjoi6IIDyBEliQgpqQZGvu9n5hncX7DnwP77WUc3f+Az/OXpu2PHiT0rTsvPtXvd Yu0/l+nYnipeKz6O+vuqS7Y4OP+/Zb3B7v7V5knPuNe3rff/vspznk7BP3+tAw8uufK/4/juM IHvUbRX+eOiBPEJdWKzJqk5T2qccejOz5fT5zxevT/v0f9blq0OMbvyxeMmnjmt9EOJpTgj0V CLuag4EQAk2DS26wIAAA== X-Env-Sender: prvs=33382dc55=dario.faggioli@citrix.com X-Msg-Ref: server-10.tower-27.messagelabs.com!1497609856!84789190!1 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 9.4.19; banners=-,-,- X-VirusChecked: Checked Received: (qmail 8801 invoked from network); 16 Jun 2017 10:44:17 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-10.tower-27.messagelabs.com with RC4-SHA encrypted SMTP; 16 Jun 2017 10:44:17 -0000 X-IronPort-AV: E=Sophos;i="5.39,346,1493683200"; d="asc'?scan'208";a="436679723" Message-ID: <1497609841.30417.5.camel@citrix.com> From: Dario Faggioli To: Jan Beulich Date: Fri, 16 Jun 2017 12:44:01 +0200 In-Reply-To: <5943B8F202000078001635CC@prv-mh.provo.novell.com> References: <149745892779.20244.4770433880444010417.stgit@Solace.fritz.box> <149745919711.20244.17843343131079129783.stgit@Solace.fritz.box> <5943B8F202000078001635CC@prv-mh.provo.novell.com> Organization: Citrix Inc. X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) MIME-Version: 1.0 Cc: Andrew Cooper , Julien Grall , Stefano Stabellini , Boris Ostrovsky , xen-devel@lists.xenproject.org Subject: Re: [Xen-devel] [PATCH] xen: idle_loop: either deal with tasklets or go idle X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote: > > > > On 14.06.17 at 18:53, wrote: > > > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -112,12 +112,18 @@ static void play_dead(void) > >   > >  static void idle_loop(void) > >  { > > +    unsigned int cpu = smp_processor_id(); > > + > >      for ( ; ; ) > >      { > > -        if ( cpu_is_offline(smp_processor_id()) ) > > +        if ( cpu_is_offline(cpu) ) > >              play_dead(); > > -        (*pm_idle)(); > > -        do_tasklet(); > > + > > +        /* Are we here for running vcpu context tasklets, or for > > idling? */ > > +        if ( unlikely(tasklet_work_to_do(cpu)) ) > > I'm not really sure about the "unlikely()" here. > It's basically already there, without this patch, at the very beginning of do_tasklet():     if ( likely(*work_to_do != (TASKLET_enqueued|TASKLET_scheduled)) )         return; Which is right the check that I moved in tasklet_work_to_do(), and as you can see, it has the likely. So, I fundamentally kept it for consistency with old code. I actually think it does make sense, but I don't have a too strong opinion about this. > > +            do_tasklet(cpu); > > +        else > > +            (*pm_idle)(); > > Please take the opportunity and drop the pointless parentheses > and indirection. > Ok. > > --- a/xen/common/tasklet.c > > +++ b/xen/common/tasklet.c > > @@ -104,19 +104,11 @@ static void do_tasklet_work(unsigned int cpu, > > struct list_head *list) > >  } > >   > >  /* VCPU context work */ > > -void do_tasklet(void) > > +void do_tasklet(unsigned int cpu) > >  { > > -    unsigned int cpu = smp_processor_id(); > > I'm not convinced it is a good idea to have the caller pass in the > CPU > number. > Yes, I know. I couldn't make up my mind about it either. I guess I get get rid of this aspect of the patch. > >      unsigned long *work_to_do = &per_cpu(tasklet_work_to_do, cpu); > >      struct list_head *list = &per_cpu(tasklet_list, cpu); > >   > > -    /* > > -     * Work must be enqueued *and* scheduled. Otherwise there is > > no work to > > -     * do, and/or scheduler needs to run to update idle vcpu > > priority. > > -     */ > > -    if ( likely(*work_to_do != > > (TASKLET_enqueued|TASKLET_scheduled)) ) > > -        return; > > Perhaps it also wouldn't hurt to convert this to an ASSERT() too. > Nope, I can't. It's a best effort check, and *work_to_do (which is per_cpu(tasklet_work_to_do,cpu)) can change, and the assert may fail. The code is, of course, safe, because, if we think that there's work but there's not, the list of pending tasklets will be empty (and we check that after taking the tasklet lock). > > --- a/xen/include/xen/tasklet.h > > +++ b/xen/include/xen/tasklet.h > > @@ -40,9 +40,19 @@ DECLARE_PER_CPU(unsigned long, > > tasklet_work_to_do); > >  #define TASKLET_enqueued   (1ul << _TASKLET_enqueued) > >  #define TASKLET_scheduled  (1ul << _TASKLET_scheduled) > >   > > +static inline bool tasklet_work_to_do(unsigned int cpu) > > +{ > > +    /* > > +     * Work must be enqueued *and* scheduled. Otherwise there is > > no work to > > +     * do, and/or scheduler needs to run to update idle vcpu > > priority. > > +     */ > > +    return per_cpu(tasklet_work_to_do, cpu) == (TASKLET_enqueued| > > +                                                TASKLET_scheduled) > > ; > > +} > > Wouldn't cpu_is_haltable() then also better use this new function? > Mmm... Perhaps. It's certainly less code chrun. ARM code would then contain two invocations of cpu_is_haltable() (the first happens with IRQ enabled, so a second one with IRQ disabled is necessary). But that is *exactly* the same thing we do on x86 (they're just in different functions in that case). So, I reworked the patch according to these suggestions, and you can look at it below. If you like it better, I'm ok re-submitting it properly in this shape. Other thoughts anyone else? Thanks and Regards, Dario --- NOTE that, since we call do_tasklet() after having checked cpu_is_haltable(), the if in there is not likely any longer. --- diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 76310ed..86cd612 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -41,20 +41,28 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu); void idle_loop(void) { + unsigned int cpu = smp_processor_id(); + for ( ; ; ) { - if ( cpu_is_offline(smp_processor_id()) ) + if ( cpu_is_offline(cpu) ) stop_cpu(); - local_irq_disable(); - if ( cpu_is_haltable(smp_processor_id()) ) + /* Are we here for running vcpu context tasklets, or for idling? */ + if ( cpu_is_haltable(cpu) ) { - dsb(sy); - wfi(); + local_irq_disable(); + /* We need to check again, with IRQ disabled */ + if ( cpu_is_haltable(cpu) ) + { + dsb(sy); + wfi(); + } + local_irq_enable(); } - local_irq_enable(); + else + do_tasklet(); - do_tasklet(); do_softirq(); /* * We MUST be last (or before dsb, wfi). Otherwise after we get the diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 49388f4..c520fdd 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -112,12 +112,18 @@ static void play_dead(void) static void idle_loop(void) { + unsigned int cpu = smp_processor_id(); + for ( ; ; ) { - if ( cpu_is_offline(smp_processor_id()) ) + if ( cpu_is_offline(cpu) ) play_dead(); - (*pm_idle)(); - do_tasklet(); + + /* Are we here for idling, or for running vcpu context tasklets? */ + if ( cpu_is_haltable(cpu) ) + pm_idle(); + else + do_tasklet(); do_softirq(); /* * We MUST be last (or before pm_idle). Otherwise after we get the diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c index 365a777..ebdce12 100644 --- a/xen/common/tasklet.c +++ b/xen/common/tasklet.c @@ -114,7 +114,7 @@ void do_tasklet(void) * Work must be enqueued *and* scheduled. Otherwise there is no work to * do, and/or scheduler needs to run to update idle vcpu priority. */ - if ( likely(*work_to_do != (TASKLET_enqueued|TASKLET_scheduled)) ) + if ( *work_to_do != (TASKLET_enqueued|TASKLET_scheduled) ) return; spin_lock_irq(&tasklet_lock);