On Tue, Apr 23, 2019 at 11:33 PM Akira Yokosawa <akiyks@xxxxxxxxx> wrote:
>
> Hi Junchang,
>
> On Tue, 23 Apr 2019 21:57:02 +0800, Junchang Wang wrote:
> > Replace the plain accesses and the use of ACCESS_ONCE() with
> > READ_ONCE()/WRITE_ONCE(). And then update the tex file accordingly.
> >
> > Signed-off-by: Junchang Wang <junchangwang@xxxxxxxxx>
> > ---
> > CodeSamples/defer/rcu.c | 5 +++--
> > CodeSamples/defer/rcu.h | 5 +++--
> > appendix/toyrcu/toyrcu.tex | 8 ++++----
> > 3 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/CodeSamples/defer/rcu.c b/CodeSamples/defer/rcu.c
> > index 4b868cc..e4fddbc 100644
> > --- a/CodeSamples/defer/rcu.c
> > +++ b/CodeSamples/defer/rcu.c
> > @@ -35,7 +35,7 @@ void synchronize_rcu(void)
> >
> > /* Advance to a new grace-period number, enforce ordering. */
> >
> > - rcu_gp_ctr += 2;
> > + WRITE_ONCE(rcu_gp_ctr, READ_ONCE(rcu_gp_ctr) + 2);
>
> This is inside of critical section protecting update of rcu_gp_ctr, and
> READ_ONCE() is not necessary. On the other hand, WRITE_ONCE() is necessary
> because other CPUs/threads can do racy read accesses.
>
> > smp_mb();
> >
> > /*
> > @@ -45,7 +45,8 @@ void synchronize_rcu(void)
> >
> > for_each_thread(t) {
> > while ((per_thread(rcu_reader_gp, t) & 0x1) &&
> > - ((per_thread(rcu_reader_gp, t) - rcu_gp_ctr) < 0)) {
> > + ((per_thread(rcu_reader_gp, t) -
> > + READ_ONCE(rcu_gp_ctr)) < 0)) {
>
> This READ_ONCE() is not necessary, neither.
>
> Please refer to Section 4.3.4.4 "Avoiding Data Races" for where to use
> READ/WRITE_ONCE(). You might want to self review 2/4 and 3/4 of this patch
> set.
Hi Akira,
Thanks a lot for pointing me to that! I will read that chapter and
submit a new patch set for your review soon.
I would like to add you to the Reviewed-by. Would that be OK for you?
>
> > /*@@@ poll(NULL, 0, 10); */
> > barrier();
> > }
> > diff --git a/CodeSamples/defer/rcu.h b/CodeSamples/defer/rcu.h
> > index f493f8b..534709e 100644
> > --- a/CodeSamples/defer/rcu.h
> > +++ b/CodeSamples/defer/rcu.h
> > @@ -41,7 +41,8 @@ static inline void rcu_read_lock(void)
> > * periodic per-thread processing.)
> > */
> >
> > - __get_thread_var(rcu_reader_gp) = rcu_gp_ctr + 1;
> > + __get_thread_var(rcu_reader_gp) =
> > + READ_ONCE(rcu_gp_ctr) + 1;
> > smp_mb();
> > }
> >
> > @@ -55,7 +56,7 @@ static inline void rcu_read_unlock(void)
> > */
> >
> > smp_mb();
> > - __get_thread_var(rcu_reader_gp) = rcu_gp_ctr;
> > + __get_thread_var(rcu_reader_gp) = READ_ONCE(rcu_gp_ctr);
> > }
> >
> > extern void synchronize_rcu(void);
> > diff --git a/appendix/toyrcu/toyrcu.tex b/appendix/toyrcu/toyrcu.tex
> > index c0a45a8..d9a7055 100644
> > --- a/appendix/toyrcu/toyrcu.tex
> > +++ b/appendix/toyrcu/toyrcu.tex
>
> Or you can extract the snippet by using \begin{snippet} meta commands.
> That is a preferred way if there is code under CodeSamples/.
> Examples of the scheme can be found in (not yet up-to-date) Style Guide
> (D.3.1.1).
>
Thanks for the comment; this is the right approach.
Then, I am planning to submit two patch sets: the first is to correct
the code, and the second is to use the new snippet scheme.
Does that sound good?
Thanks,
--Junchang
> That said, you are not obliged to do so. Conversion to the new scheme is
> on my to-do list anyway.
>
> Thanks, Akira
>
> > @@ -1223,7 +1223,7 @@ thread-local accesses to one, as is done in the next section.
> > 1 static void rcu_read_lock(void)
> > 2 {
> > 3 __get_thread_var(rcu_reader_gp) =
> > - 4 ACCESS_ONCE(rcu_gp_ctr) + 1;
> > + 4 READ_ONCE(rcu_gp_ctr) + 1;
> > 5 smp_mb();
> > 6 }
> > 7
> > @@ -1231,7 +1231,7 @@ thread-local accesses to one, as is done in the next section.
> > 9 {
> > 10 smp_mb();
> > 11 __get_thread_var(rcu_reader_gp) =
> > -12 ACCESS_ONCE(rcu_gp_ctr);
> > +12 READ_ONCE(rcu_gp_ctr);
> > 13 }
> > 14
> > 15 void synchronize_rcu(void)
> > @@ -1240,12 +1240,12 @@ thread-local accesses to one, as is done in the next section.
> > 18
> > 19 smp_mb();
> > 20 spin_lock(&rcu_gp_lock);
> > -21 ACCESS_ONCE(rcu_gp_ctr) += 2;
> > +21 WRITE_ONCE(rcu_gp_ctr, READ_ONCE(rcu_gp_ctr) + 2);
> > 22 smp_mb();
> > 23 for_each_thread(t) {
> > 24 while ((per_thread(rcu_reader_gp, t) & 0x1) &&
> > 25 ((per_thread(rcu_reader_gp, t) -
> > -26 ACCESS_ONCE(rcu_gp_ctr)) < 0)) {
> > +26 READ_ONCE(rcu_gp_ctr)) < 0)) {
> > 27 poll(NULL, 0, 10);
> > 28 }
> > 29 }
> >
>
