[Intel-gfx] [PATCH] drm/i915: Mark up racy read of rq->engine

[Intel-gfx] [PATCH] drm/i915: Mark up racy read of rq->engine

[Chris Wilson]

As the i915_request.engine may be updated by a virtual engine to either
point to the virtual engine or the real physical engine on submission,
we have to be wary that the engine pointer may change.

[  213.317076] BUG: KCSAN: data-race in execlists_dequeue [i915] / i915_request_wait [i915]
[  213.317097]
[  213.317110] write (marked) to 0xffff8881e8647650 of 8 bytes by interrupt on cpu 2:
[  213.317386]  execlists_dequeue+0x43b/0x1670 [i915]
[  213.317645]  __execlists_submission_tasklet+0x48/0x60 [i915]
[  213.317905]  execlists_submission_tasklet+0xd3/0x170 [i915]
[  213.317926]  tasklet_action_common.isra.0+0x42/0x90
[  213.317943]  __do_softirq+0xc8/0x206
[  213.317958]  irq_exit+0xcd/0xe0
[  213.317980]  irq_work_interrupt+0xf/0x20
[  213.317999]  __tsan_read8+0x30/0x100
[  213.318272]  retire_requests+0xdd/0xf0 [i915]
[  213.318502]  engine_retire+0xa6/0xe0 [i915]
[  213.318519]  process_one_work+0x3af/0x640
[  213.318534]  worker_thread+0x80/0x670
[  213.318548]  kthread+0x19a/0x1e0
[  213.318566]  ret_from_fork+0x1f/0x30
[  213.318584]
[  213.318595] read to 0xffff8881e8647650 of 8 bytes by task 458 on cpu 1:
[  213.318847]  i915_request_wait+0x3e3/0x510 [i915]
[  213.319088]  i915_gem_object_wait_fence+0x81/0xa0 [i915]
[  213.319328]  i915_gem_object_wait+0x26b/0x560 [i915]
[  213.319578]  i915_gem_wait_ioctl+0x141/0x290 [i915]
[  213.319597]  drm_ioctl_kernel+0xe9/0x130
[  213.319613]  drm_ioctl+0x27d/0x45e
[  213.319628]  ksys_ioctl+0x89/0xb0
[  213.319648]  __x64_sys_ioctl+0x42/0x60
[  213.319666]  do_syscall_64+0x6e/0x2c0
[  213.319680]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

In this case, we are merely trying to flush the most recent engine
associated with the request, and do not care which as the process of
chaing the engine is done by a tasklet, with which we are yielding to.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 22635bbabf06..e9fd20242438 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1660,7 +1660,7 @@ long i915_request_wait(struct i915_request *rq,
 			break;
 		}
 
