[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2] xen: idle_loop: either deal with tasklets or go idle



On Tue, 20 Jun 2017, Dario Faggioli wrote:
> In fact, there are two kinds of tasklets: vCPU and
> softirq context. 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) we want to run some vCPU context tasklet work.
> 
> If we're in case 2), it does not make sense to even
> try to go idle (as the check will _always_ fail).
> 
> This patch rearranges the code of the body of 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 if there's any tasklet work to do 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@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Changes from v1:
> * drop the pointless parentheses and indirection of pm_idle (in x86's idle
>   loop);
> * remove the 'cpu' input parameter to do_tasklet(), as suggested during 
> review;
> * in do_tasklet(), convert the check that there is tasklet work to do into an
>   ASSERT() to the same effect, as suggested during review;
> * add a comment in cpu_is_haltable() on why we check the per-cpu
>   tasklet_work_to_do variable directly, rather than calling the new
>   tasklet_work_to_do() helper.
> ---
>  xen/arch/arm/domain.c     |   21 ++++++++++++++-------
>  xen/arch/x86/domain.c     |   12 +++++++++---
>  xen/common/tasklet.c      |   12 ++++++++----
>  xen/include/xen/sched.h   |    5 +++++
>  xen/include/xen/tasklet.h |   10 ++++++++++
>  5 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 76310ed..2dc8b0a 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();
> +        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..3a061a9 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();
> +        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..0f0a6f8 100644
> --- a/xen/common/tasklet.c
> +++ b/xen/common/tasklet.c
> @@ -111,11 +111,15 @@ void do_tasklet(void)
>      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.
> +     * We want to be sure any caller has checked that a tasklet is both
> +     * enqueued and scheduled, before calling this. And, if the caller has
> +     * actually checked, it's not an issue that we are outside of the
> +     * critical region, in fact:
> +     * - TASKLET_enqueued is cleared only here,
> +     * - TASKLET_scheduled is only cleared when schedule() find it set,
> +     *   without TASKLET_enqueued being set as well.
>       */
> -    if ( likely(*work_to_do != (TASKLET_enqueued|TASKLET_scheduled)) )
> -        return;
> +    ASSERT(tasklet_work_to_do(cpu));
>  
>      spin_lock_irq(&tasklet_lock);
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 1127ca9..6673b27 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -843,6 +843,11 @@ uint64_t get_cpu_idle_time(unsigned int cpu);
>  /*
>   * Used by idle loop to decide whether there is work to do:
>   *  (1) Run softirqs; or (2) Play dead; or (3) Run tasklets.
> + *
> + * About (3), if a tasklet is enqueued, it will be scheduled
> + * really really soon, and hence it's pointless to try to
> + * sleep between these two events (that's why we don't call
> + * the tasklet_work_to_do() helper).
>   */
>  #define cpu_is_haltable(cpu)                    \
>      (!softirq_pending(cpu) &&                   \
> diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
> index 8c3de7e..23d69c7 100644
> --- a/xen/include/xen/tasklet.h
> +++ b/xen/include/xen/tasklet.h
> @@ -40,6 +40,16 @@ 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);
> 

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.