| Message ID | 20180420134550.17590-2-mika.kuoppala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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(); \ if (COND) { \ ret__ = 0; \ break; \ @@ -96,6 +98,8 @@ u64 now = local_clock(); \ if (!(ATOMIC)) \ preempt_enable(); \ + /* Guarantee COND check prior to timeout */ \ + barrier(); \ if (COND) { \ ret = 0; \ break; \
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(+)