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

Fri Apr 6 15:00:43 CEST 2018

Hugo Beauzée-Luyssen wrote:
> On Thu, Apr 5, 2018, at 1:40 PM, KO Myung-Hun wrote:
>> 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.
>> 
> 
> Since there is no acquire fence before reading, you cannot make any
> assumption about what will be read. It could have been modified by
> another thread and the cache might not be updated.
> 

You're right. However, it does not matter. The important is that if
second thread see done with 1, it's guaranteed that cb() was completed.
Even if cache was not updated, it will be blocked by mutex. At this
time, cache will be updated. And done is checked again.

>>> 
>>>> 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.
>> 
> 
> So you cannot know the actual value of *obj if *obj != expected?
> 

No.

>> In fact, this was the question needed at the beginning instead of 
>> rejecting review.
>> 
>> Anyway do you want to use C11 atomics instead ?
>> 
> 
> I'd rather indeed :)
> 

Ok, attached patch using C11 atomics.

>>> 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.
>> 
> 
> Yep, that might have been too lazy from me :D I definitely meant
> atomic_compare_exchange (strong, in this case) Your point about done
> needing to be set to 2 after cb() is completely correct. I guess it's
> trivial to modify the code correctly since you are in a locked scoped
> already. Granted, having a mutex locked to access an atomic is an
> anti pattern, but I'm not sure how to it properly otherwise (beside
> having an atomic variable and a non atomic variable for the sake of
> it)

Sleep ? ^^

> 
>>> 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/
>> 
>> _______________________________________________ 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/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-os2-implement-vlc_once.patch
Type: application/x-patch
Size: 1800 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180406/69757ed3/attachment.bin>