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

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

>>> Dario Faggioli <dario.faggioli@xxxxxxxxxx> 07/27/17 10:01 AM >>>
>Instead of having the CPU where a callback is queued, busy
>looping on rcu_pending(), use a timer.

Isn't this rcu_needs_cpu(), according to patch 4?

>--- 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)
>+    rcu_idle_timer_start();

Is the ordering wrt cpufreq handling here arbitrary? If so, wouldn't it be
better to suspend cpufreq handling before starting the idle timer (and
also invert the ordering when going back to normal operation below)?


At which point I'd then wonder whether this couldn't be integrated into
sched_tick_{suspend,resume}(), avoiding arch-specific code to be

>@@ -756,6 +756,7 @@ static void mwait_idle(void)
>        local_irq_enable();
>        sched_tick_resume();
>        cpufreq_dbs_timer_resume();
>+                rcu_idle_timer_stop();

Indentation looks odd here, but I can't exclude this being an effect produced
by my mail web frontend.

>--- 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;

If I'm not mistaken someone else had already commented on this: If this
is (mostly) arbitrary, please say so in a comment.

>@@ -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;

Please take the opportunity and drop the pointless !! here (unless it's needed
for better matching up with the Linux original).

>+ * 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;

I would have expected this to be the inverse of the original condition in
rcu_needs_cpu() - why is there no rcu_pending() invocation 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);

Again, unless it is this bogus way in the Linux original, please drop the
pointless cast, or at least correct its style.


Xen-devel mailing list



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