On 1 June 2017 at 15:52, Grazvydas Ignotas <nota...@gmail.com> wrote: > 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. > Right I should have said "a mutex" as opposed to the _eglGlobal one. Mostly because we have ~20 instances vs ~3 atomics. In either way, this is more of a bikeshedding so as long as you guys are happy, let's go with any solution.
In either way, please add the bugzilla reference. -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
