[Mesa-dev] [PATCH mesa 2/2] egl/display: make platform detection thread-safe

[Emil Velikov]

Hi guys,

On 1 June 2017 at 12:56, Grazvydas Ignotas <notasas at gmail.com> wrote:
> On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom
> <eric.engestrom at 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 <notasas at gmail.com>
>
> Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252
>
>> Signed-off-by: Eric Engestrom <eric.engestrom at 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.

Reverting the patch (as suggested by Grazvydas) does not fully address
the problem, but only makes it less likely to hit.

-Emil
P.S. Even with this resolved we're likely to hit another issue as
mentioned here https://bugs.freedesktop.org/show_bug.cgi?id=54971