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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Wed, 2017-08-30 at 01:18 -0600, Jan Beulich wrote:
> > > > On 29.08.17 at 18:06, <george.dunlap@xxxxxxxxxx> wrote:
> > 
> > On 08/22/2017 02:04 PM, Jan Beulich wrote:
> > > > > > On 18.08.17 at 20:04, <dario.faggioli@xxxxxxxxxx> wrote:
> > > > 
> > > > --- a/xen/arch/x86/cpu/mwait-idle.c
> > > > +++ b/xen/arch/x86/cpu/mwait-idle.c
> > > > @@ -741,9 +741,8 @@ static void mwait_idle(void)
> > > >         }
> > > >  
> > > >         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();
> > > 
> > > Is this a leftover from v1? Otherwise, why do you do the
> > > adjustment
> > > here but not in acpi_processor_idle()?
> > > 
> > > > --- a/xen/common/rcupdate.c
> > > > +++ b/xen/common/rcupdate.c
> > > > @@ -84,8 +84,37 @@ 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;
> > > >  };
> > > >  
> > > > +/*
> > > > + * If a CPU with RCU callbacks queued goes idle, when the
> > > > grace period is
> > > > + * not finished yet, how can we make sure that the callbacks
> > > > will 
> > 
> > eventually
> > > > + * be executed? In Linux (2.6.21, the first "tickless idle"
> > > > Linux kernel),
> > > > + * the periodic timer tick would not be stopped for such CPU.
> > > > Here in Xen,
> > > > + * we (may) don't even have a periodic timer tick, so we need
> > > > to use a
> > > > + * special purpose timer.
> > > > + *
> > > > + * Such timer:
> > > > + * 1) is armed only when a CPU with an RCU callback(s) queued
> > > > goes idle
> > > > + *    before the end of the current grace period (_not_ for
> > > > any CPUs that
> > > > + *    go idle!);
> > > > + * 2) when it fires, it is only re-armed if the grace period
> > > > is still
> > > > + *    running;
> > > > + * 3) it is stopped immediately, if the CPU wakes up from idle
> > > > and
> > > > + *    resumes 'normal' execution.
> > > > + *
> > > > + * About how far in the future the timer should be programmed
> > > > each time,
> > > > + * it's hard to tell (guess!!). Since this mimics Linux's
> > > > periodic timer
> > > > + * tick, take values used there as an indication. In Linux
> > > > 2.6.21, tick
> > > > + * period can be 10ms, 4ms, 3.33ms or 1ms. Let's use 10ms, to
> > > > enable
> > > > + * at least some power saving on the CPU that is going idle.
> > > > + */
> > > > +#define RCU_IDLE_TIMER_PERIOD MILLISECS(10)
> > > 
> > > With you even mentioning that the original Linux code has ways
> > > to use different values, wouldn't it be worth allowing this to be
> > > command line controllable?
> > 
> > Dario is on holiday, and I think it would be good to get this
> > functionality in sooner rather than later to shake out as many bugs
> > as
> > possible.  Would you be willing to let the idle timer period be set
> > with
> > a follow-up patch?
> 
So, I'm back, and can do such a patch.

Do we want to enforce a maximum value, to try to at least avoid severe
injuries, even for users that shot themselves in the foot? Or we just
accept anything which is below STIME_MAX?

I personally would only accept values smaller than 100ms (In fact, I
was keeping it below that level in patch 6, where the heuristics was
implemnted) or, if we really want, 1s.

Going above these values is basically equivalent to saying that the bug
this series has fixed was not really a bug! :-/

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: signature.asc
Description: This is a digitally signed message part

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