Re: [Xen-devel] [PATCH 5/5] xen: RCU: avoid busy waiting until the end of grace period.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 27 Jul 2017, Dario Faggioli wrote:
> Instead of having the CPU where a callback is queued, busy
> looping on rcu_pending(), use a timer.
> 
> In fact, we let the CPU go idla,e but we program a timer
                              ^ idle,


> that will periodically wake it up, for checking whether the
> grace period has actually ended.
> 
> It is kind of similar to introducing a periodic tick, but
> with a much more limited scope, and a lot less overhead. In
> fact, this timer is:
> - only active for the CPU(s) that have callbacks queued,
>   waiting for the end of a grace period;
> - only active when those CPU(s) are idle (and stopped as
>   soon as they resume execution).
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain.c         |    4 ++-
>  xen/arch/x86/acpi/cpu_idle.c  |    6 +++--
>  xen/arch/x86/cpu/mwait-idle.c |    6 +++--
>  xen/common/rcupdate.c         |   52 
> ++++++++++++++++++++++++++++++++++++++++-
>  xen/include/xen/rcupdate.h    |    3 ++
>  5 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 666b7ef..01da96e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -43,8 +43,9 @@ static void do_idle(void)
>  {
>      unsigned int cpu = smp_processor_id();
>  
> +    rcu_idle_timer_start();
>      sched_tick_suspend();
> -    /* sched_tick_suspend() can raise TIMER_SOFTIRQ. Process it now. */
> +    /* Timer related operations can raise TIMER_SOFTIRQ. Process it now. */
>      process_pending_softirqs();
>  
>      local_irq_disable();
> @@ -58,6 +59,7 @@ static void do_idle(void)
>      local_irq_enable();
>  
>      sched_tick_resume();
> +    rcu_idle_timer_stop();
>  }
>  
>  void idle_loop(void)
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index 04c52e8..b97986f 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -576,10 +576,10 @@ static void acpi_processor_idle(void)
>          return;
>      }
>  
> +    rcu_idle_timer_start();
>      cpufreq_dbs_timer_suspend();
> -
>      sched_tick_suspend();
> -    /* sched_tick_suspend() can raise TIMER_SOFTIRQ. Process it now. */
> +    /* Timer related operations can raise TIMER_SOFTIRQ. Process it now. */
>      process_pending_softirqs();
>  
>      /*
> @@ -593,6 +593,7 @@ static void acpi_processor_idle(void)
>          local_irq_enable();
>          sched_tick_resume();
>          cpufreq_dbs_timer_resume();
> +        rcu_idle_timer_stop();
>          return;
>      }
>  
> @@ -726,6 +727,7 @@ static void acpi_processor_idle(void)
>  
>      sched_tick_resume();
>      cpufreq_dbs_timer_resume();
> +    rcu_idle_timer_stop();
>  
>      if ( cpuidle_current_governor->reflect )
>          cpuidle_current_governor->reflect(power);
> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
> index ae9e92b..c426e41 100644
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -743,10 +743,10 @@ static void mwait_idle(void)
>               return;
>       }
>  
> +     rcu_idle_timer_start();
>       cpufreq_dbs_timer_suspend();
> -
>       sched_tick_suspend();
> -     /* sched_tick_suspend() can raise TIMER_SOFTIRQ. Process it now. */
> +     /* Timer related operations can raise TIMER_SOFTIRQ. Process it now. */
>       process_pending_softirqs();
>  
>       /* Interrupts must be disabled for C2 and higher transitions. */
> @@ -756,6 +756,7 @@ static void mwait_idle(void)
>               local_irq_enable();
>               sched_tick_resume();
>               cpufreq_dbs_timer_resume();
> +                rcu_idle_timer_stop();
>               return;
>       }
>  
> @@ -802,6 +803,7 @@ static void mwait_idle(void)
>  
>       sched_tick_resume();
>       cpufreq_dbs_timer_resume();
> +     rcu_idle_timer_stop();
>  
>       if ( cpuidle_current_governor->reflect )
>               cpuidle_current_governor->reflect(power);
> diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
> index f0fdc87..4586f2a 100644
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -84,8 +84,14 @@ struct rcu_data {
>      int cpu;
>      struct rcu_head barrier;
>      long            last_rs_qlen;     /* qlen during the last resched */
> +
> +    /* 3) idle CPUs handling */
> +    struct timer idle_timer;
> +    bool idle_timer_active;
>  };
>  
> +#define RCU_IDLE_TIMER_PERIOD MILLISECS(10)

