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

[Ilia Mirkin]

p_atomic_read is fine as-is. The read is guaranteed to be atomic up to
a word size on x86 processors. I suspect other cpu's have similar
guarantees, and if not, then hopefully have other ops to perform the
read atomically.

On Fri, Jun 26, 2015 at 12:18 PM, Marek Olšák <maraeo at gmail.com> wrote:
> 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