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

[Grazvydas Ignotas]

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

> Putting it out there as a possible fix, but my vote is to not merge it,
> not until this per-thread-platform-detection becomes possible anyway.
> ---
>  src/egl/main/egldisplay.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/egl/main/egldisplay.c b/src/egl/main/egldisplay.c
> index 3f7752f53d..92586def0c 100644
> --- a/src/egl/main/egldisplay.c
> +++ b/src/egl/main/egldisplay.c
> @@ -36,6 +36,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include "c11/threads.h"
> +#include "util/u_atomic.h"
>
>  #include "eglcontext.h"
>  #include "eglcurrent.h"
> @@ -181,23 +182,27 @@ _EGLPlatformType
>  _eglGetNativePlatform(void *nativeDisplay)
>  {
>     static _EGLPlatformType native_platform = _EGL_INVALID_PLATFORM;
> +   _EGLPlatformType detected_platform = native_platform;
>     char *detection_method = NULL;
>
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _eglGetNativePlatformFromEnv();
> +   if (detected_platform == _EGL_INVALID_PLATFORM) {
> +      detected_platform = _eglGetNativePlatformFromEnv();
>        detection_method = "environment overwrite";
>     }
>
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _eglNativePlatformDetectNativeDisplay(nativeDisplay);
> +   if (detected_platform == _EGL_INVALID_PLATFORM) {
> +      detected_platform = _eglNativePlatformDetectNativeDisplay(nativeDisplay);
>        detection_method = "autodetected";
>     }
>
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _EGL_NATIVE_PLATFORM;
> +   if (detected_platform == _EGL_INVALID_PLATFORM) {
> +      detected_platform = _EGL_NATIVE_PLATFORM;
>        detection_method = "build-time configuration";
>     }
>
> +   p_atomic_cmpxchg(&native_platform, _EGL_INVALID_PLATFORM,
> +                    detected_platform);

I'd make the first "if (detected_platform == _EGL_INVALID_PLATFORM) {"
block end here to avoid doing the slow cmpxchg every time, which makes
it similar to how it looked before your original  patch that I wanted
to revert.

Gražvydas