[PATCH] xen: idle_loop: either deal with tasklets or go idle

[PATCH] xen: idle_loop: either deal with tasklets or go idle

[Dario Faggioli]

Hi,

following up on this:

 https://lists.xen.org/archives/html/xen-devel/2017-06/msg01260.html

I did make a patch that moves do_tasklet() up a bit, within idle_loop().

While there, I did a bit more than that... let's see what you guys think. :-D

I've verified that this builds on ARM too, but have not run it (while I did
that, for x86):

 https://travis-ci.org/fdario/xen/builds/242888986

Thanks and Regards,
Dario
---
Dario Faggioli (1):
      xen: idle_loop: either deal with tasklets or go idle

 xen/arch/arm/domain.c     |   21 ++++++++++++++-------
 xen/arch/x86/domain.c     |   12 +++++++++---
 xen/common/tasklet.c      |   10 +---------
 xen/include/xen/tasklet.h |   12 +++++++++++-
 4 files changed, 35 insertions(+), 20 deletions(-)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH] xen: idle_loop: either deal with tasklets or go idle

[Dario Faggioli]

In fact, there exists two kind of tasklets: vCPU and
softirq context tasklets. When we want to do vCPU
context tasklet work, we force the idle vCPU (of a
particular pCPU) into execution, and run it from there.

This means there are two possible reasons for choosing
to run the idle vCPU:
1) we want a pCPU to go idle,
2) wa want to run some vCPU context tasklet work.

If we're in case 2), it does not make sense to try to
see whether we can go idle, and only afterwords (as the
check _will_ fail), go processing tasklets.

This patch rearranges the code of the body of the idle
vCPUs, so that we actually check whether we are in
case 1) or 2), and act accordingly.

As a matter of fact, this also means that we do not
check for any tasklet work to be done, after waking up
from idle. This is not a problem, because:
a) for softirq context tasklets, if any is queued
   "during" wakeup from idle, TASKLET_SOFTIRQ is
   raised, and the call to do_softirq() (which is still
   happening *after* the wakeup) will take care of it;
b) for vCPU context tasklets, if any is queued "during"
   wakeup from idle, SCHEDULE_SOFTIRQ is raised and
   do_softirq() (happening after the wakeup) calls
   the scheduler. The scheduler sees that there is
   tasklet work pending and confirms the idle vCPU
   in execution, which then will get to execute
   do_tasklet().

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/arm/domain.c     |   21 ++++++++++++++-------
 xen/arch/x86/domain.c     |   12 +++++++++---
 xen/common/tasklet.c      |   10 +---------
 xen/include/xen/tasklet.h |   12 +++++++++++-
 4 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 76310ed..0ceeb5b 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -41,20 +41,27 @@ 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 ( unlikely(tasklet_work_to_do(cpu)) )
+            do_tasklet(cpu);
+        else
         {
-            dsb(sy);
-            wfi();
+            local_irq_disable();
+            if ( cpu_is_haltable(cpu) )
+            {
+                dsb(sy);
+                wfi();
+            }
+            local_irq_enable();
         }
-        local_irq_enable();
 
-        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..d06700d 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 running vcpu context tasklets, or for idling? */
+        if ( unlikely(tasklet_work_to_do(cpu)) )
+            do_tasklet(cpu);
+        else
+            (*pm_idle)();
         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..0465751 100644
--- 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();
     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;
-
     spin_lock_irq(&tasklet_lock);
 
     do_tasklet_work(cpu, list);
diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
index 8c3de7e..1a3f861 100644
--- 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);
+}
+
 void tasklet_schedule_on_cpu(struct tasklet *t, unsigned int cpu);
 void tasklet_schedule(struct tasklet *t);
-void do_tasklet(void);
+void do_tasklet(unsigned int cpu);
 void tasklet_kill(struct tasklet *t);
 void tasklet_init(
     struct tasklet *t, void (*func)(unsigned long), unsigned long data);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Jan Beulich]

>>> On 14.06.17 at 18:53, <dario.faggioli@citrix.com> 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.

> +            do_tasklet(cpu);
> +        else
> +            (*pm_idle)();

Please take the opportunity and drop the pointless parentheses
and indirection.

> --- 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. In any event, if you do it this way, we will want an ASSERT()
to replace the initialization above.

>      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.

> --- 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?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 7253 bytes --]

On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote:
> > > > On 14.06.17 at 18:53, <dario.faggioli@citrix.com> 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);
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Jan Beulich]

>>> On 16.06.17 at 12:44, <dario.faggioli@citrix.com> wrote:
> On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote:
>> > > > On 14.06.17 at 18:53, <dario.faggioli@citrix.com> 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.

Okay then.

>> >      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.

How that? TASKLET_enqueued can only be cleared by do_tasklet()
itself, and I don't think nesting invocations of the function can or
should occur. TASKLET_scheduled is only being cleared when
schedule() observes that bit set without TASKLET_enqueued also
set. IOW there may be races in setting of the bits, but since we
expect the caller to have done this check already, I think an
ASSERT() would be quite fine here.

