[PATCH 0/2] Patches to chapter Deferred Processing

[PATCH 0/2] Patches to chapter Deferred Processing

[Junchang Wang]

Hi Paul and list,

Attached are two patches for routetorture.h. Please take a look.

One of my remaining question is about the two smp_mb() in routetorture.h. Take
the smp_mb() in perftest() as example, my understanding is that it is used to
prevent the access to nthreads_running and access to goflag from being
reorganized. If that's the case, don't we need a paired memory barrier
instruction in perftest_reader()? Specifically, in between line 105
(atomic_inc(&nthreads_running);) and line 106 (gf = READ_ONCE(goflag);). 

Another puzzle is that we are using shared memory 'pap' to transfer statistic
results from working threads, e.g. perftest_reader, to main thread, e.g.
perftest. Do we need a smp_mb() at the tail (before instruction return) of
function perftest_reader to force the results being written before the thread
terminates? In other words, I'm not sure if the two events, writing to shared
memory pap and thread termination, could be reordered.  Hints are welcome!

Thanks,
--Jason

Junchang Wang (2):
  Fix typos in help messages
  routetorture.h: Switch from ACCESS_ONCE() to READ_ONCE()/WRITE_ONCE()

 CodeSamples/defer/routetorture.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/2] routetorture.h: Fix typos in help messages

[Junchang Wang]

Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
 CodeSamples/defer/routetorture.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/CodeSamples/defer/routetorture.h b/CodeSamples/defer/routetorture.h
index fa4fdab..86693e9 100644
--- a/CodeSamples/defer/routetorture.h
+++ b/CodeSamples/defer/routetorture.h
@@ -346,10 +346,10 @@ void usage(char *progname, const char *format, ...)
 	fprintf(stderr, "\t\tNumber of elements, defaults to 100.  Must be\n");
 	fprintf(stderr, "\t\t1 or greater.\n");
 	fprintf(stderr, "\t--nreaders\n");
-	fprintf(stderr, "\t\tNumber of readers, defaults to 1.  Must be 1\n");
+	fprintf(stderr, "\t\tNumber of readers, defaults to 7.  Must be 1\n");
 	fprintf(stderr, "\t\tor greater.\n");
 	fprintf(stderr, "\t--nupdaters\n");
-	fprintf(stderr, "\t\tNumber of updaters, defaults to 1.  Must be 1\n");
+	fprintf(stderr, "\t\tNumber of updaters, defaults to 7.  Must be 1\n");
 	fprintf(stderr, "\t\tor greater.\n");
 	fprintf(stderr, "\t--cpustride\n");
 	fprintf(stderr, "\t\tStride when spreading threads across CPUs,\n");
-- 
2.7.4

[PATCH 2/2] routetorture.h: Switch from ACCESS_ONCE() to READ_ONCE()/WRITE_ONCE()

[Junchang Wang]

Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
 CodeSamples/defer/routetorture.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/CodeSamples/defer/routetorture.h b/CodeSamples/defer/routetorture.h
