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

[Grazvydas Ignotas]

On Thu, Jun 1, 2017 at 4:23 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.

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