On 2017/05/31 11:46:55 -0700, Paul E. McKenney wrote:
> 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.
You mean on GCC for ppc64?
> 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.
I'm OK with this, but it is a whole rewrite of the code, so
Reported-by: Akira Yokosawa <akiyks@xxxxxxxxx>
looks appropriate in this case.
It's up to you which tag to use.
Thanks, Akira
>
> 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
