* [PATCH 0/4] toyrcu: replace ACCESS_ONCE() and fix typos
@ 2019-04-23 13:57 Junchang Wang
2019-04-23 13:57 ` [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp Junchang Wang
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Junchang Wang @ 2019-04-23 13:57 UTC (permalink / raw)
To: paulmck; +Cc: perfbook, Junchang Wang
Hi Paul and list,
I'm reading Chapter Toy RCU, and the following are some corrections.
Most of them focus on replacing ACCESS_ONCE() with READ_ONCE()
and WRITE_ONCE(). Please let me know if they look OK.
Thanks,
--Junchang
--
Junchang Wang (4):
Replace the plain accesses and the use of ACCESS_ONCE() with
READ_ONCE()/WRITE_ONCE().
ACCESS_ONCE() with READ_ONCE()/WRITE_ONCE()
Replace the plain accesses and the use of ACCESS_ONCE() with
READ_ONCE()/WRITE_ONCE().
toyrcu: fix typo.
CodeSamples/defer/rcu.c | 5 +++--
CodeSamples/defer/rcu.h | 5 +++--
CodeSamples/defer/rcu_nest.c | 5 +++--
CodeSamples/defer/rcu_nest.h | 8 +-------
CodeSamples/defer/rcu_qs.c | 5 +++--
CodeSamples/defer/rcu_qs.h | 4 ++--
appendix/toyrcu/toyrcu.tex | 28 ++++++++++++++--------------
7 files changed, 29 insertions(+), 31 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp
2019-04-23 13:57 [PATCH 0/4] toyrcu: replace ACCESS_ONCE() and fix typos Junchang Wang
@ 2019-04-23 13:57 ` Junchang Wang
2019-04-23 15:33 ` Akira Yokosawa
2019-04-23 13:57 ` [PATCH 2/4] rcu_nest: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_gp_ctr Junchang Wang
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Junchang Wang @ 2019-04-23 13:57 UTC (permalink / raw)
To: paulmck; +Cc: perfbook, Junchang Wang
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@gmail.com>
---
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);
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)) {
/*@@@ 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
@@ -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 }
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/4] rcu_nest: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_gp_ctr
2019-04-23 13:57 [PATCH 0/4] toyrcu: replace ACCESS_ONCE() and fix typos Junchang Wang
2019-04-23 13:57 ` [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp Junchang Wang
@ 2019-04-23 13:57 ` Junchang Wang
2019-04-23 13:57 ` [PATCH 3/4] rcu_qs: Use READ_ONCE() AND " Junchang Wang
2019-04-23 13:57 ` [PATCH 4/4] toyrcu: fix typo Junchang Wang
3 siblings, 0 replies; 7+ messages in thread
From: Junchang Wang @ 2019-04-23 13:57 UTC (permalink / raw)
To: paulmck; +Cc: perfbook, Junchang Wang
Replace the plain accesses and the use of ACCESS_ONCE() with
READ_ONCE()/WRITE_ONCE(). And then update the tex file accordingly.
Remove the goto statement in rcu_read_lock() because it seems this goto
logic has been discarded. If we should keep this code snippet, could you
please let me know the goal of this code snippet? Then we should update
the tex file accordingly.
Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
CodeSamples/defer/rcu_nest.c | 5 +++--
CodeSamples/defer/rcu_nest.h | 8 +-------
appendix/toyrcu/toyrcu.tex | 8 ++++----
3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/CodeSamples/defer/rcu_nest.c b/CodeSamples/defer/rcu_nest.c
index 64e4087..845be6f 100644
--- a/CodeSamples/defer/rcu_nest.c
+++ b/CodeSamples/defer/rcu_nest.c
@@ -35,7 +35,7 @@ void synchronize_rcu(void)
/* Advance to a new grace-period number, enforce ordering. */
- rcu_gp_ctr += RCU_GP_CTR_BOTTOM_BIT;
+ WRITE_ONCE(rcu_gp_ctr, READ_ONCE(rcu_gp_ctr) + RCU_GP_CTR_BOTTOM_BIT);
smp_mb();
/*
@@ -45,7 +45,8 @@ void synchronize_rcu(void)
for_each_thread(t) {
while (rcu_gp_ongoing(t) &&
- ((per_thread(rcu_reader_gp, t) - rcu_gp_ctr) < 0)) {
+ ((per_thread(rcu_reader_gp, t) -
+ READ_ONCE(rcu_gp_ctr)) < 0)) {
/*@@@ poll(NULL, 0, 10); */
barrier();
}
diff --git a/CodeSamples/defer/rcu_nest.h b/CodeSamples/defer/rcu_nest.h
index bcc4cde..7bb99bc 100644
--- a/CodeSamples/defer/rcu_nest.h
+++ b/CodeSamples/defer/rcu_nest.h
@@ -49,18 +49,12 @@ static void rcu_read_lock(void)
*/
rrgp = &__get_thread_var(rcu_reader_gp);
-retry:
tmp = *rrgp;
if ((tmp & RCU_GP_CTR_NEST_MASK) == 0)
- tmp = rcu_gp_ctr;
+ tmp = READ_ONCE(rcu_gp_ctr);
tmp++;
*rrgp = tmp;
smp_mb();
- if (((tmp & RCU_GP_CTR_NEST_MASK) == 1) &&
- ((rcu_gp_ctr - tmp) > (RCU_GP_CTR_NEST_MASK << 8)) != 0) {
- (*rrgp)--;
- goto retry;
- }
}
static void rcu_read_unlock(void)
diff --git a/appendix/toyrcu/toyrcu.tex b/appendix/toyrcu/toyrcu.tex
index d9a7055..39126d2 100644
--- a/appendix/toyrcu/toyrcu.tex
+++ b/appendix/toyrcu/toyrcu.tex
@@ -1420,7 +1420,7 @@ variables.
6 rrgp = &__get_thread_var(rcu_reader_gp);
7 tmp = *rrgp;
8 if ((tmp & RCU_GP_CTR_NEST_MASK) == 0)
- 9 tmp = ACCESS_ONCE(rcu_gp_ctr);
+ 9 tmp = READ_ONCE(rcu_gp_ctr);
10 tmp++;
11 *rrgp = tmp;
12 smp_mb();
@@ -1440,13 +1440,13 @@ variables.
26
27 smp_mb();
28 spin_lock(&rcu_gp_lock);
-29 ACCESS_ONCE(rcu_gp_ctr) +=
-30 RCU_GP_CTR_BOTTOM_BIT;
+29 WRITE_ONCE(rcu_gp_ctr, READ_ONCE(rcu_gp_ctr) +
+30 RCU_GP_CTR_BOTTOM_BIT);
31 smp_mb();
32 for_each_thread(t) {
33 while (rcu_gp_ongoing(t) &&
34 ((per_thread(rcu_reader_gp, t) -
-35 rcu_gp_ctr) < 0)) {
+35 READ_ONCE(rcu_gp_ctr)) < 0)) {
36 poll(NULL, 0, 10);
37 }
38 }
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/4] rcu_qs: Use READ_ONCE() AND WRITE_ONCE() for shared variable rcu_gp_ctr
2019-04-23 13:57 [PATCH 0/4] toyrcu: replace ACCESS_ONCE() and fix typos Junchang Wang
2019-04-23 13:57 ` [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp Junchang Wang
2019-04-23 13:57 ` [PATCH 2/4] rcu_nest: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_gp_ctr Junchang Wang
@ 2019-04-23 13:57 ` Junchang Wang
2019-04-23 13:57 ` [PATCH 4/4] toyrcu: fix typo Junchang Wang
3 siblings, 0 replies; 7+ messages in thread
From: Junchang Wang @ 2019-04-23 13:57 UTC (permalink / raw)
To: paulmck; +Cc: perfbook, Junchang Wang
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@gmail.com>
---
CodeSamples/defer/rcu_qs.c | 5 +++--
CodeSamples/defer/rcu_qs.h | 4 ++--
appendix/toyrcu/toyrcu.tex | 10 +++++-----
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/CodeSamples/defer/rcu_qs.c b/CodeSamples/defer/rcu_qs.c
index 27e45f7..dc17162 100644
--- a/CodeSamples/defer/rcu_qs.c
+++ b/CodeSamples/defer/rcu_qs.c
@@ -41,7 +41,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);
smp_mb();
/*
@@ -51,7 +51,8 @@ void synchronize_rcu(void)
for_each_thread(t) {
while (rcu_gp_ongoing(t) &&
- ((per_thread(rcu_reader_qs_gp, t) - rcu_gp_ctr) < 0)) {
+ ((per_thread(rcu_reader_qs_gp, t) -
+ READ_ONCE(rcu_gp_ctr)) < 0)) {
/* @@@ poll(NULL, 0, 10); */
barrier();
}
diff --git a/CodeSamples/defer/rcu_qs.h b/CodeSamples/defer/rcu_qs.h
index 3bb45a4..d8be43e 100644
--- a/CodeSamples/defer/rcu_qs.h
+++ b/CodeSamples/defer/rcu_qs.h
@@ -59,14 +59,14 @@ static void rcu_read_unlock(void)
static void rcu_quiescent_state(void)
{
smp_mb();
- __get_thread_var(rcu_reader_qs_gp) = ACCESS_ONCE(rcu_gp_ctr) + 1;
+ __get_thread_var(rcu_reader_qs_gp) = READ_ONCE(rcu_gp_ctr) + 1;
smp_mb();
}
static void rcu_thread_offline(void)
{
smp_mb();
- __get_thread_var(rcu_reader_qs_gp) = ACCESS_ONCE(rcu_gp_ctr);
+ __get_thread_var(rcu_reader_qs_gp) = READ_ONCE(rcu_gp_ctr);
smp_mb();
}
diff --git a/appendix/toyrcu/toyrcu.tex b/appendix/toyrcu/toyrcu.tex
index 39126d2..c25d205 100644
--- a/appendix/toyrcu/toyrcu.tex
+++ b/appendix/toyrcu/toyrcu.tex
@@ -1704,7 +1704,7 @@ overhead.
10 {
11 smp_mb();
12 __get_thread_var(rcu_reader_qs_gp) =
- 13 ACCESS_ONCE(rcu_gp_ctr) + 1;
+ 13 READ_ONCE(rcu_gp_ctr) + 1;
14 smp_mb();
15 }
16
@@ -1712,7 +1712,7 @@ overhead.
18 {
19 smp_mb();
20 __get_thread_var(rcu_reader_qs_gp) =
- 21 ACCESS_ONCE(rcu_gp_ctr);
+ 21 READ_ONCE(rcu_gp_ctr);
22 smp_mb();
23 }
24
@@ -1764,7 +1764,7 @@ to prevent any code prior to the quiescent state (including possible
RCU read-side critical sections) from being reordered
into the quiescent state.
Lines~12-13 pick up a copy of the global \co{rcu_gp_ctr}, using
-\co{ACCESS_ONCE()} to ensure that the compiler does not employ any
+\co{READ_ONCE()} to ensure that the compiler does not employ any
optimizations that would result in \co{rcu_gp_ctr} being fetched
more than once,
and then adds one to the value fetched and stores it into
@@ -1837,12 +1837,12 @@ quiescent state.
4
5 smp_mb();
6 spin_lock(&rcu_gp_lock);
- 7 rcu_gp_ctr += 2;
+ 7 WRITE_ONCE(rcu_gp_ctr, READ_ONCE(rcu_gp_ctr) + 2);
8 smp_mb();
9 for_each_thread(t) {
10 while (rcu_gp_ongoing(t) &&
11 ((per_thread(rcu_reader_qs_gp, t) -
- 12 rcu_gp_ctr) < 0)) {
+ 12 READ_ONCE(rcu_gp_ctr)) < 0)) {
13 poll(NULL, 0, 10);
14 }
15 }
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/4] toyrcu: fix typo.
2019-04-23 13:57 [PATCH 0/4] toyrcu: replace ACCESS_ONCE() and fix typos Junchang Wang
` (2 preceding siblings ...)
2019-04-23 13:57 ` [PATCH 3/4] rcu_qs: Use READ_ONCE() AND " Junchang Wang
@ 2019-04-23 13:57 ` Junchang Wang
3 siblings, 0 replies; 7+ messages in thread
From: Junchang Wang @ 2019-04-23 13:57 UTC (permalink / raw)
To: paulmck; +Cc: perfbook, Junchang Wang
This typo did confuse me when I was jumping from perfbook to the code.
So fix it.
Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
appendix/toyrcu/toyrcu.tex | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/appendix/toyrcu/toyrcu.tex b/appendix/toyrcu/toyrcu.tex
index c25d205..5627c46 100644
--- a/appendix/toyrcu/toyrcu.tex
+++ b/appendix/toyrcu/toyrcu.tex
@@ -491,7 +491,7 @@ scheme that is more favorable to writers.
\end{listing}
Listing~\ref{lst:app:toyrcu:RCU Read-Side Using Global Reference-Count Pair}
-(\path{rcu_rcgp.h})
+(\path{rcu_rcpg.h})
shows the read-side primitives of an RCU implementation that uses a pair
of reference counters (\co{rcu_refcnt[]}),
along with a global index that
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp
2019-04-23 13:57 ` [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp Junchang Wang
@ 2019-04-23 15:33 ` Akira Yokosawa
2019-04-24 2:28 ` Junchang Wang
0 siblings, 1 reply; 7+ messages in thread
From: Akira Yokosawa @ 2019-04-23 15:33 UTC (permalink / raw)
To: Junchang Wang, paulmck; +Cc: perfbook
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@gmail.com>
> ---
> 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.
> /*@@@ 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).
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 }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp
2019-04-23 15:33 ` Akira Yokosawa
@ 2019-04-24 2:28 ` Junchang Wang
0 siblings, 0 replies; 7+ messages in thread
From: Junchang Wang @ 2019-04-24 2:28 UTC (permalink / raw)
To: Akira Yokosawa; +Cc: Paul McKenney, perfbook
On Tue, Apr 23, 2019 at 11:33 PM Akira Yokosawa <akiyks@gmail.com> 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@gmail.com>
> > ---
> > 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 }
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-24 2:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23 13:57 [PATCH 0/4] toyrcu: replace ACCESS_ONCE() and fix typos Junchang Wang
2019-04-23 13:57 ` [PATCH 1/4] rcu: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_reader_gp Junchang Wang
2019-04-23 15:33 ` Akira Yokosawa
2019-04-24 2:28 ` Junchang Wang
2019-04-23 13:57 ` [PATCH 2/4] rcu_nest: Use READ_ONCE() and WRITE_ONCE() for shared variable rcu_gp_ctr Junchang Wang
2019-04-23 13:57 ` [PATCH 3/4] rcu_qs: Use READ_ONCE() AND " Junchang Wang
2019-04-23 13:57 ` [PATCH 4/4] toyrcu: fix typo Junchang Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.