Patches to chapter Deferred Processing

PATCH In review Junchang Wang 2 participants 5 messages Jun 8, 2017

lore

Share & tools

lore

Junchang Wang

author

2017-06-08 03:16 UTC #0 lore raw

cover Patches to chapter Deferred Processing

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(-)

Signature · 1 line

2.7.4

Junchang Wang

author

2017-06-08 03:16 UTC #1 lore raw

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

Signed-off-by Junchang Wang <[email protected]>

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

CodeSamples/defer/routetorture.h +2 -2

index fa4fdab..86693e9 100644
@@ -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");

Signature · 1 line

2.7.4

Junchang Wang

author

2017-06-08 03:16 UTC #2 lore raw

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

In reply to Junchang Wang

Signed-off-by Junchang Wang <[email protected]>

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

CodeSamples/defer/routetorture.h +8 -8

index 86693e9..b0d642a 100644
@@ -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();

Signature · 1 line

2.7.4

Paul E. McKenney

2017-06-08 03:38 UTC #3 lore raw

In reply to Junchang Wang

Quoted 3 lines · Junchang Wang

Hi Paul and list,
Attached are two patches for routetorture.h. Please take a look.

Good catches, queued and pushed, thank you!

Quoted 6 lines

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!

Quoted 6 lines

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

Quoted 13 lines

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(-)
Signature · 1 line
2.7.4

Junchang Wang

author

2017-06-08 04:30 UTC #4 lore raw

Hi Paul,

Quoted 14 lines · Paul E. McKenney

Quoted 3 lines · Junchang Wang
Hi Paul and list,
Attached are two patches for routetorture.h. Please take a look.
Good catches, queued and pushed, thank you!
Quoted 6 lines
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);).

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

Quoted 6 lines
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!

Quoted 6 lines

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

Quoted 16 lines

Thanx, Paul
Quoted 13 lines
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