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

[Hugo Beauzée-Luyssen]

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.

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

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

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


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