On 2017/05/14 9:58, Akira Yokosawa wrote:
> On 2017/05/14 7:56, Paul E. McKenney wrote:
>> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
>>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney 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?
>>>
>>> I assume you meant:
>>>
>>> #define READ_ONCE(x) ({ ACCESS_ONCE(x); })
>>
>> Indeed! Good catch!!!
>>
>>> I'm afraid this still permits uses such as:
>>>
>>> READ_ONCE(y)++;
>>>
>>> Looks like we need a complex definition which resembles that of
>>> include/linux/compiler.h. Hmm???
>>
>> OK, how about this?
>>
>> #define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
>
> Ah, this resulted in an error as expected!
>
> main.c: In function ‘inc_test’:
> main.c:11:18: error: lvalue required as increment operand
> READ_ONCE(*p)++;
>
But the definition above will conflict with the argument "___x":
y = READ_ONCE(___x);
This won't work as expected.
For CodeSamples, I guess we can safely reserve the name "___x".
Thoughts?
Thanks, Akira
> Glad to know we can avoid the complexity of the kernel.
>
> Thanks, Akira
>
>>
>>> And I have another pending question regarding 2/2 of this patch set.
>>> That might result in other addition of line to the code. I think
>>> I can send it tomorrow.
>>
>> Looking forward to seeing it! ;-)
>>
>> Thanx, Paul
>>
>>>>> 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
