[PATCH] drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[PATCH] drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[Chris Wilson]

By using TYPESAFE_BY_RCU, we accept that requests may be swapped out from
underneath us, even when using rcu_read_lock(). We use a strong barrier
on acquiring the refcount during lookup, but this needs to be paired
with a barrier on re-initialising it. Currently we call dma_fence_init,
which ultimately does a plain atomic_set(1) on the refcount, not
providing any memory barriers. As we inspect some state before even
acquiring the refcount in the lookup (by arguing that we can detect
inconsistent requests), that state should be initialised before the
refcount.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_request.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 5c2c93cbab12..04a0b8e75533 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -768,6 +768,13 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
 	rq->timeline = ce->ring->timeline;
 	GEM_BUG_ON(rq->timeline == &engine->timeline);
 
+	/*
+	 * In order to coordinate with our RCU lookup,
+	 * __i915_gem_active_get_rcu(), we need to ensure that the change
+	 * to rq->engine is visible before acquring the refcount in the lookup.
+	 */
+	smp_wmb();
+
 	spin_lock_init(&rq->lock);
 	dma_fence_init(&rq->fence,
 		       &i915_fence_ops,
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[Patchwork]

== Series Details ==

Series: drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation
URL   : https://patchwork.freedesktop.org/series/47698/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4617 -> Patchwork_9854 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47698/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9854 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_guc:
      fi-skl-guc:         NOTRUN -> DMESG-WARN (fdo#107175, fdo#107258)

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       NOTRUN -> DMESG-FAIL (fdo#106947, fdo#106560)
      fi-skl-guc:         NOTRUN -> DMESG-FAIL (fdo#107174)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-kbl-r:           PASS -> DMESG-WARN (fdo#105602)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-kbl-7560u:       INCOMPLETE -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7567u:       DMESG-FAIL (fdo#106947, fdo#106560) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#102614, fdo#106103) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      {fi-byt-clapper}:   FAIL (fdo#103191, fdo#107362) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107175 https://bugs.freedesktop.org/show_bug.cgi?id=107175
  fdo#107258 https://bugs.freedesktop.org/show_bug.cgi?id=107258
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (51 -> 47) ==

  Additional (1): fi-skl-guc 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4617 -> Patchwork_9854

  CI_DRM_4617: d9f22d63ed928e975312a8a90f7b95157a41f7e0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9854: 22c307b4dfb5e2a2cec7a89d07fab069fbe6f638 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

22c307b4dfb5 drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9854/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[Patchwork]

== Series Details ==

Series: drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation
URL   : https://patchwork.freedesktop.org/series/47698/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4617_full -> Patchwork_9854_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9854_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9854_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9854_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-indfb-plflip-blt:
      shard-hsw:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9854_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          PASS -> FAIL (fdo#105703)

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> FAIL (fdo#103375)

    igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#104873)

    igt@kms_setmode@basic:
      shard-kbl:          NOTRUN -> FAIL (fdo#99912)

    igt@perf_pmu@busy-accuracy-50-bcs0:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          DMESG-FAIL (fdo#106947, fdo#106560) -> PASS

    igt@drv_suspend@shrink:
      shard-snb:          INCOMPLETE (fdo#106886, fdo#105411) -> PASS

    igt@gem_eio@reset-stress:
      shard-hsw:          FAIL -> PASS

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_setmode@basic:
      shard-glk:          FAIL (fdo#99912) -> PASS

    
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4617 -> Patchwork_9854

  CI_DRM_4617: d9f22d63ed928e975312a8a90f7b95157a41f7e0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9854: 22c307b4dfb5e2a2cec7a89d07fab069fbe6f638 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9854/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> By using TYPESAFE_BY_RCU, we accept that requests may be swapped out from
> underneath us, even when using rcu_read_lock(). We use a strong barrier
> on acquiring the refcount during lookup, but this needs to be paired
> with a barrier on re-initialising it. Currently we call dma_fence_init,
> which ultimately does a plain atomic_set(1) on the refcount, not
> providing any memory barriers. As we inspect some state before even
> acquiring the refcount in the lookup (by arguing that we can detect
> inconsistent requests), that state should be initialised before the
> refcount.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_request.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 5c2c93cbab12..04a0b8e75533 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -768,6 +768,13 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>  	rq->timeline = ce->ring->timeline;
>  	GEM_BUG_ON(rq->timeline == &engine->timeline);
>  
> +	/*
> +	 * In order to coordinate with our RCU lookup,
> +	 * __i915_gem_active_get_rcu(), we need to ensure that the change
> +	 * to rq->engine is visible before acquring the refcount in the lookup.
> +	 */
> +	smp_wmb();
> +

There is quite a lot going on here as we try to get a reference
into a shapeshifting request.

By looking at the code acquiring it, dma_fence_get_rcu
and dma_fence_init and then the precheck of the request,
should memory barrier be:

smb_mb_before_atomic()?

Admittedly that would be uglier as fence_init hides the atomic_set,
but it is atomic on we are serializing. Especially
as there is no atomic in callsight.

Further, as engine and the kref are tightly bound,
should we initialize everything not related first, then
do engine init, wmb, fence init in a tight proximity?

Thanks,
-Mika

>  	spin_lock_init(&rq->lock);
>  	dma_fence_init(&rq->fence,
>  		       &i915_fence_ops,
> -- 
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-06 12:12:15)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > By using TYPESAFE_BY_RCU, we accept that requests may be swapped out from
> > underneath us, even when using rcu_read_lock(). We use a strong barrier
> > on acquiring the refcount during lookup, but this needs to be paired
> > with a barrier on re-initialising it. Currently we call dma_fence_init,
> > which ultimately does a plain atomic_set(1) on the refcount, not
> > providing any memory barriers. As we inspect some state before even
> > acquiring the refcount in the lookup (by arguing that we can detect
> > inconsistent requests), that state should be initialised before the
> > refcount.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_request.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 5c2c93cbab12..04a0b8e75533 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -768,6 +768,13 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
> >       rq->timeline = ce->ring->timeline;
> >       GEM_BUG_ON(rq->timeline == &engine->timeline);
> >  
> > +     /*
> > +      * In order to coordinate with our RCU lookup,
> > +      * __i915_gem_active_get_rcu(), we need to ensure that the change
> > +      * to rq->engine is visible before acquring the refcount in the lookup.
> > +      */
> > +     smp_wmb();
> > +
> 
> There is quite a lot going on here as we try to get a reference
> into a shapeshifting request.
> 
> By looking at the code acquiring it, dma_fence_get_rcu
> and dma_fence_init and then the precheck of the request,
> should memory barrier be:
> 
> smb_mb_before_atomic()?

No. The code does have a mb, smb_mb_before_atomic is only for atomics
that don't themselves enforce a mb and so you need a bit of extra
weight. On x86, it's not even a mb, just a compiler barrier.

> Admittedly that would be uglier as fence_init hides the atomic_set,
> but it is atomic on we are serializing. Especially
> as there is no atomic in callsight.

Right, the suggestion in the thread was to use atomic_set_release(), but
that requires a lot of deconstruction merely to do the same: it adds
smp_mb() before the atomic_set.

> Further, as engine and the kref are tightly bound,
> should we initialize everything not related first, then
> do engine init, wmb, fence init in a tight proximity?

As we do. The existing order is sufficient for our needs. Everything
that needs to be initialised before the kref, is -- though I think it's
overkill as our argument about checking stale state is still correct and
safe. So what this nails down is the stability of a full referenced
request -- which is less worrisome as it will only be exposed to the rcu
onlookers much later, we don't have the same danger of immediate
exposure to rcu walkers.

What I do think is useful overall is that it gives the companion mb to
the one referenced by __i915_gem_active_get_rcu, and dma_fence_get_rcu
generally.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enforce TYPESAFE_BY_RCU vs refcount mb on reinitialisation

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-08-06 12:12:15)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > By using TYPESAFE_BY_RCU, we accept that requests may be swapped out from
>> > underneath us, even when using rcu_read_lock(). We use a strong barrier
>> > on acquiring the refcount during lookup, but this needs to be paired
>> > with a barrier on re-initialising it. Currently we call dma_fence_init,
>> > which ultimately does a plain atomic_set(1) on the refcount, not
>> > providing any memory barriers. As we inspect some state before even
>> > acquiring the refcount in the lookup (by arguing that we can detect
>> > inconsistent requests), that state should be initialised before the
>> > refcount.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/i915_request.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> > index 5c2c93cbab12..04a0b8e75533 100644
>> > --- a/drivers/gpu/drm/i915/i915_request.c
>> > +++ b/drivers/gpu/drm/i915/i915_request.c
>> > @@ -768,6 +768,13 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
>> >       rq->timeline = ce->ring->timeline;
>> >       GEM_BUG_ON(rq->timeline == &engine->timeline);
>> >  
>> > +     /*
>> > +      * In order to coordinate with our RCU lookup,
>> > +      * __i915_gem_active_get_rcu(), we need to ensure that the change
>> > +      * to rq->engine is visible before acquring the refcount in the lookup.
>> > +      */
>> > +     smp_wmb();
>> > +
>> 
>> There is quite a lot going on here as we try to get a reference
>> into a shapeshifting request.
>> 
>> By looking at the code acquiring it, dma_fence_get_rcu
>> and dma_fence_init and then the precheck of the request,
>> should memory barrier be:
>> 
>> smb_mb_before_atomic()?
>
> No. The code does have a mb, smb_mb_before_atomic is only for atomics
> that don't themselves enforce a mb and so you need a bit of extra
> weight. On x86, it's not even a mb, just a compiler barrier.
>
>> Admittedly that would be uglier as fence_init hides the atomic_set,
>> but it is atomic on we are serializing. Especially
>> as there is no atomic in callsight.
>
> Right, the suggestion in the thread was to use atomic_set_release(), but
> that requires a lot of deconstruction merely to do the same: it adds
> smp_mb() before the atomic_set.
>
>> Further, as engine and the kref are tightly bound,
>> should we initialize everything not related first, then
>> do engine init, wmb, fence init in a tight proximity?
>
> As we do. The existing order is sufficient for our needs. Everything
> that needs to be initialised before the kref, is -- though I think it's
> overkill as our argument about checking stale state is still correct and
> safe. So what this nails down is the stability of a full referenced
> request -- which is less worrisome as it will only be exposed to the rcu
> onlookers much later, we don't have the same danger of immediate
> exposure to rcu walkers.
>
> What I do think is useful overall is that it gives the companion mb to
> the one referenced by __i915_gem_active_get_rcu, and dma_fence_get_rcu
> generally.

Agreed.

I tried to think how to improve the comment pairing, but
the rabbit hole is deep in here. Mentioning the refcount
should guide the reader into right spots tho.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx