On Sat, May 13, 2017 at 8:45 PM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> On Sat, May 13, 2017 at 09:04:38PM +0900, Akira Yokosawa wrote:
>> Hi Jason & Paul,
>>
>> although this has already been applied, I have a comment.
>>
>> On 2017/05/11 23:03:41 +0800, Junchang Wang wrote:
>> > Signed-off-by: Junchang Wang <junchangwang@xxxxxxxxx>
>> > ---
>> > CodeSamples/count/count_stat_eventual.c | 8 ++++----
>> > 1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
>> > index 059ab8b..cbde4aa 100644
>> > --- a/CodeSamples/count/count_stat_eventual.c
>> > +++ b/CodeSamples/count/count_stat_eventual.c
>> > @@ -27,12 +27,12 @@ int stopflag;
>> >
>> > void inc_count(void)
>> > {
>> > - ACCESS_ONCE(__get_thread_var(counter))++;
>> > + READ_ONCE(__get_thread_var(counter))++;
>>
>> This is OK because READ_ONCE() is defined as the same as ACCESS_ONCE()
>> in CodeSamples. However, the definition in the current Linux kernel
>> would not permit this.
>>
>> A read-modify-write access would need both READ_ONCE() and WRITE_ONCE().
>> However, since "counter" is thread local and updated only by its owner,
>> we don't need READ_ONCE() here. So:
>>
>> + WRITE_ONCE(__get_thread_var(counter), __get_thread_var(counter) + 1);
>>
>> should have been sufficient.
>>
>> Problem with this change is that the line gets too wide when applied to
>> the code snippet in 2-column layout.
>
> Good point -- though renumbering the code is not all -that- hard.
>
> I clearly should have made a better READ_ONCE() that enforced the same
> constraints as does the Linux kernel, perhaps something like this:
>
> #define READ_ONCE(x) ({ ACCESS_ONCE(x) })
>
> Thoughts?
Hi Akira and Paul,
When I was working on the code, my idea was that ACCESS_ONCE() brings
the following benefits:
1) Clearly underlines the code programmers need to take care.
2) Prevent compiler from arranging the code.
3) Remove register/reference hassle.
So, functionally READ_ONCE alone is enough. And it makes the code
'easy' to read :-)
But I agree WRITE_ONCE is logically right here. And it would be better
if the definition in CodeSample is idential to the kernel.
Cheers!
--Jason
>
>
>> Hmm...
>>
>> Thanks, Akira
>>
>> > }
>> >
>> > unsigned long read_count(void)
>> > {
>> > - return ACCESS_ONCE(global_count);
>> > + return READ_ONCE(global_count);
>> > }
>> >
>> > void *eventual(void *arg)
>> > @@ -43,8 +43,8 @@ void *eventual(void *arg)
>> > while (stopflag < 3) {
>> > sum = 0;
>> > for_each_thread(t)
>> > - sum += ACCESS_ONCE(per_thread(counter, t));
>> > - ACCESS_ONCE(global_count) = sum;
>> > + sum += READ_ONCE(per_thread(counter, t));
>> > + WRITE_ONCE(global_count, sum);
>> > poll(NULL, 0, 1);
>> > if (stopflag) {
>> > smp_mb();
>> >
>>
>
--
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
