If p_atomic_read is fine, then this patch is fine too. So you're telling that this should work:
while (p_atomic_read(var)); I wouldn't be concerned about a memory barrier. This is only 1 int, so it should make its way into the shared cache eventually. Marek On Fri, Jun 26, 2015 at 6:25 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote: > 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 <mar...@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 <imir...@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 <mar...@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 <imir...@alum.mit.edu> wrote: >>>>> On Fri, Jun 26, 2015 at 11:33 AM, Marek Olšák <mar...@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 <imir...@alum.mit.edu> >>>>>> wrote: >>>>>>> On Fri, Jun 26, 2015 at 7:05 AM, Marek Olšák <mar...@gmail.com> wrote: >>>>>>>> From: Marek Olšák <marek.ol...@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@lists.freedesktop.org >>>>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev