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

[Eric Engestrom]

On Thursday, 2017-06-15 14:24:55 +0300, Grazvydas Ignotas wrote:
> On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom
>  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 
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100877
> 
> > Signed-off-by: Eric Engestrom 
> 
> It seems this has stalled.

Yes indeed, I kinda forgot about this patchset.
Thanks for the reminder :)

> With the commit message updated (as
> detailed in my previous mail, bad things can happen even with matching
> thread results), and optionally putting everything in a if block to
> avoid expensive cmpxchg on non-first run, this is
> 
> Reviewed-by: Grazvydas Ignotas 
> 
> I think you can add Emil's Acked-by too.

I'll do all that tonight.

Cheers,
  Eric

/me notes that he should look at his other forgotten patches.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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
 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 

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100877

> Signed-off-by: Eric Engestrom 

It seems this has stalled. With the commit message updated (as
detailed in my previous mail, bad things can happen even with matching
thread results), and optionally putting everything in a if block to
avoid expensive cmpxchg on non-first run, this is

Reviewed-by: Grazvydas Ignotas 

I think you can add Emil's Acked-by too.

> ---
>
> This is unnecessary in my opinion, but doesn't hurt :)
> 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 
>  #include 
>  #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(_platform, _EGL_INVALID_PLATFORM,
> +detected_platform);
> +
> if (detection_method != NULL) {
>_eglLog(_EGL_DEBUG, "Native platform type: %s (%s)",
>egl_platforms[native_platform].name, detection_method);
> --
> Cheers,
>   Eric
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 1 June 2017 at 15:52, Grazvydas Ignotas  wrote:
> On Thu, Jun 1, 2017 at 4:23 PM, Emil Velikov  wrote:
>> Hi guys,
>>
>> On 1 June 2017 at 12:56, Grazvydas Ignotas  wrote:
>>> On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom
>>>  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 
>>>
>>> Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252
>>>
 Signed-off-by: Eric Engestrom 
 ---

 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

Re: [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  wrote:
> Hi guys,
>
> On 1 June 2017 at 12:56, Grazvydas Ignotas  wrote:
>> On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom
>>  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 
>>
>> Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252
>>
>>> Signed-off-by: Eric Engestrom 
>>> ---
>>>
>>> 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  wrote:
> On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom
>  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 
>
> Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252
>
>> Signed-off-by: Eric Engestrom 
>> ---
>>
>> 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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
 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 

Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252

> Signed-off-by: Eric Engestrom 
> ---
>
> 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 
>  #include 
>  #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(_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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eric Engestrom]

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 
Signed-off-by: Eric Engestrom 
---

This is unnecessary in my opinion, but doesn't hurt :)
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 
 #include 
 #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(_platform, _EGL_INVALID_PLATFORM,
+detected_platform);
+
if (detection_method != NULL) {
   _eglLog(_EGL_DEBUG, "Native platform type: %s (%s)",
   egl_platforms[native_platform].name, detection_method);
-- 
Cheers,
  Eric

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev