Re: [PATCH 2/2] drm/i915: Add compiler barrier to wait_for

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Mika Kuoppala (2018-04-20 14:45:50)
> We need to be careful to not let compiler evaluate
> the expiration and the operation on it's terms.
> 
> Document and enforce that COND will be evaluated
> before checking timeout expiration.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ac7565220aa3..4dc346716bb4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -56,6 +56,8 @@
>         for (;;) {                                                      \
>                 const bool expired__ = ktime_after(ktime_get_raw(), end__); \
>                 OP;                                                     \
> +               /* Guarantee COND check prior to timeout */             \
> +               barrier();                                              \

Which side of OP is debatable, since OP is caller supplied and our
constraint is targeted at expired__. However, it is likely to be more
useful between OP/COND, so I can see some advantage there.

As Mika noted, moving to a function call should mean that the compiler
has less freedom to reorder/re-evaluate but doesn't completely prevent
it. As such being clear about the order of operations here is just as
important for the reader. There is a long history of gotchas here, and
this patch set continues the trend :)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx