Re: [Xen-devel] [PATCH 3/5] xen: RCU/x86/ARM: discount CPUs that were idle when grace period started.

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Mon, 2017-08-07 at 02:35 -0600, Jan Beulich wrote:
> > > > Dario Faggioli <dario.faggioli@xxxxxxxxxx> 07/27/17 10:01 AM
> > > > >>>
> > --- a/xen/arch/x86/acpi/cpu_idle.c
> > +++ b/xen/arch/x86/acpi/cpu_idle.c
> > 
> > @@ -433,12 +435,14 @@ static void acpi_idle_do_entry(struct
> > acpi_processor_cx *cx)
> > because chipsets cannot guarantee that STPCLK# signal
> > gets asserted in time to freeze execution properly. */
> > inl(pmtmr_ioport);
> > -        return;
> > +        break;
> 
>      >case ACPI_CSTATE_EM_HALT:
> 
> Wiuld be nice if you also added blank lines between the break-s and
> the
> subsequent case labels.
> 
Ok.

> > @@ -787,6 +792,8 @@ static void mwait_idle(void)
> > irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
> 
>  >
> > /* Now back in C0. */
> > +    rcu_idle_exit(cpu);
> > +
> > update_idle_stats(power, cx, before, after);
> > local_irq_enable();
> 
>  
> Compared to the ACPI code, the window is quite a bit larger here. Any
> reason
> you can't put these just around the mwait_idle_with_hints()
> invocation, which
> would match what you do for the ACPI-based driver?
> 
No, no reasons not to do that. I'll do it.

> > @@ -248,7 +249,14 @@ static void rcu_start_batch(struct rcu_ctrlblk
> > *rcp)
> > smp_wmb();
> > rcp->cur++;
> 
>  >
> > -        cpumask_copy(&rcp->cpumask, &cpu_online_map);
> > +       /*
> > +        * Accessing idle_cpumask before incrementing rcp->cur
> > needs a
> > +        * Barrier  Otherwise it can cause tickless idle CPUs to be
> > +        * included in rcp->cpumask, which will extend graceperiods
> > +        * unnecessarily.
> > +        */
> > +        smp_mb();
> > +        cpumask_andnot(&rcp->cpumask, &cpu_online_map, &rcp-
> > >idle_cpumask);
> 
> I have some trouble with understanding the comment: You don't access
> ->idle_cpumask before you increment ->cur.
>
It comes verbatim from the Linux commit. You're not the first one that
finds it unclear, and I don't like it either.

So, this is the Linux patch:

        if (rcp->next_pending &&
                        rcp->completed == rcp->cur) {
-               /* Can't change, since spin lock held. */
-               cpus_andnot(rsp->cpumask, cpu_online_map, nohz_cpu_mask);
-
                rcp->next_pending = 0;
-               /* next_pending == 0 must be visible in 
__rcu_process_callbacks()
-                * before it can see new value of cur.
+               /*
+                * next_pending == 0 must be visible in
+                * __rcu_process_callbacks() before it can see new value of cur.
                 */
                smp_wmb();
                rcp->cur++;
+
+               /*
+                * Accessing nohz_cpu_mask before incrementing rcp->cur needs a
+                * Barrier  Otherwise it can cause tickless idle CPUs to be
+                * included in rsp->cpumask, which will extend graceperiods
+                * unnecessarily.
+                */
+               smp_mb();
+               cpus_andnot(rsp->cpumask, cpu_online_map, nohz_cpu_mask);
+

_I_think_ what the original author meant was something along the line
of <<Accessing nohz_cpu_mask before incrementing rcp->cur is unsafe.
Therefore, let's access it afterwords, and put a barrier in between.>>

But yeah, as said, I don't like it myself. In fact, it's the same exact
wording used in the changelog of the patch (Linux commit
c3f5902325d3053986e7359f706581d8f032e72f), but while it is fine there,
here is completely misleading, as it does not comment/describe the
final look of the code.

I'm going to change it.

> (Also, as a general remark,
> please go through patch description and comments again to correct
> spelling and alike issues.)
> 
Sure.

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