[Mesa-dev] [PATCH] gallium/os: add os_wait_until_zero

[Marek Olšák]

My question was how to fix p_atomic_read? Would the volatile read that
I proposed work?

Marek


On Fri, Jun 26, 2015 at 5:59 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> The compiler doesn't know that there's another thread. And it won't
> start to assume that there might always be another thread because then
> it could never optimize pointer derefs.
>
> On Fri, Jun 26, 2015 at 11:55 AM, Marek Olšák <maraeo at gmail.com> wrote:
>> I assumed the atomic operation in another thread would act as a
>> barrier in this case. Is that right?
>>
>> Marek
>>
>> On Fri, Jun 26, 2015 at 5:47 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> On Fri, Jun 26, 2015 at 11:33 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>>> I expect the variable will be changed using an atomic operation by the
>>>> CPU, or using a coherent store instruction by the GPU.
>>>>
>>>> If this is wrong and volatile is really required here, then
>>>> p_atomic_read is wrong too. Should we fix it? For example:
>>>>
>>>> #define p_atomic_read(_v) (*(volatile int*)(_v))
>>>>
>>>> Then, os_wait_until_zero can use p_atomic_read.
>>>
>>> The problem is the lack of a memory barrier. A function call to
>>> sched_yield implies a barrier (since the function could well have
>>> changed the memory that var points to), and so it's all fine. But on a
>>> non-PIPE_OS_UNIX os, this becomes
>>>
>>> while (*var);
>>>
>>> Which the compiler would be well within its right to rewrite to
>>>
>>> x = *var;
>>> while (x);
>>>
>>> which wouldn't end well. You need a barrier that tells the compiler
>>> that memory may be invalidated... I believe this maps to either mb()
>>> or barrier() in the linux kernel, and they have some fancy assembly
>>> syntax to tell the compiler "oops, memory may have changed". However
>>> in this case, it may be easier to just mark the var volatile.
>>>
>>>>
>>>> Marek
>>>>
>>>> On Fri, Jun 26, 2015 at 4:48 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>> On Fri, Jun 26, 2015 at 7:05 AM, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>>
>>>>>> This will be used by radeon and amdgpu winsyses.
>>>>>> Copied from the amdgpu winsys.
>>>>>> ---
>>>>>>  src/gallium/auxiliary/os/os_time.c | 36 +++++++++++++++++++++++++++++++++++-
>>>>>>  src/gallium/auxiliary/os/os_time.h | 10 ++++++++++
>>>>>>  2 files changed, 45 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/gallium/auxiliary/os/os_time.c b/src/gallium/auxiliary/os/os_time.c
>>>>>> index f7e4ca4..63b6879 100644
>>>>>> --- a/src/gallium/auxiliary/os/os_time.c
>>>>>> +++ b/src/gallium/auxiliary/os/os_time.c
>>>>>> @@ -33,11 +33,12 @@
>>>>>>   */
>>>>>>
>>>>>>
>>>>>> -#include "pipe/p_config.h"
>>>>>> +#include "pipe/p_defines.h"
>>>>>>
>>>>>>  #if defined(PIPE_OS_UNIX)
>>>>>>  #  include <time.h> /* timeval */
>>>>>>  #  include <sys/time.h> /* timeval */
>>>>>> +#  include <sched.h> /* sched_yield */
>>>>>>  #elif defined(PIPE_SUBSYSTEM_WINDOWS_USER)
>>>>>>  #  include <windows.h>
>>>>>>  #else
>>>>>> @@ -92,3 +93,36 @@ os_time_sleep(int64_t usecs)
>>>>>>  }
>>>>>>
>>>>>>  #endif
>>>>>> +
>>>>>> +
>>>>>> +bool os_wait_until_zero(int *var, uint64_t timeout)
>>>>>
>>>>> Does this need to be volatile?
>>>>>
>>>>>> +{
>>>>>> +   if (!*var)
>>>>>> +      return true;
>>>>>> +
>>>>>> +   if (!timeout)
>>>>>> +      return false;
>>>>>> +
>>>>>> +   if (timeout == PIPE_TIMEOUT_INFINITE) {
>>>>>> +      while (*var) {
>>>>>> +#if defined(PIPE_OS_UNIX)
>>>>>> +         sched_yield();
>>>>>> +#endif
>>>>>> +      }
>>>>>> +      return true;
>>>>>> +   }
>>>>>> +   else {
>>>>>> +      int64_t start_time = os_time_get_nano();
>>>>>> +      int64_t end_time = start_time + timeout;
>>>>>> +
>>>>>> +      while (*var) {
>>>>>> +         if (os_time_timeout(start_time, end_time, os_time_get_nano()))
>>>>>> +            return false;
>>>>>> +
>>>>>> +#if defined(PIPE_OS_UNIX)
>>>>>> +         sched_yield();
>>>>>> +#endif
>>>>>> +      }
>>>>>> +      return true;
>>>>>> +   }
>>>>>> +}
>>>>>> diff --git a/src/gallium/auxiliary/os/os_time.h b/src/gallium/auxiliary/os/os_time.h
>>>>>> index 4fab03c..fdc8040 100644
>>>>>> --- a/src/gallium/auxiliary/os/os_time.h
>>>>>> +++ b/src/gallium/auxiliary/os/os_time.h
>>>>>> @@ -94,6 +94,16 @@ os_time_timeout(int64_t start,
>>>>>>  }
>>>>>>
>>>>>>
>>>>>> +/**
>>>>>> + * Wait until the variable at the given memory location is zero.
>>>>>> + *
>>>>>> + * \param var           variable
>>>>>> + * \param timeout       timeout, can be anything from 0 (no wait) to
>>>>>> + *                      PIPE_TIME_INFINITE (wait forever)
>>>>>> + * \return     true if the variable is zero
>>>>>> + */
>>>>>> +bool os_wait_until_zero(int *var, uint64_t timeout);
>>>>>> +
>>>>>>  #ifdef __cplusplus
>>>>>>  }
>>>>>>  #endif
>>>>>> --
>>>>>> 2.1.0
>>>>>>
>>>>>> _______________________________________________
>>>>>> mesa-dev mailing list
>>>>>> mesa-dev at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev