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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.17 at 10:48, <dario.faggioli@xxxxxxxxxx> wrote:
> On Mon, 2017-08-07 at 02:35 -0600, Jan Beulich wrote:
>> > > > Dario Faggioli <dario.faggioli@xxxxxxxxxx> 07/27/17 10:01 AM
>> > @@ -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.

Perhaps worth submitting a Linux patch too then?

Jan


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