>> > --- 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.

I'm confused: You've added further uses of cpu_is_haltable()
where the cheaper tasklet_work_to_do() would do. That's sort
of the opposite of what I've suggested. Of course that suggestion
of mine was more than 1:1 replacement - the implied question was
whether cpu_is_haltable() simply checking tasklet_work_to_do to
be non-zero isn't too lax (and wouldn't better be
!tasklet_work_to_do()).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 5245 bytes --]

On Fri, 2017-06-16 at 05:47 -0600, Jan Beulich wrote:
> > On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote:
> > > > > > On 14.06.17 at 18:53, <dario.faggioli@citrix.com> wrote:
> > > >      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.
> 
> How that? TASKLET_enqueued can only be cleared by do_tasklet()
> itself, and I don't think nesting invocations of the function can or
> should occur. TASKLET_scheduled is only being cleared when
> schedule() observes that bit set without TASKLET_enqueued also
> set. IOW there may be races in setting of the bits, but since we
> expect the caller to have done this check already, I think an
> ASSERT() would be quite fine here.
> 
Ok, makes sense. I will add the ASSERT() (with something like what you
wrote here as a comment).

> > > 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.
> 
> I'm confused: You've added further uses of cpu_is_haltable()
> where the cheaper tasklet_work_to_do() would do.
>
Indeed. Sorry!

Fact is, I've read your comment backwards, i.e., as you were saying
something like "wouldn't cpu_is_haltable() better be used here, instead
of this new function?"

And it's not that your wording is ambiguous, it's me that, apparently,
can't read English! :-/

I'll rework the patch again...

> Of course that suggestion
> of mine was more than 1:1 replacement - the implied question was
> whether cpu_is_haltable() simply checking tasklet_work_to_do to
> be non-zero isn't too lax (and wouldn't better be
> !tasklet_work_to_do()).
> 
Now, to try to answer the real question...

Let's assume that, on cpu x, we are about to check cpu_is_haltable(),
while, on cpu y, tasklet_schedule_on_cpu(x) is being called, and
manages to set _TASKLET_enqueued in *work_to_do.

I.e., in current code:

 CPU x                           CPU y
 |                               |
 cpu_is_haltable(x)              tasklet_schedule_on_cpu(x)
 |!softirq_pending(x) == true    tasklet_enqueue()
 |cpu_online(x) == true          test_and_set(TASKLET_enqueued,
 |                                            work_to_do)
 |!work_to_do == FALSE

So we don't go to sleep, and we stay in the idle loop for the next
iteration. At which point, CPU y will have raised SCHEDULE_SOFTIRQ on
x, schedule (still on x) will set TASKLET_scheduled, and we'll call
do_tasklet().

Basically, right now, we risk spinning for the time that passes between
TASKLET_enqueued being set and SCHEDULE_SOFTIRQ being raised and
reaching cpu x. This should be a very short window, and, considering
how the TASKLET_* flags are handled, this looks the correct behavior to
me.

If we use !tasklet_work_to_do() in cpu_is_haltable():

 CPU x                               CPU y
 |                                   |
 cpu_is_haltable(x)                  tasklet_schedule_on_cpu(x)
 |!softirq_pending(x) == true        tasklet_enqueue()
 |cpu_online(x) == true              test_and_set(TASKLET_enqueued,
 |                                              work_to_do)
 |!(work_to_do == TASKLET_enqueued+
                  TASKLET_scheduled) == TRUE

Which means we'd go to sleep... just for (most likely) be woken up very
very soon by SCHEDULE_SOFTIRQ being thrown at us (cpu x) by cpu y.

Am I overlooking anything? And is this (this time) what you were
asking?

Assuming answers are 'no' and 'yes', I think I'd leave
cpu_is_haltable() as it is (perhaps adding a brief comment on why it's
ok/better to check work_to_do directly, instead than calling
tasklet_work_to_do()).

Sorry again for the misunderstanding,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Jan Beulich]

>>> On 16.06.17 at 15:34, <dario.faggioli@citrix.com> wrote:
> Assuming answers are 'no' and 'yes', I think I'd leave
> cpu_is_haltable() as it is

Agreed, you analysis was quite helpful.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5882 bytes --]

On Fri, 16 Jun 2017, Dario Faggioli wrote:
> On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote:
> > > > > On 14.06.17 at 18:53, <dario.faggioli@citrix.com> 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();

Are you sure you want to check that cpu_is_haltable twice? It doesn't
make sense to me.

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2180 bytes --]

On Fri, 2017-06-16 at 10:41 -0700, Stefano Stabellini wrote:
> On Fri, 16 Jun 2017, Dario Faggioli wrote:
> > 
> > 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();
> 
> Are you sure you want to check that cpu_is_haltable twice? It doesn't
> make sense to me.
>
It's because of IRQ being disabled the first time.

But anyway, discard this patch. I'll go back to (a slightly modified
version of) the first one I sent, which defines a tasklet specific
helper function.

I'll send it on Monday.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel