CodeSamples: Cleanups and fixes

Quoted 1 line

From e325a5ff132d8e4a79dfe465b18f573b97da75db Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Sun, 28 May 2017 12:38:27 +0900
Subject: [RFC PATCH 3/4] CodeSamples: Use 'intptr_t' to be compatible with 'void *'

On x86_64, casting between "int" and "void *" in threadcreate.c causes GCC to emit warnings such as:

warning: cast from pointer to integer of different size
warning: cast from integer to pointer of different size

Let's adopt C99 way of writing portable code.

Also do the same changes where appropriate. (Other than threadcreate.c, no warnings were observed on x86_64, but we can remove a few casts to "long".)

Signed-off-by Akira Yokosawa <[email protected]>

---
 CodeSamples/SMPdesign/matmul.c     | 13 +++++++------
 CodeSamples/SMPdesign/smpalloc.c   | 11 ++++++-----
 CodeSamples/defer/gettimestampmp.c |  2 +-
 CodeSamples/intro/threadcreate.c   |  7 ++++---
 4 files changed, 18 insertions(+), 15 deletions(-)

CodeSamples/SMPdesign/matmul.c +0 -0

index 6a07730..9f0b250 100644
@@ -18,12 +18,13 @@
  * Copyright (c) 2009 Paul E. McKenney, IBM Corporation.

CodeSamples/SMPdesign/matmul.c +3 -3

 	atomic_inc(&nstarted);
 	while (goflag == GOFLAG_INIT)

