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

[Dario Faggioli <dario.faggioli@citrix.com>]

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

On Thu, 2017-09-28 at 06:59 -0600, Jan Beulich wrote:
> > > > On 28.09.17 at 12:15, <dario.faggioli@citrix.com> 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.
> 
Ok.

> > --- 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?
> 
So, the cases where you happen to need the current time multiple times,
and you expect the difference between calling NOW() repeatedly, and
using a value sampled at the beginning is small enough, or does not
matter (and therefore you decide to save the overhead).

foo()
{
 s_time_t now = NOW();
 ...
 bar(now);
 ...
 bar2(barbar, now);
 ...
 if ( timer_is_expired(timer, now) )
 {
  ...
 }
}

This is something we do, some of the times. And a function that takes a
'now' parameter, allows both use cases: the (more natural?) one, where
you pass it NOW(), and the least-overhead one, where you pass it a
cached value of NOW().

But I don't feel like arguing too much about this (especially now that
this is patch is the only use case).

If the problem is "just" the parameter (or maybe both the parameter's
and the function's) name(s), I 'd be happy to change the parameter name
to 't', or 'time' (and the function to 'timer_expires_before()'), and
this is my preference.

But if you strongly prefer it to just use NOW() inside, I'll go for it.

Thanks and 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