Re: [PATCH 2/2] Let a writer thread go back to the original CPU in the toy RCU pseudocode

[Hitoshi Mitake <mitake.hitoshi@gmail.com>]

Hi Paul,

At Sat, 20 Feb 2016 21:30:20 -0800,
Paul E. McKenney wrote:
> 
> On Sat, Feb 20, 2016 at 05:52:02PM +0900, Hitoshi Mitake wrote:
> > This commit a little bit improves the logic of a writer thread. The
> > new logic lets the writer go back to the original CPU after run_on()
> > the final CPU.
> > 
> > Of course it is a trivial improvement and the original two lines
> > pseudocode is beautiful. If it isn't worth to apply, please ignore it.
> > 
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@gmail.com>
> 
> Hello, Hitoshi,
> 
> Thank you for the submission, but each addition of this sort invites
> another.  For example, you capture the initial CPU, but suppose that
> the thread was originally bound to a pair of CPUs?  Wouldn't you then
> want to restore the binding to that pair of CPUs?
> 
> But if you did that, suppose that both of those CPUs went offline
> while going through the for_each_online_cpu() loop?  Wouldn't you
> need to instead bind to some CPUs that were actually online?
> 
> And suppose that some other CPU changed the binding of the thread
> just after you captured the initial binding?  Wouldn't you want to
> reflect that change (give or take CPU hotplug operations) at the
> end of the loop?
> 
> And it just gets worse from there.  So we need to keep this one
> simple, which means that I cannot accept this patch.

Thanks a lot for your review. I couldn't consider about atomicity of
the CPU state, the affinity of the thread, etc. I assumed the
atomicity is provided and this assumption is wrong.

Again, thanks for your time and review. Your review improved my
understanding of RCU :)

# BTW, how do you think about the change of the figure in the 1st
# patch? Of couse I'm not sure it is worth to apply or not.

Thanks,
Hitoshi

> 
> 							Thanx, Paul
> 
> > ---
> >  defer/rcuintro.tex | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/defer/rcuintro.tex b/defer/rcuintro.tex
> > index 03d6e12..135922c 100644
> > --- a/defer/rcuintro.tex
> > +++ b/defer/rcuintro.tex
> > @@ -166,8 +166,11 @@ quite complex, a toy implementation is exceedingly simple:
> >  \begin{minipage}[t]{\columnwidth}
> >  \scriptsize
> >  \begin{verbatim}
> > -  1 for_each_online_cpu(cpu)
> > -  2   run_on(cpu);
> > +  1 int orig_cpu = smp_processor_id();
> > +  2 for_each_online_cpu(cpu)
> > +  3   run_on(cpu);
> > +  4 if (orig_cpu != smp_processor_id())
> > +  5   run_on(orig_cpu);
> >  \end{verbatim}
> >  \end{minipage}
> >  \vspace{5pt}
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe perfbook" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
>