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

["Jan Beulich" <jbeulich@xxxxxxxx>]

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

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)?

>-
>sched_tick_suspend();

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

>@@ -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;
>};
 >
>+#define RCU_IDLE_TIMER_PERIOD MILLISECS(10)

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.

Jan


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