@@ -58,7 +59,7 @@ void *matmul_thread(void *me_in)
 
 int main(int argc, char *argv[])
 {
-	int i, j;
+	intptr_t i, j;
 	long long startcreatetime;
 	long long starttime;
 	long long endtime;

@@ -87,7 +88,7 @@ int main(int argc, char *argv[])
 	goflag = GOFLAG_INIT;
 	startcreatetime = get_microseconds();
 	for (i = 0; i < nthread; i++)
-		create_thread(matmul_thread, (void *)(long)i);
+		create_thread(matmul_thread, (void *)i);
 	while (atomic_read(&nstarted) != nthread)
 		barrier();
 	starttime = get_microseconds();

@@ -95,7 +96,7 @@ int main(int argc, char *argv[])
 	while (atomic_read(&ndone) != nthread)
 		poll(NULL, 0, 1);
 	endtime = get_microseconds();
-	printf("dim = %ld, nthread = %d, duration = %lld : %lld us\n",
+	printf("dim = %" PRIdPTR ", nthread = %d, duration = %lld : %lld us\n",
 	       dim, nthread, endtime - startcreatetime, endtime - starttime);
 	return 0;
 }

CodeSamples/SMPdesign/smpalloc.c +6 -5

index 95b21a1..0882f63 100644
@@ -19,6 +19,7 @@
  * Copyright (c) 2006 Paul E. McKenney, IBM Corporation.
  */
 
+#include <inttypes.h>
 #include "../api.h"
 
 #define TARGET_POOL_SIZE 3

@@ -123,7 +124,7 @@ void *memblock_test(void *arg)
 	long cnt = 0;
 	long cntfail = 0;
 	int i;
-	int runlength = (int)(long)arg;
+	intptr_t runlength = (intptr_t)arg;
 	struct memblock *p[MAX_RUN];
 
 	if (runlength > MAX_RUN)

@@ -159,7 +160,7 @@ int main(int argc, char *argv[])
 	long long nc;
 	long long nf;
 	int nkids = 1;
-	int runlength = 1;
+	intptr_t runlength = 1;
 	int totbefore;
 
 	smp_init();

@@ -176,12 +177,12 @@ int main(int argc, char *argv[])
 	if (argc > 2) {
 		runlength = strtoul(argv[2], NULL, 0);
 		if (runlength > MAX_RUN) {
-			fprintf(stderr, "nkids = %d too large, max = %d\n",
+			fprintf(stderr, "nkids = %" PRIdPTR " too large, max = %d\n",
 				runlength, MAX_RUN);
 			usage(argv[0]);
 		}
 	}
-	printf("%d %d ", nkids, runlength);
+	printf("%d %" PRIdPTR, nkids, runlength);
 
 	init_per_thread(results, 0L);
 	init_per_thread(failures, 0L);

@@ -189,7 +190,7 @@ int main(int argc, char *argv[])
 
 	goflag = 1;
 	for (i = 0; i < nkids; i++)
-		create_thread(memblock_test, (void *)(long)runlength);
+		create_thread(memblock_test, (void *)runlength);
 
 	sleep(1);
 	goflag = 0;

CodeSamples/defer/gettimestampmp.c +1 -1

index bc0b9ea..e794e82 100644
@@ -29,7 +29,7 @@ long curtimestamp = 0;
 
 void *collect_timestamps(void *mask_in)
 {
-	long mask = (long)mask_in;
+	intptr_t mask = (intptr_t)mask_in;
 
 	while (curtimestamp < MAX_TIMESTAMPS) {
 		while ((curtimestamp & CURTIMESTAMP_MASK) != mask)

CodeSamples/intro/threadcreate.c +4 -3

index 26b7ba9..470bb11 100644
@@ -18,13 +18,14 @@
  * Copyright (c) 2006 Paul E. McKenney, IBM Corporation.
  */
 
+#include <inttypes.h>
 #include "../api.h"
 
 void *thread_test(void *arg)
 {
-	int myarg = (int)arg;
+	intptr_t myarg = (intptr_t)arg;
 
-	printf("child thread %d: smp_thread_id() = %d\n",
+	printf("child thread %" PRIdPTR ": smp_thread_id() = %d\n",
 	       myarg, smp_thread_id());
 	return NULL;
 }

@@ -38,7 +39,7 @@ void usage(char *progname)
 
 int main(int argc, char *argv[])
 {
-	int i;
+	intptr_t i;
 	int nkids = 1;
 
 	smp_init();

Signature · 7 lines

2.7.4


--
To unsubscribe from this list: send the line "unsubscribe perfbook" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line

From fd5fb9afb6b99b15370a9823b42fa2a92342b92a Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Sun, 28 May 2017 15:42:45 +0900
Subject: [RFC PATCH 4/4] CodeSamples/defer: Add compiler barriers in gettimestampmp.c

They ensure curtimestamp is read and written once in every iteration.

Signed-off-by Akira Yokosawa <[email protected]>

---
 CodeSamples/defer/gettimestampmp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

CodeSamples/defer/gettimestampmp.c +2 -1

index e794e82..7012caa 100644
@@ -32,7 +32,7 @@ void *collect_timestamps(void *mask_in)
 	intptr_t mask = (intptr_t)mask_in;
 
 	while (curtimestamp < MAX_TIMESTAMPS) {
-		while ((curtimestamp & CURTIMESTAMP_MASK) != mask)
+		while ((READ_ONCE(curtimestamp) & CURTIMESTAMP_MASK) != mask)
 			continue;
 		if (curtimestamp >= MAX_TIMESTAMPS)
 			break;

@@ -40,6 +40,7 @@ void *collect_timestamps(void *mask_in)
 		/* Don't need memory barrier -- no other shared vars!!! */
 
 		ts[curtimestamp++] = get_timestamp();
+		barrier();
 	}
 	smp_mb();
 	return (NULL);

Signature · 1 line

2.7.4

Quoted 1 line

From fd5fb9afb6b99b15370a9823b42fa2a92342b92a Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 06:52:40 +0900
Subject: [RFC PATCH 0/4] CodeSamples: Cleanups and fixes

Hi Paul,

While I was looking around CodeSamples, I encountered several warnings in build and an infinite loop in one of sample codes. Also, I found that CodeSamples/api.h and CodeSamples/Makefile.arch can be removed from repository if their recipe are properly written in Makefiles.

Patch 1 adds rules to generate Makefile.arch and api.h that suit the architecture of you host. As I don't have ppc64 or arm host environment, I tested it only on x86_32 and x86_64. I'd like to know if it works on ppc64 and arm.

Patch 2 removes the redundant files.

Patch 3 and 4 are fixes of warnings and infinite loops.

Thoughts?

Thanks, Akira

Signature · 35 lines

Akira Yokosawa (4):
  CodeSamples: Add rule to generate Makefile.arch and api.h
  CodeSamples: Remove generated files from repository
  CodeSamples: Use 'intptr_t' to be compatible with 'void *'
  CodeSamples/defer: Add compiler barriers in gettimestampmp.c

 CodeSamples/.gitignore                         |   4 +-
 CodeSamples/Makefile                           |  18 +
 CodeSamples/Makefile.arch                      |   6 -
 CodeSamples/SMPdesign/Makefile                 |   3 +
 CodeSamples/SMPdesign/matmul.c                 |  13 +-
 CodeSamples/SMPdesign/smpalloc.c               |  11 +-
 CodeSamples/advsync/Makefile                   |   3 +
 CodeSamples/api.h                              | 808 -------------------------
 CodeSamples/count/Makefile                     |   3 +
 CodeSamples/datastruct/Issaquah/Makefile       |   3 +
 CodeSamples/datastruct/Issaquah/existence_test | Bin 26359 -> 0 bytes
 CodeSamples/datastruct/hash/Makefile           |   3 +
 CodeSamples/datastruct/log/Makefile            |   3 +
 CodeSamples/datastruct/skiplist/Makefile       |   3 +
 CodeSamples/defer/Makefile                     |   3 +
 CodeSamples/defer/gettimestampmp.c             |   5 +-
 CodeSamples/depends.mk                         |  43 ++
 CodeSamples/intro/Makefile                     |   3 +
 CodeSamples/intro/threadcreate.c               |   7 +-
 CodeSamples/locking/Makefile                   |   3 +
 CodeSamples/toolsoftrade/Makefile              |   3 +
 21 files changed, 117 insertions(+), 831 deletions(-)
 delete mode 100644 CodeSamples/Makefile.arch
 delete mode 100644 CodeSamples/api.h
 delete mode 100755 CodeSamples/datastruct/Issaquah/existence_test
 create mode 100644 CodeSamples/depends.mk

-- 
2.7.4

Quoted 18 lines

Quoted 1 line

From fd5fb9afb6b99b15370a9823b42fa2a92342b92a Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 06:52:40 +0900
Subject: [RFC PATCH 0/4] CodeSamples: Cleanups and fixes

Hi Paul,

While I was looking around CodeSamples, I encountered several warnings in build and an infinite loop in one of sample codes. Also, I found that CodeSamples/api.h and CodeSamples/Makefile.arch can be removed from repository if their recipe are properly written in Makefiles.

Patch 1 adds rules to generate Makefile.arch and api.h that suit the architecture of you host. As I don't have ppc64 or arm host environment, I tested it only on x86_32 and x86_64. I'd like to know if it works on ppc64 and arm.

Quoted 1 line

Patch 2 removes the redundant files.

I took the first two, and it is nice to just be able to type "make" in the CodeSamples directory! ;-)

Quoted 2 lines

Patch 3 and 4 are fixes of warnings and infinite loops.

I have some concerns here, and will reply to the patches.

Thanx, Paul

Quoted 40 lines

Thoughts?

Thanks, Akira

Signature · 35 lines

Akira Yokosawa (4):
 CodeSamples: Add rule to generate Makefile.arch and api.h
 CodeSamples: Remove generated files from repository
 CodeSamples: Use 'intptr_t' to be compatible with 'void *'
 CodeSamples/defer: Add compiler barriers in gettimestampmp.c

CodeSamples/.gitignore                         |   4 +-
CodeSamples/Makefile                           |  18 +
CodeSamples/Makefile.arch                      |   6 -
CodeSamples/SMPdesign/Makefile                 |   3 +
CodeSamples/SMPdesign/matmul.c                 |  13 +-
CodeSamples/SMPdesign/smpalloc.c               |  11 +-
CodeSamples/advsync/Makefile                   |   3 +
CodeSamples/api.h                              | 808 -------------------------
CodeSamples/count/Makefile                     |   3 +
CodeSamples/datastruct/Issaquah/Makefile       |   3 +
CodeSamples/datastruct/Issaquah/existence_test | Bin 26359 -> 0 bytes
CodeSamples/datastruct/hash/Makefile           |   3 +
CodeSamples/datastruct/log/Makefile            |   3 +
CodeSamples/datastruct/skiplist/Makefile       |   3 +
CodeSamples/defer/Makefile |   3 +
CodeSamples/defer/gettimestampmp.c             |   5 +-
CodeSamples/depends.mk                         |  43 ++
CodeSamples/intro/Makefile                     |   3 +
CodeSamples/intro/threadcreate.c               |   7 +-
CodeSamples/locking/Makefile                   |   3 +
CodeSamples/toolsoftrade/Makefile              |   3 +
21 files changed, 117 insertions(+), 831 deletions(-)
delete mode 100644 CodeSamples/Makefile.arch
delete mode 100644 CodeSamples/api.h
delete mode 100755 CodeSamples/datastruct/Issaquah/existence_test
create mode 100644 CodeSamples/depends.mk

-- 
2.7.4

Quoted 1 line

From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 20:44:52 +0900
Subject: [RFC PATCH v2 0/2] CodeSamples: Cleanups and fixes

Hi Paul,

This is the respin of the latter two patches of v1. I'm keeping RFC because of some questions.

"long" -> "intptr_t" changes in Patch 1 have no effect on a platform where "long" and "intptr_t" have the same width, but I think they are good in portability POV.

WRITE_ONCE() in Patch 2 is placed under the assignment to the array because I could not translate post increment in any other way. Does the WRITE_ONCE() ensure the outer "while" capture the value?

Quoted 14 lines · Akira Yokosawa

Quoted 1 line

From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 20:44:52 +0900
Subject: [RFC PATCH v2 0/2] CodeSamples: Cleanups and fixes

Hi Paul,

This is the respin of the latter two patches of v1. I'm keeping RFC because of some questions.

"long" -> "intptr_t" changes in Patch 1 have no effect on a platform where "long" and "intptr_t" have the same width, but I think they are good in portability POV.

Quoted 3 lines

WRITE_ONCE() in Patch 2 is placed under the assignment to the array because I could not translate post increment in any other way. Does the WRITE_ONCE() ensure the outer "while" capture the value?

Wow, that loop is old code!!! My current compiler creates an infinite loop for it, so yes, there is more required.

Plus there are confusing
and redundant comparisons, so that it is not entirely clear to me that the loop is guaranteed to terminate properly

So I took both patches, but rewrote the loop in the second patch as shown below.

If you are OK with this rewrite, I will push them.

Thanx, Paul

------------------------------------------------------------------------

commit 8a54d9aeeeefa1909db062dc893705ff8fefd702

Author: Akira Yokosawa <[email protected]>
Date:   Tue May 30 20:40:04 2017 +0900

CodeSamples/defer: Rework loop in gettimestampmp.c

Add READ_ONCE() and WRITE_ONCE() ensure curtimestamp is read and written once in every iteration. The READ_ONCE() is not optional, as modern compilers can (and do) emit an infinite loop for the earlier code.

Signed-off-by Akira Yokosawa <[email protected]>

[ paulmck: Rework loop to eliminate redundant fetches and comparisons. ]

Signed-off-by Paul E. McKenney <[email protected]>

CodeSamples/defer/gettimestampmp.c +8 -5

index 2abade42e233..8780b71f33d7 100644
@@ -30,16 +30,19 @@ long curtimestamp = 0;
 void *collect_timestamps(void *mask_in)
 {
 	long mask = (intptr_t)mask_in;
+	long cts;
 
-	while (curtimestamp < MAX_TIMESTAMPS) {
-		while ((curtimestamp & CURTIMESTAMP_MASK) != mask)
-			continue;
-		if (curtimestamp >= MAX_TIMESTAMPS)
+	for (;;) {
+		cts = READ_ONCE(curtimestamp);
+		if (cts >= MAX_TIMESTAMPS)
 			break;
+		if ((cts & CURTIMESTAMP_MASK) != mask)
+			continue;
 
 		/* Don't need memory barrier -- no other shared vars!!! */
 
-		ts[curtimestamp++] = get_timestamp();
+		ts[cts] = get_timestamp();
+		WRITE_ONCE(curtimestamp, cts + 1);
 	}
 	smp_mb();
 	return (NULL);

Quoted 15 lines · Paul E. McKenney

Quoted 14 lines · Akira Yokosawa

Quoted 1 line

From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 20:44:52 +0900
Subject: [RFC PATCH v2 0/2] CodeSamples: Cleanups and fixes

Hi Paul,

This is the respin of the latter two patches of v1. I'm keeping RFC because of some questions.

"long" -> "intptr_t" changes in Patch 1 have no effect on a platform where "long" and "intptr_t" have the same width, but I think they are good in portability POV.

Quoted 6 lines

Quoted 3 lines

WRITE_ONCE() in Patch 2 is placed under the assignment to the array because I could not translate post increment in any other way. Does the WRITE_ONCE() ensure the outer "while" capture the value?

Wow, that loop is old code!!! My current compiler creates an infinite loop for it, so yes, there is more required.

You mean on GCC for ppc64?

Quoted 7 lines

Plus there are confusing
and redundant comparisons, so that it is not entirely clear to me that the loop is guaranteed to terminate properly

So I took both patches, but rewrote the loop in the second patch as shown below.

Quoted 1 line

If you are OK with this rewrite, I will push them.

I'm OK with this, but it is a whole rewrite of the code, so

Reported-by Akira Yokosawa <[email protected]>

looks appropriate in this case.
It's up to you which tag to use.

Thanks, Akira

Quoted 50 lines

Thanx, Paul

------------------------------------------------------------------------

commit 8a54d9aeeeefa1909db062dc893705ff8fefd702

Author: Akira Yokosawa <[email protected]>
Date:   Tue May 30 20:40:04 2017 +0900

CodeSamples/defer: Rework loop in gettimestampmp.c

Add READ_ONCE() and WRITE_ONCE() ensure curtimestamp is read and written once in every iteration. The READ_ONCE() is not optional, as modern compilers can (and do) emit an infinite loop for the earlier code.

Signed-off-by Akira Yokosawa <[email protected]>

[ paulmck: Rework loop to eliminate redundant fetches and comparisons. ]

Signed-off-by Paul E. McKenney <[email protected]>

CodeSamples/defer/gettimestampmp.c +8 -5

index 2abade42e233..8780b71f33d7 100644
@@ -30,16 +30,19 @@ long curtimestamp = 0;
 void *collect_timestamps(void *mask_in)
 {
 	long mask = (intptr_t)mask_in;
+	long cts;
 
-	while (curtimestamp < MAX_TIMESTAMPS) {
-		while ((curtimestamp & CURTIMESTAMP_MASK) != mask)
-			continue;
-		if (curtimestamp >= MAX_TIMESTAMPS)
+	for (;;) {
+		cts = READ_ONCE(curtimestamp);
+		if (cts >= MAX_TIMESTAMPS)
 			break;
+		if ((cts & CURTIMESTAMP_MASK) != mask)
+			continue;
 
 		/* Don't need memory barrier -- no other shared vars!!! */
 
-		ts[curtimestamp++] = get_timestamp();
+		ts[cts] = get_timestamp();
+		WRITE_ONCE(curtimestamp, cts + 1);
 	}
 	smp_mb();
 	return (NULL);

Quoted 10 lines · Akira Yokosawa

Quoted 1 line

From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 20:44:52 +0900
Subject: [RFC PATCH v2 0/2] CodeSamples: Cleanups and fixes

Hi Paul,

This is the respin of the latter two patches of v1. I'm keeping RFC because of some questions.

Quoted 7 lines

"long" -> "intptr_t" changes in Patch 1 have no effect on a platform where "long" and "intptr_t" have the same width, but I think they are good in portability POV.

WRITE_ONCE() in Patch 2 is placed under the assignment to the array because I could not translate post increment in any other way. Does the WRITE_ONCE() ensure the outer "while" capture the value?

Wow, that loop is old code!!! My current compiler creates an infinite
loop for it, so yes, there is more required. Plus there are confusing
and redundant comparisons, so that it is not entirely clear to me that the loop is guaranteed to terminate properly.

So I took both patches, but rewrote the loop in the second patch as shown below.

If you are OK with this rewrite, I will push them.

Thanx, Paul

------------------------------------------------------------------------

commit 8a54d9aeeeefa1909db062dc893705ff8fefd702

Author: Akira Yokosawa <[email protected]>
Date:   Tue May 30 20:40:04 2017 +0900

CodeSamples/defer: Rework loop in gettimestampmp.c

Add READ_ONCE() and WRITE_ONCE() ensure curtimestamp is read and written once in every iteration. The READ_ONCE() is not optional, as modern compilers can (and do) emit an infinite loop for the earlier code.

Signed-off-by Akira Yokosawa <[email protected]>

[ paulmck: Rework loop to eliminate redundant fetches and comparisons. ]

Signed-off-by Paul E. McKenney <[email protected]>

CodeSamples/defer/gettimestampmp.c +8 -6

index 2abade42e233..8780b71f33d7 100644
@@ -30,16 +30,19 @@ long curtimestamp = 0;
 void *collect_timestamps(void *mask_in)
 {
        long mask = (intptr_t)mask_in;
+       long cts;
 
-       while (curtimestamp < MAX_TIMESTAMPS) {
-               while ((curtimestamp & CURTIMESTAMP_MASK) != mask)
-                       continue;
-               if (curtimestamp >= MAX_TIMESTAMPS)
+       for (;;) {
+               cts = READ_ONCE(curtimestamp);
+               if (cts >= MAX_TIMESTAMPS)
                        break;
+               if ((cts & CURTIMESTAMP_MASK) != mask)
+                       continue;
 
                /* Don't need memory barrier -- no other shared vars!!! */
 
-               ts[curtimestamp++] = get_timestamp();
+               ts[cts] = get_timestamp();
+               WRITE_ONCE(curtimestamp, cts + 1);
        }
        smp_mb();
        return (NULL);
 
--

To unsubscribe from this list: send the line "unsubscribe perfbook" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Paul and Akira,

The code looks clear and easy to understand now :-).

While checking the patch, I have one question. Are there any technical reason that we prefer for(;;) instead of while(1) in CodeSamples? Just out of curiosity :-)

Thanks

Quoted 81 lines · Paul E. McKenney

Quoted 10 lines · Akira Yokosawa

Quoted 1 line

From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 20:44:52 +0900
Subject: [RFC PATCH v2 0/2] CodeSamples: Cleanups and fixes

Hi Paul,

This is the respin of the latter two patches of v1. I'm keeping RFC because of some questions.

Quoted 7 lines

"long" -> "intptr_t" changes in Patch 1 have no effect on a platform where "long" and "intptr_t" have the same width, but I think they are good in portability POV.

WRITE_ONCE() in Patch 2 is placed under the assignment to the array because I could not translate post increment in any other way. Does the WRITE_ONCE() ensure the outer "while" capture the value?

Wow, that loop is old code!!! My current compiler creates an infinite
loop for it, so yes, there is more required. Plus there are confusing
and redundant comparisons, so that it is not entirely clear to me that the loop is guaranteed to terminate properly.

So I took both patches, but rewrote the loop in the second patch as shown below.

If you are OK with this rewrite, I will push them.

Thanx, Paul

------------------------------------------------------------------------

commit 8a54d9aeeeefa1909db062dc893705ff8fefd702

Author: Akira Yokosawa <[email protected]>
Date:   Tue May 30 20:40:04 2017 +0900

CodeSamples/defer: Rework loop in gettimestampmp.c

Add READ_ONCE() and WRITE_ONCE() ensure curtimestamp is read and written once in every iteration. The READ_ONCE() is not optional, as modern compilers can (and do) emit an infinite loop for the earlier code.

Signed-off-by Akira Yokosawa <[email protected]>

[ paulmck: Rework loop to eliminate redundant fetches and comparisons. ]

Signed-off-by Paul E. McKenney <[email protected]>

CodeSamples/defer/gettimestampmp.c +8 -6

index 2abade42e233..8780b71f33d7 100644
@@ -30,16 +30,19 @@ long curtimestamp = 0;
 void *collect_timestamps(void *mask_in)
 {
        long mask = (intptr_t)mask_in;
+       long cts;
 
-       while (curtimestamp < MAX_TIMESTAMPS) {
-               while ((curtimestamp & CURTIMESTAMP_MASK) != mask)
-                       continue;
-               if (curtimestamp >= MAX_TIMESTAMPS)
+       for (;;) {
+               cts = READ_ONCE(curtimestamp);
+               if (cts >= MAX_TIMESTAMPS)
                        break;
+               if ((cts & CURTIMESTAMP_MASK) != mask)
+                       continue;
 
                /* Don't need memory barrier -- no other shared vars!!! */
 
-               ts[curtimestamp++] = get_timestamp();
+               ts[cts] = get_timestamp();
+               WRITE_ONCE(curtimestamp, cts + 1);
        }
        smp_mb();
        return (NULL);
 
--

To unsubscribe from this list: send the line "unsubscribe perfbook" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Junchang Wang

Hi Paul and Akira,

The code looks clear and easy to understand now :-).

Glad you like it! I am sure that other code in there could use similar help.

Quoted 4 lines

While checking the patch, I have one question. Are there any technical reason that we prefer for(;;) instead of while(1) in CodeSamples? Just out of curiosity :-)

No. Just thirty-five years of habit plus the preferences of the Linux kernel community. And the latter are probably because "while (1)" has one more character than does "for (;;)". ;-)

Thanx, Paul

Quoted 86 lines

Thanks

Quoted 81 lines · Paul E. McKenney

Quoted 10 lines · Akira Yokosawa

Quoted 1 line

From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001

From: Akira Yokosawa <[email protected]>
Date: Tue, 30 May 2017 20:44:52 +0900
Subject: [RFC PATCH v2 0/2] CodeSamples: Cleanups and fixes

Hi Paul,

This is the respin of the latter two patches of v1. I'm keeping RFC because of some questions.

Quoted 7 lines

"long" -> "intptr_t" changes in Patch 1 have no effect on a platform where "long" and "intptr_t" have the same width, but I think they are good in portability POV.

WRITE_ONCE() in Patch 2 is placed under the assignment to the array because I could not translate post increment in any other way. Does the WRITE_ONCE() ensure the outer "while" capture the value?

Wow, that loop is old code!!! My current compiler creates an infinite
loop for it, so yes, there is more required. Plus there are confusing
and redundant comparisons, so that it is not entirely clear to me that the loop is guaranteed to terminate properly.

So I took both patches, but rewrote the loop in the second patch as shown below.

If you are OK with this rewrite, I will push them.

Thanx, Paul

------------------------------------------------------------------------

commit 8a54d9aeeeefa1909db062dc893705ff8fefd702

Author: Akira Yokosawa <[email protected]>
Date:   Tue May 30 20:40:04 2017 +0900

CodeSamples/defer: Rework loop in gettimestampmp.c

Add READ_ONCE() and WRITE_ONCE() ensure curtimestamp is read and written once in every iteration. The READ_ONCE() is not optional, as modern compilers can (and do) emit an infinite loop for the earlier code.

Signed-off-by Akira Yokosawa <[email protected]>

[ paulmck: Rework loop to eliminate redundant fetches and comparisons. ]

Signed-off-by Paul E. McKenney <[email protected]>

CodeSamples/defer/gettimestampmp.c +8 -6

index 2abade42e233..8780b71f33d7 100644
@@ -30,16 +30,19 @@ long curtimestamp = 0;
 void *collect_timestamps(void *mask_in)
 {
        long mask = (intptr_t)mask_in;
+       long cts;
 
-       while (curtimestamp < MAX_TIMESTAMPS) {
-               while ((curtimestamp & CURTIMESTAMP_MASK) != mask)
-                       continue;
-               if (curtimestamp >= MAX_TIMESTAMPS)
+       for (;;) {
+               cts = READ_ONCE(curtimestamp);
+               if (cts >= MAX_TIMESTAMPS)
                        break;
+               if ((cts & CURTIMESTAMP_MASK) != mask)
+                       continue;
 
                /* Don't need memory barrier -- no other shared vars!!! */
 
-               ts[curtimestamp++] = get_timestamp();
+               ts[cts] = get_timestamp();
+               WRITE_ONCE(curtimestamp, cts + 1);
        }
        smp_mb();
        return (NULL);
 
--

To unsubscribe from this list: send the line "unsubscribe perfbook" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html