[vlc-devel] [PATCH 1/6] os2: implement vlc_once()

[KO Myung-Hun]

Hi/2.

Hugo Beauzée-Luyssen wrote:
> Hi,
> 
> On Wed, Apr 4, 2018, at 4:08 PM, KO Myung-Hun wrote:
>>
>>
>> Rémi Denis-Courmont wrote:
>>> Le mardi 3 avril 2018, 15:48:23 EEST Kamil Rytarowski a écrit :
>>>> On 03.04.2018 13:52, Rémi Denis-Courmont wrote:
>>>>> I gave you the benefits of doubt, but now you are obviously trolling.
>>>>
>>>> So given that the only argument rejecting the patch is ad personam
>>>
>>> No, the argument for rejecting the original patch and the NetBSD-derived one 
>>> is that they are invalid as demonstrated in my previous post - but it should 
>>> be obvious to anybody who understands _and_ acknowledges either the ISO C11 or 
>>> POSIX.2008 threading/memory rules.
>>>
>>
>> Do you mean that sharing non-atomic variable among threads without any
>> memory order or locking is the violation of memory model ?
>>
> 
> The short version is that the memory model garanties you that if you access the same variable from different threads with proper synchronisation, neither your compiler nor your CPU will reorder instructions.
> You could also phrase this as "any un-synchronized access to the same variable from multiple threads will violate the memory model/yield undefined behavior"
> 
>> In general, the violation of memory model may lead to undefined
>> behavior, but this is not the case.
>>
> 
> While it is likely to be ok in most cases, you cannot make that assumption, since it is an undefined behavior. It might change for next compiler version, on a different CPU, and is by definition undefined.

The result of reading once->done will be 0 or 1. If it is 0, no problem
because mutex lock will act as memory barrier and once->done is
double-checked. And if it is 1, it's guaranteed that cb() has been
completed. Because function call acts as memory barrier and it will not
be reordered. Therefore, no problem at all.

> 
>> BTW, you did reject to review both the second patch and the third patch
>> using the atomic operations. Why ? They do not violate the memory model.
>>
> 
> Third patch is using a sleep instead of a mutex, which feels wrong IMHO.

I agree, but I submitted third patch because Rémi said that second patch
was "Seems insanely complicated". If he reviewed it concretely, third
patch using a sleep would not be submitted.

> As for patch 2, the usage of __atomic_cmpxchg32 doesn't seem to match the usage I'd expect from a usual compare-exchange function, which makes it hard to follow.
> 

__atomic_cmpxchg32( uint32_t *obj, uint32_t desired, uint32_t expected )
returns 1 with updating *obj to desired iff *obj == expected, otherwise 0.

In fact, this was the question needed at the beginning instead of
rejecting review.

Anyway do you want to use C11 atomics instead ?

> Regarding NetBSD's variant, I have to agree with Rémi, pto_done is read without any synchronization, which can't be correct (even though there's no observable problem, doesn't mean there's no theoretical problem)
> 
> Something along the line of
> 
> vlc_once
> {
>   int expected = 2;
>   if ( atomic_exchange( &done, &expected, 1 ) == false )
>   {
>       mutex_lock;
>       expected = 1;
>       if ( atomic_exchange( &done, &expected, 2 ) == true )
>           cb();
>       mutex_unlock;
>   }
> }
> 

You meant atomic_compare_exchange_*() ? And done should be set to 2
after cb() is completed. Otherwise, second call of vlc_once() may return
without cb() complemented.

> should do the trick
> /!\ warning /!\ : not that much thoughts went into this, so this might be completely wrong/invalid/undefined/scary
> 
> Regards,
> 
>>> As for the ones with atomic ops are most certainly more intricate than they 
>>> need be. If somebody else is capable, willing and available to review them, s/
>>> he is most welcome. They are not getting merged without a proper review 
>>> though.
>>>
>>
>> -- 
>> KO Myung-Hun
>>
>> Using Mozilla SeaMonkey 2.7.2
>> Under OS/2 Warp 4 for Korean with FixPak #15
>> In VirtualBox v4.1.32 on Intel Core i7-3615QM 2.30GHz with 8GB RAM
>>
>> Korean OS/2 User Community : http://www.os2.kr/
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> 
> 

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 2.7.2
Under OS/2 Warp 4 for Korean with FixPak #15
In VirtualBox v4.1.32 on Intel Core i7-3615QM 2.30GHz with 8GB RAM

Korean OS/2 User Community : http://www.os2.kr/