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

[Ilia Mirkin]

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