index 86693e9..b0d642a 100644
--- a/CodeSamples/defer/routetorture.h
+++ b/CodeSamples/defer/routetorture.h
@@ -104,7 +104,7 @@ void *perftest_reader(void *arg)
 	/* Announce our presence and enter the test loop. */
 	atomic_inc(&nthreads_running);
 	for (;;) {
-		gf = ACCESS_ONCE(goflag);
+		gf = READ_ONCE(goflag);
 		if (gf != GOFLAG_RUN) {
 			if (gf == GOFLAG_STOP)
 				break;
@@ -149,7 +149,7 @@ void perftest(void)
 	pap = malloc(sizeof(*pap) * nreaders);
 	BUG_ON(pap == NULL);
 	atomic_set(&nthreads_running, 0);
-	goflag = GOFLAG_INIT;
+	WRITE_ONCE(goflag, GOFLAG_INIT);

 	/* Populate route table. */
 	for (i = 0; i < nelems; i++)
@@ -173,9 +173,9 @@ void perftest(void)

 	/* Run the test. */
 	starttime = get_microseconds();
-	ACCESS_ONCE(goflag) = GOFLAG_RUN;
+	WRITE_ONCE(goflag, GOFLAG_RUN);
 	poll(NULL, 0, duration);
-	ACCESS_ONCE(goflag) = GOFLAG_STOP;
+	WRITE_ONCE(goflag, GOFLAG_STOP);
 	starttime = get_microseconds() - starttime;
 	wait_all_threads();

@@ -225,7 +225,7 @@ void *stresstest_updater(void *arg)
 	/* Announce our presence and enter the test loop. */
 	atomic_inc(&nthreads_running);
 	for (;;) {
-		gf = ACCESS_ONCE(goflag);
+		gf = READ_ONCE(goflag);
 		if (gf != GOFLAG_RUN) {
 			if (gf == GOFLAG_STOP)
 				break;
@@ -286,7 +286,7 @@ void stresstest(void)
 	pap = malloc(sizeof(*pap) * nupdaters);
 	BUG_ON(pap == NULL);
 	atomic_set(&nthreads_running, 0);
-	goflag = GOFLAG_INIT;
+	WRITE_ONCE(goflag, GOFLAG_INIT);

 	for (i = 0; i < nupdaters; i++) {
 		pap[i].myid = i;
@@ -306,9 +306,9 @@ void stresstest(void)

 	/* Run the test. */
 	starttime = get_microseconds();
-	ACCESS_ONCE(goflag) = GOFLAG_RUN;
+	WRITE_ONCE(goflag, GOFLAG_RUN);
 	poll(NULL, 0, duration);
-	ACCESS_ONCE(goflag) = GOFLAG_STOP;
+	WRITE_ONCE(goflag, GOFLAG_STOP);
 	starttime = get_microseconds() - starttime;
 	wait_all_threads();

-- 
2.7.4

Re: [PATCH 0/2] Patches to chapter Deferred Processing

[Paul E. McKenney]

On Thu, Jun 08, 2017 at 11:16:31AM +0800, Junchang Wang wrote:
> Hi Paul and list,
> 
> Attached are two patches for routetorture.h. Please take a look.

Good catches, queued and pushed, thank you!

> One of my remaining question is about the two smp_mb() in routetorture.h. Take
> the smp_mb() in perftest() as example, my understanding is that it is used to
> prevent the access to nthreads_running and access to goflag from being
> reorganized. If that's the case, don't we need a paired memory barrier
> instruction in perftest_reader()? Specifically, in between line 105
> (atomic_inc(&nthreads_running);) and line 106 (gf = READ_ONCE(goflag);). 

This one is unusual.  The ordering controls not correctness, but
time duration.  Plus the assignment of GOFLAG_RUN to goflag cannot
happen until after the effect of the atomic_inc() propagates back.
Interestingly enough, this code does fully not take control dependencies
into account, which could reduce the number of memory barriers.

In short, this situation is unusual and makes complex reliance on
implicit ordering guarantees.

But it might well have bugs.  If you have time, one
approach would be to read https://lwn.net/Articles/718628/ and
https://lwn.net/Articles/720550/, download the tool described, and try
to create the corresponding litmus test.  Either way, please let me know
your intentions.  This would be a really cool way for you to learn the
latest and greatest stuff about the Linux kernel memory model!

> Another puzzle is that we are using shared memory 'pap' to transfer statistic
> results from working threads, e.g. perftest_reader, to main thread, e.g.
> perftest. Do we need a smp_mb() at the tail (before instruction return) of
> function perftest_reader to force the results being written before the thread
> terminates? In other words, I'm not sure if the two events, writing to shared
> memory pap and thread termination, could be reordered.  Hints are welcome!

The pthread_create() system call guarantees that the child will see
all of the parent's memory accesses that precede the pthread_create().
Similarly, the pthread_join() system call guarantees that the parent's
accesses following the pthread_join() will see all of the (now terminated)
child's accesses.

							Thanx, Paul

> Thanks,
> --Jason
> 
> Junchang Wang (2):
>   Fix typos in help messages
>   routetorture.h: Switch from ACCESS_ONCE() to READ_ONCE()/WRITE_ONCE()
> 
>  CodeSamples/defer/routetorture.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH 0/2] Patches to chapter Deferred Processing

[Junchang Wang]

Hi Paul,

On Thu, Jun 8, 2017 at 11:38 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Jun 08, 2017 at 11:16:31AM +0800, Junchang Wang wrote:
>> Hi Paul and list,
>>
>> Attached are two patches for routetorture.h. Please take a look.
>
> Good catches, queued and pushed, thank you!
>
>> One of my remaining question is about the two smp_mb() in routetorture.h. Take
>> the smp_mb() in perftest() as example, my understanding is that it is used to
>> prevent the access to nthreads_running and access to goflag from being
>> reorganized. If that's the case, don't we need a paired memory barrier
>> instruction in perftest_reader()? Specifically, in between line 105
>> (atomic_inc(&nthreads_running);) and line 106 (gf = READ_ONCE(goflag);).
>
> This one is unusual.  The ordering controls not correctness, but
> time duration.  Plus the assignment of GOFLAG_RUN to goflag cannot
> happen until after the effect of the atomic_inc() propagates back.
> Interestingly enough, this code does fully not take control dependencies
> into account, which could reduce the number of memory barriers.
>
> In short, this situation is unusual and makes complex reliance on
> implicit ordering guarantees.

Thanks for the reply. You are right, this is a special case in which
the first shared data has been protected by primitive atomic_read(),
such that the smp_mb() is not necessary.

>
> But it might well have bugs.  If you have time, one
> approach would be to read https://lwn.net/Articles/718628/ and
> https://lwn.net/Articles/720550/, download the tool described, and try
> to create the corresponding litmus test.  Either way, please let me know
> your intentions.  This would be a really cool way for you to learn the
> latest and greatest stuff about the Linux kernel memory model!
>

I'm currently very interested in parallel programming, but I find, in
practice, there are too much ``insane'' hardware/compiler/library
details. I'm becoming more and more hesitate to say my code is correct
even if it can run correctly on a specific hardware for 100 times :-(
.  So I would be very happy to read the articles you pointed out and
try the tool.

>> Another puzzle is that we are using shared memory 'pap' to transfer statistic
>> results from working threads, e.g. perftest_reader, to main thread, e.g.
>> perftest. Do we need a smp_mb() at the tail (before instruction return) of
>> function perftest_reader to force the results being written before the thread
>> terminates? In other words, I'm not sure if the two events, writing to shared
>> memory pap and thread termination, could be reordered.  Hints are welcome!
>
> The pthread_create() system call guarantees that the child will see
> all of the parent's memory accesses that precede the pthread_create().
> Similarly, the pthread_join() system call guarantees that the parent's
> accesses following the pthread_join() will see all of the (now terminated)
> child's accesses.
>

Got it. Thanks a lot!

--Jason


>                                                         Thanx, Paul
>
>> Thanks,
>> --Jason
>>
>> Junchang Wang (2):
>>   Fix typos in help messages
>>   routetorture.h: Switch from ACCESS_ONCE() to READ_ONCE()/WRITE_ONCE()
>>
>>  CodeSamples/defer/routetorture.h | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> --
>> 2.7.4
>>
>