On Thu, Jun 1, 2017 at 4:23 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > Hi guys, > > On 1 June 2017 at 12:56, Grazvydas Ignotas <nota...@gmail.com> wrote: >> On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom >> <eric.engest...@imgtec.com> wrote: >>> If the detections methods ever become able to return different results >>> for different threads, the if chain might make threads go back and forth >>> between invalid and valid platforms. >>> Solve this by doing the detection in a local var and only overwriting >>> the global one at the end, if no other thread has updated it since. >>> >>> This means the platform detected in the thread might not be the platform >>> returned by the function, but this is a different issue that will need >>> to be discussed when this becomes possible. >>> >>> Reported-by: Grazvydas Ignotas <nota...@gmail.com> >> >> Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252 >> >>> Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com> >>> --- >>> >>> This is unnecessary in my opinion, but doesn't hurt :) >> >> It is necessary, without it things will work most of the time but >> there will be rare failures. >> Imagine there are 2 threads that both call _eglGetNativePlatform() >> simultaneously: >> - thread 1 completes the first "if (native_platform == >> _EGL_INVALID_PLATFORM)" check and is preemted to do something else >> - thread 2 executes the whole function, does "native_platform = >> _EGL_NATIVE_PLATFORM" and just before returning it's preemted >> - thread 1 wakes up and calls _eglGetNativePlatformFromEnv() which >> returns _EGL_INVALID_PLATFORM because no env vars are set, updates >> native_platform and then gets preemted again >> - thread 2 wakes up and returns wrong _EGL_INVALID_PLATFORM >> > Afaict this/similar fix is necessary, yet let me put an alternative > idea - add locking via _eglGlobal.Mutex. > May be an bit of overkill, but it's what we consistently use > throughout the code base.
It looks like that mutex is meant to protect _eglGlobal and not some random variables, so I'd prefer the atomic version. > Reverting the patch (as suggested by Grazvydas) does not fully address > the problem, but only makes it less likely to hit. Yeah I meant reverting instead of taking 1/2 and then applying this fix on top. OTOH I'm also ok with this series as-is. GraÅžvydas _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
