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

Fri Apr 6 10:16:53 CEST 2018

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.

> > 
> >> 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?

> 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 :)

> > 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)

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

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr