On 2017/05/14 21:28:56 +0800, Junchang Wang wrote:
> Hi Akira,
>
> Good comments! Please see below:
>
> On Sun, May 14, 2017 at 6:57 PM, Akira Yokosawa <akiyks@xxxxxxxxx> wrote:
>> Hi Jason & Paul,
>>
>> So, here is a comment regarding 2/2 of this patch set.
>> I'm aware that this patch only touches slow path, but anyway...
>>
>> On 2017/05/11 23:03:42 +0800, Junchang Wang wrote:
>>> Signed-off-by: Junchang Wang <junchangwang@xxxxxxxxx>
>>> ---
>>> CodeSamples/count/count_stat_eventual.c | 11 ++++++-----
>>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
>>> index cbde4aa..2caebfd 100644
>>> --- a/CodeSamples/count/count_stat_eventual.c
>>> +++ b/CodeSamples/count/count_stat_eventual.c
>>> @@ -40,15 +40,15 @@ void *eventual(void *arg)
>>> int t;
>>> unsigned long sum;
>>>
>>> - while (stopflag < 3) {
>>> + while (READ_ONCE(stopflag) < 3) {
>>> sum = 0;
>>> for_each_thread(t)
>>> sum += READ_ONCE(per_thread(counter, t));
>>> WRITE_ONCE(global_count, sum);
>>> poll(NULL, 0, 1);
>>> - if (stopflag) {
>>> + if (READ_ONCE(stopflag)) {
>>> smp_mb();
>>> - stopflag++;
>>> + WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
>>
>> If I'm reading this correctly, the smp_mb() is to ensure the ordering of write
>> to global_count and write to stopflag.
>
> Besides that, smp_mb() is to ensure the correctness of WAR of
> stopflag, because stopflag works as a shared variable to synchronize
> two threads. I know it's not necessary for x86/gcc, but as Paul has
> pointed out, other architectures/Compilers may need it :-).
Well, Section 14.2.9 says:
3. A series of stores to a single variable will appear to all CPUs
to have occurred in a single order, though this order might not
be predictable from the code, and in fact the order might vary
from one run to another.
This means that for the value of stopflag, we don't need any memory barrier.
>
>> This "if" branch is taken after
>> count_cleanup()'s update of stopflag is observed. After the update,
>> only the "eventual()" thread updates stopflag. So, the READ_ONCE() within
>> the WRITE_ONCE() is not necessary.
>> To make the locality obvious, it might be better to hold the value in an auto
>> variable.
>> Here is my version of the entire eventual() function. Note that read of stopflag
>> is not necessary at the beginning of the while loop.
>>
>> void *eventual(void *arg)
>> {
>> int t;
>> unsigned long sum;
>> int stopflag_l = 0;
>>
>> while (stopflag_l < 3) {
>> sum = 0;
>> for_each_thread(t)
>> sum += READ_ONCE(per_thread(counter, t));
>> WRITE_ONCE(global_count, sum);
>> poll(NULL, 0, 1);
>> if ((stopflag_l = READ_ONCE(stopflag)) != 0) {
>> smp_mb();
>> WRITE_ONCE(stopflag, ++stopflag_l);
>> }
>> }
>> return NULL;
>> }
>>
>
> Personally, I don't like to add too many implications in the code such
> as excessive optimizations unless we have a strong reason (performance
> gain). Excessive optimizations may confuse compilers and other
> programmers. But again, I agree your code might be slightly better in
> performance :-)
It might be easier to see the purpose of the memory barrier in my version.
Performance wise, eventual() does not matter much.
>
>>> }
>>> }
>>> return NULL;
>>> @@ -66,8 +66,9 @@ void count_init(void)
>>>
>>> void count_cleanup(void)
>>> {
>>> - stopflag = 1;
>>> - while (stopflag < 3)
>>> + WRITE_ONCE(stopflag, 1);
>>> + smp_mb();
>>
>> Is this extra smp_mb() really necessary?
>> We'll loop until stopflag becomes 3 anyway, we can rely on memory coherency
>> for the updated value to propagate.
>>
>
> It's necessary to protect RAW of stopflag on weak consistency machines :-).
Ditto.
>
> Cheers!
> --Jason
>
>>> + while (READ_ONCE(stopflag) < 3)
>>> poll(NULL, 0, 1);
>>> smp_mb();
>>
>> This smp_mb() pairs with the smp_mb() in eventual() and ensures the value of
>> global_count to be read in read_count() is the one written preceding the
>> update of stopflag.
This barrier is necessary because we read both global_count and stopflag.
Have I made my point clear enough?
Thanks, Akira
>>
>> Am I reading right?
>>
>> Thanks, Akira
>>
>>> }
>>>
>
--
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
