Re: [Xen-devel] [PATCH v2 1/3] xen: RCU: let the RCU idle timer handler run

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.17 at 12:15, <dario.faggioli@xxxxxxxxxx> wrote:
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -465,7 +465,21 @@ void rcu_idle_timer_stop()
>          return;
>  
>      rdp->idle_timer_active = false;
> -    stop_timer(&rdp->idle_timer);
> +
> +    /*
> +     * In general, as the CPU is becoming active again, we don't need the
> +     * idle timer, and so we want to stop it.
> +     *
> +     * However, in case we are here because idle_timer has (just) fired and
> +     * has woken up the CPU, we skip stop_timer() now. In fact, if we stop
> +     * it, then the TIMER_SOFTIRQ handler wouldn't find idle_timer among the
> +     * active timers any longer, and hence won't call 
> rcu_idle_timer_handler().

I think it would help if you said explicitly that the softirq run
necessarily happens after this code ran.

> +     * Therefore, if we see that the timer is expired already, leave it 
> alone.
> +     * It will be finally deactiveted by the TIMER_SOFTIRQ handler.

deactivated

> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -332,6 +332,23 @@ void stop_timer(struct timer *timer)
>  }
>  
>  
> +bool timer_is_expired(struct timer *timer, s_time_t now)

If you call the parameter now, why is it needed? Wouldn't it be
even more accurate if you instead used ...

> +{
> +    unsigned long flags;
> +    bool ret = false;
> +
> +    if ( !timer_lock_irqsave(timer, flags) )
> +        return ret;
> +
> +    if ( active_timer(timer) && timer->expires <= now )

... NOW() here?

Jan


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