On Tue, May 30, 2017 at 09:05:15PM +0900, Akira Yokosawa wrote:
> >From 489b5e3bdeba2f9b733dbe3d85390368dd159174 Mon Sep 17 00:00:00 2001
> From: Akira Yokosawa <akiyks@xxxxxxxxx>
> 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?
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 <akiyks@xxxxxxxxx>
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 <akiyks@xxxxxxxxx>
[ paulmck: Rework loop to eliminate redundant fetches and comparisons. ]
Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
diff --git a/CodeSamples/defer/gettimestampmp.c b/CodeSamples/defer/gettimestampmp.c
index 2abade42e233..8780b71f33d7 100644
--- a/CodeSamples/defer/gettimestampmp.c
+++ b/CodeSamples/defer/gettimestampmp.c
@@ -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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