Isn't this a bit too short? How is it chosen?


>  static DEFINE_PER_CPU(struct rcu_data, rcu_data);
>  
>  static int blimit = 10;
> @@ -402,7 +408,48 @@ int rcu_needs_cpu(int cpu)
>  {
>      struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
>  
> -    return (!!rdp->curlist || rcu_pending(cpu));
> +    return (!!rdp->curlist || rcu_pending(cpu)) && !rdp->idle_timer_active;
> +}
> +
> +/*
> + * Timer for making sure the CPU where a callback is queued does
> + * periodically poke rcu_pedning(), so that it will invoke the callback
> + * not too late after the end of the grace period.
> + */
> +void rcu_idle_timer_start()
> +{
> +    struct rcu_data *rdp = &this_cpu(rcu_data);
> +
> +    if (likely(!rdp->curlist))
> +        return;
> +
> +    set_timer(&rdp->idle_timer, NOW() + RCU_IDLE_TIMER_PERIOD);
> +    rdp->idle_timer_active = true;
> +}
> +
> +void rcu_idle_timer_stop()
> +{
> +    struct rcu_data *rdp = &this_cpu(rcu_data);
> +
> +    if (likely(!rdp->idle_timer_active))
> +        return;
> +
> +    rdp->idle_timer_active = false;
> +    stop_timer(&rdp->idle_timer);
> +}
> +
> +static void rcu_idle_timer_handler(void* data)
> +{
> +    /*
> +     * Nothing, really... And in fact, we don't expect to ever get in here,
> +     * as rcu_idle_timer_stop(), called while waking from idle, prevent that
> +     * to happen by stopping the timer before the TIMER_SOFTIRQ handler has
> +     * a chance to run.
> +     *
> +     * But that's fine, because all we want is the CPU that needs to execute
> +     * the callback to be periodically woken up and check rcu_pending().
> +     */
> +    ASSERT_UNREACHABLE();
>  }
>  
>  void rcu_check_callbacks(int cpu)
> @@ -423,6 +470,8 @@ static void rcu_move_batch(struct rcu_data *this_rdp, 
> struct rcu_head *list,
>  static void rcu_offline_cpu(struct rcu_data *this_rdp,
>                              struct rcu_ctrlblk *rcp, struct rcu_data *rdp)
>  {
> +    kill_timer(&rdp->idle_timer);
> +
>      /* If the cpu going offline owns the grace period we can block
>       * indefinitely waiting for it, so flush it here.
>       */
> @@ -451,6 +500,7 @@ static void rcu_init_percpu_data(int cpu, struct 
> rcu_ctrlblk *rcp,
>      rdp->qs_pending = 0;
>      rdp->cpu = cpu;
>      rdp->blimit = blimit;
> +    init_timer(&rdp->idle_timer, rcu_idle_timer_handler, (void*) rdp, cpu);
>  }
>  
>  static int cpu_callback(
> diff --git a/xen/include/xen/rcupdate.h b/xen/include/xen/rcupdate.h
> index 561ac43..3402eb5 100644
> --- a/xen/include/xen/rcupdate.h
> +++ b/xen/include/xen/rcupdate.h
> @@ -149,4 +149,7 @@ int rcu_barrier(void);
>  void rcu_idle_enter(unsigned int cpu);
>  void rcu_idle_exit(unsigned int cpu);
>  
> +void rcu_idle_timer_start(void);
> +void rcu_idle_timer_stop(void);
> +
>  #endif /* __XEN_RCUPDATE_H */
> 

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