-		intel_engine_flush_submission(rq->engine);
+		intel_engine_flush_submission(READ_ONCE(rq->engine));
 
 		if (signal_pending_state(state, current)) {
 			timeout = -ERESTARTSYS;
-- 
2.20.1

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Mark up racy read of rq->engine

[Patchwork]

== Series Details ==

Series: drm/i915: Mark up racy read of rq->engine
URL   : https://patchwork.freedesktop.org/series/76392/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8352 -> Patchwork_17439
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17439/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_pm:
    - fi-byt-j1900:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8352/fi-byt-j1900/igt@i915_selftest@live@gt_pm.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17439/fi-byt-j1900/igt@i915_selftest@live@gt_pm.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@workarounds:
    - {fi-tgl-dsi}:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8352/fi-tgl-dsi/igt@i915_selftest@live@workarounds.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17439/fi-tgl-dsi/igt@i915_selftest@live@workarounds.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@sanitycheck:
    - fi-bwr-2160:        [PASS][5] -> [INCOMPLETE][6] ([i915#489])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8352/fi-bwr-2160/igt@i915_selftest@live@sanitycheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17439/fi-bwr-2160/igt@i915_selftest@live@sanitycheck.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - fi-snb-2600:        [FAIL][7] ([i915#1763]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8352/fi-snb-2600/igt@i915_selftest@live@gt_lrc.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17439/fi-snb-2600/igt@i915_selftest@live@gt_lrc.html

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

  [i915#1763]: https://gitlab.freedesktop.org/drm/intel/issues/1763
  [i915#489]: https://gitlab.freedesktop.org/drm/intel/issues/489


Participating hosts (48 -> 43)
------------------------------

  Additional (1): fi-bsw-kefka 
  Missing    (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8352 -> Patchwork_17439

  CI-20190529: 20190529
  CI_DRM_8352: 248cbab28d58c203de956df1db4cdeb53ea97a89 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5608: e7bcaf1dd251d454706c7cd64282f531aec50183 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17439: caf81f8e993e52fdcebf6e56807b254a65257613 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

caf81f8e993e drm/i915: Mark up racy read of rq->engine

== Logs ==

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

Re: [Intel-gfx] [PATCH] drm/i915: Mark up racy read of rq->engine

[Tvrtko Ursulin]

On 23/04/2020 12:58, Chris Wilson wrote:
> As the i915_request.engine may be updated by a virtual engine to either
> point to the virtual engine or the real physical engine on submission,
> we have to be wary that the engine pointer may change.
> 
> [  213.317076] BUG: KCSAN: data-race in execlists_dequeue [i915] / i915_request_wait [i915]
> [  213.317097]
> [  213.317110] write (marked) to 0xffff8881e8647650 of 8 bytes by interrupt on cpu 2:
> [  213.317386]  execlists_dequeue+0x43b/0x1670 [i915]
> [  213.317645]  __execlists_submission_tasklet+0x48/0x60 [i915]
> [  213.317905]  execlists_submission_tasklet+0xd3/0x170 [i915]
> [  213.317926]  tasklet_action_common.isra.0+0x42/0x90
> [  213.317943]  __do_softirq+0xc8/0x206
> [  213.317958]  irq_exit+0xcd/0xe0
> [  213.317980]  irq_work_interrupt+0xf/0x20
> [  213.317999]  __tsan_read8+0x30/0x100
> [  213.318272]  retire_requests+0xdd/0xf0 [i915]
> [  213.318502]  engine_retire+0xa6/0xe0 [i915]
> [  213.318519]  process_one_work+0x3af/0x640
> [  213.318534]  worker_thread+0x80/0x670
> [  213.318548]  kthread+0x19a/0x1e0
> [  213.318566]  ret_from_fork+0x1f/0x30
> [  213.318584]
> [  213.318595] read to 0xffff8881e8647650 of 8 bytes by task 458 on cpu 1:
> [  213.318847]  i915_request_wait+0x3e3/0x510 [i915]
> [  213.319088]  i915_gem_object_wait_fence+0x81/0xa0 [i915]
> [  213.319328]  i915_gem_object_wait+0x26b/0x560 [i915]
> [  213.319578]  i915_gem_wait_ioctl+0x141/0x290 [i915]
> [  213.319597]  drm_ioctl_kernel+0xe9/0x130
> [  213.319613]  drm_ioctl+0x27d/0x45e
> [  213.319628]  ksys_ioctl+0x89/0xb0
> [  213.319648]  __x64_sys_ioctl+0x42/0x60
> [  213.319666]  do_syscall_64+0x6e/0x2c0
> [  213.319680]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> In this case, we are merely trying to flush the most recent engine
> associated with the request, and do not care which as the process of
> chaing the engine is done by a tasklet, with which we are yielding to.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 22635bbabf06..e9fd20242438 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1660,7 +1660,7 @@ long i915_request_wait(struct i915_request *rq,
>   			break;
>   		}
>   
> -		intel_engine_flush_submission(rq->engine);
> +		intel_engine_flush_submission(READ_ONCE(rq->engine));
>   
>   		if (signal_pending_state(state, current)) {
>   			timeout = -ERESTARTSYS;
> 

What with the mutex_acquire/release in this case? No practical effect 
but they are also dereferencing rq->engine... Take a copy of engine for 
lockdep at start and another read for engine flushing in the loop?

Regards,

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

Re: [Intel-gfx] [PATCH] drm/i915: Mark up racy read of rq->engine

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-04-23 15:53:44)
> 
> On 23/04/2020 12:58, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 22635bbabf06..e9fd20242438 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1660,7 +1660,7 @@ long i915_request_wait(struct i915_request *rq,
> >                       break;
> >               }
> >   
> > -             intel_engine_flush_submission(rq->engine);
> > +             intel_engine_flush_submission(READ_ONCE(rq->engine));
> >   
> >               if (signal_pending_state(state, current)) {
> >                       timeout = -ERESTARTSYS;
> > 
> 
> What with the mutex_acquire/release in this case? No practical effect 
> but they are also dereferencing rq->engine... Take a copy of engine for 
> lockdep at start and another read for engine flushing in the loop?

No practical difference [today], since the lockmap is on the gt.

Well that will be interesting in the future. Hmm, we could replace it
with a static [global] lockmap and then link all gt->reset.mutex to.
The only danger then is that we link a wait on one gt with a wait on
another -- except that if we allow it, that will happen naturally.
So I don't see a loss in generality in using a global lockmap.

Make a note for the future.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx