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

[Emil Velikov]

On 1 June 2017 at 15:52, Grazvydas Ignotas <notasas at gmail.com> wrote:
> 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.
>
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