From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/6] drm/i915: Avoid accessing request->timeline outside of its lifetime
Date: Mon, 31 Oct 2016 17:35:50 +0000 [thread overview]
Message-ID: <888199f1-3635-2cc4-e5e1-1057519d9cfb@linux.intel.com> (raw)
In-Reply-To: <20161031102645.29495-2-chris@chris-wilson.co.uk>
On 31/10/2016 10:26, Chris Wilson wrote:
> Whilst waiting on a request, we may do so without holding any locks or
> any guards beyond a reference to the request. In order to avoid taking
> locks within request deallocation, we drop references to its timeline
> (via the context and ppgtt) upon retirement. We should avoid chasing
Couldn't find that there is a reference taken (or dropped) on the
timeline when stored in a request. It looks like a borrowed pointer to me?
> such pointers outside of their control, in particular we inspect the
> request->timeline to see if we may restore the RPS waitboost for a
> client. If we instead look at the engine->timeline, we will have similar
> behaviour on both full-ppgtt and !full-ppgtt systems and reduce the
> amount of reward we give towards stalling clients (i.e. only if the
> client stalls and the GPU is uncontended does it reclaim its boost).
> This restores behaviour back to pre-timelines, whilst fixing:
>
> [ 645.078485] BUG: KASAN: use-after-free in i915_gem_object_wait_fence+0x1ee/0x2e0 at addr ffff8802335643a0
> [ 645.078577] Read of size 4 by task gem_exec_schedu/28408
> [ 645.078638] CPU: 1 PID: 28408 Comm: gem_exec_schedu Not tainted 4.9.0-rc2+ #64
> [ 645.078724] Hardware name: / , BIOS PYBSWCEL.86A.0027.2015.0507.1758 05/07/2015
> [ 645.078816] ffff88022daef9a0 ffffffff8143d059 ffff880235402a80 ffff880233564200
> [ 645.078998] ffff88022daef9c8 ffffffff81229c5c ffff88022daefa48 ffff880233564200
> [ 645.079172] ffff880235402a80 ffff88022daefa38 ffffffff81229ef0 000000008110a796
> [ 645.079345] Call Trace:
> [ 645.079404] [<ffffffff8143d059>] dump_stack+0x68/0x9f
> [ 645.079467] [<ffffffff81229c5c>] kasan_object_err+0x1c/0x70
> [ 645.079534] [<ffffffff81229ef0>] kasan_report_error+0x1f0/0x4b0
> [ 645.079601] [<ffffffff8122a244>] kasan_report+0x34/0x40
> [ 645.079676] [<ffffffff81634f5e>] ? i915_gem_object_wait_fence+0x1ee/0x2e0
> [ 645.079741] [<ffffffff81229951>] __asan_load4+0x61/0x80
> [ 645.079807] [<ffffffff81634f5e>] i915_gem_object_wait_fence+0x1ee/0x2e0
> [ 645.079876] [<ffffffff816364bf>] i915_gem_object_wait+0x19f/0x590
> [ 645.079944] [<ffffffff81636320>] ? i915_gem_object_wait_priority+0x500/0x500
> [ 645.080016] [<ffffffff8110fb30>] ? debug_show_all_locks+0x1e0/0x1e0
> [ 645.080084] [<ffffffff8110abdc>] ? check_chain_key+0x14c/0x210
> [ 645.080157] [<ffffffff8110a796>] ? __lock_is_held+0x46/0xc0
> [ 645.080226] [<ffffffff8163bc61>] ? i915_gem_set_domain_ioctl+0x141/0x690
> [ 645.080296] [<ffffffff8163bcc2>] i915_gem_set_domain_ioctl+0x1a2/0x690
> [ 645.080366] [<ffffffff811f8f85>] ? __might_fault+0x75/0xe0
> [ 645.080433] [<ffffffff815a55f7>] drm_ioctl+0x327/0x640
> [ 645.080508] [<ffffffff8163bb20>] ? i915_gem_obj_prepare_shmem_write+0x3a0/0x3a0
> [ 645.080603] [<ffffffff815a52d0>] ? drm_ioctl_permit+0x120/0x120
> [ 645.080670] [<ffffffff8110abdc>] ? check_chain_key+0x14c/0x210
> [ 645.080738] [<ffffffff81275717>] do_vfs_ioctl+0x127/0xa20
> [ 645.080804] [<ffffffff8120268c>] ? do_mmap+0x47c/0x580
> [ 645.080871] [<ffffffff811da567>] ? vm_mmap_pgoff+0x117/0x140
> [ 645.080938] [<ffffffff812755f0>] ? ioctl_preallocate+0x150/0x150
> [ 645.081011] [<ffffffff81108c53>] ? up_write+0x23/0x50
> [ 645.081078] [<ffffffff811da567>] ? vm_mmap_pgoff+0x117/0x140
> [ 645.081145] [<ffffffff811da450>] ? vma_is_stack_for_current+0x90/0x90
> [ 645.081214] [<ffffffff8110d853>] ? mark_held_locks+0x23/0xc0
> [ 645.082030] [<ffffffff81288408>] ? __fget+0x168/0x250
> [ 645.082106] [<ffffffff819ad517>] ? entry_SYSCALL_64_fastpath+0x5/0xb1
> [ 645.082176] [<ffffffff81288592>] ? __fget_light+0xa2/0xc0
> [ 645.082242] [<ffffffff8127604c>] SyS_ioctl+0x3c/0x70
> [ 645.082309] [<ffffffff819ad52e>] entry_SYSCALL_64_fastpath+0x1c/0xb1
> [ 645.082374] Object at ffff880233564200, in cache kmalloc-8192 size: 8192
> [ 645.082431] Allocated:
> [ 645.082480] PID = 28408
> [ 645.082535] [ 645.082566] [<ffffffff8103ae66>] save_stack_trace+0x16/0x20
> [ 645.082623] [ 645.082656] [<ffffffff81228b06>] save_stack+0x46/0xd0
> [ 645.082716] [ 645.082756] [<ffffffff812292fd>] kasan_kmalloc+0xad/0xe0
> [ 645.082817] [ 645.082848] [<ffffffff81631752>] i915_ppgtt_create+0x52/0x220
> [ 645.082908] [ 645.082941] [<ffffffff8161db96>] i915_gem_create_context+0x396/0x560
> [ 645.083027] [ 645.083059] [<ffffffff8161f857>] i915_gem_context_create_ioctl+0x97/0xf0
> [ 645.083152] [ 645.083183] [<ffffffff815a55f7>] drm_ioctl+0x327/0x640
> [ 645.083243] [ 645.083274] [<ffffffff81275717>] do_vfs_ioctl+0x127/0xa20
> [ 645.083334] [ 645.083372] [<ffffffff8127604c>] SyS_ioctl+0x3c/0x70
> [ 645.083432] [ 645.083464] [<ffffffff819ad52e>] entry_SYSCALL_64_fastpath+0x1c/0xb1
> [ 645.083551] Freed:
> [ 645.083599] PID = 27629
> [ 645.083648] [ 645.083676] [<ffffffff8103ae66>] save_stack_trace+0x16/0x20
> [ 645.083738] [ 645.083770] [<ffffffff81228b06>] save_stack+0x46/0xd0
> [ 645.083830] [ 645.083862] [<ffffffff81229203>] kasan_slab_free+0x73/0xc0
> [ 645.083922] [ 645.083961] [<ffffffff812279c9>] kfree+0xa9/0x170
> [ 645.084021] [ 645.084053] [<ffffffff81629f60>] i915_ppgtt_release+0x100/0x180
> [ 645.084139] [ 645.084171] [<ffffffff8161d414>] i915_gem_context_free+0x1b4/0x230
> [ 645.084257] [ 645.084288] [<ffffffff816537b2>] intel_lr_context_unpin+0x192/0x230
> [ 645.084380] [ 645.084413] [<ffffffff81645250>] i915_gem_request_retire+0x620/0x630
> [ 645.084500] [ 645.085226] [<ffffffff816473d1>] i915_gem_retire_requests+0x181/0x280
> [ 645.085313] [ 645.085352] [<ffffffff816352ba>] i915_gem_retire_work_handler+0xca/0xe0
> [ 645.085440] [ 645.085471] [<ffffffff810c725b>] process_one_work+0x4fb/0x920
> [ 645.085532] [ 645.085562] [<ffffffff810c770d>] worker_thread+0x8d/0x840
> [ 645.085622] [ 645.085653] [<ffffffff810d21e5>] kthread+0x185/0x1b0
> [ 645.085718] [ 645.085750] [<ffffffff819ad7a7>] ret_from_fork+0x27/0x40
> [ 645.085811] Memory state around the buggy address:
> [ 645.085869] ffff880233564280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 645.085956] ffff880233564300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 645.086053] >ffff880233564380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 645.086138] ^
> [ 645.086193] ffff880233564400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 645.086283] ffff880233564480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>
> Fixes: 73cb97010d4f ("drm/i915: Combine seqno + tracking into a global timeline struct")
> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 7 +++----
> drivers/gpu/drm/i915/i915_gem.c | 4 ++--
> drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
> 5 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9bef6f55f99d..bf19192dcc3b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1348,9 +1348,8 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>
> seq_printf(m, "%s:\n", engine->name);
> seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
> - engine->hangcheck.seqno,
> - seqno[id],
> - engine->timeline->last_submitted_seqno);
> + engine->hangcheck.seqno, seqno[id],
> + intel_engine_last_submit(engine));
> seq_printf(m, "\twaiters? %s, fake irq active? %s\n",
> yesno(intel_engine_has_waiter(engine)),
> yesno(test_bit(engine->id,
> @@ -3167,7 +3166,7 @@ static int i915_engine_info(struct seq_file *m, void *unused)
> seq_printf(m, "%s\n", engine->name);
> seq_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [score %d]\n",
> intel_engine_get_seqno(engine),
> - engine->timeline->last_submitted_seqno,
> + intel_engine_last_submit(engine),
> engine->hangcheck.seqno,
> engine->hangcheck.score);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1e5d2bf777e4..b9f540b16a45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -371,7 +371,7 @@ i915_gem_object_wait_fence(struct dma_fence *fence,
> if (flags & I915_WAIT_LOCKED && i915_gem_request_completed(rq))
> i915_gem_request_retire_upto(rq);
You could have stuck with req or request when adding new code. Now we
have three names for requests in the code base. :(
>
> - if (rps && rq->fence.seqno == rq->timeline->last_submitted_seqno) {
> + if (rps && rq->global_seqno == intel_engine_last_submit(rq->engine)) {
Okay so not touching the rq->timeline here and rq->global_seqno is from
the same seqno-space as the engine timeline seqnos I assume.
> /* The GPU is now idle and this client has stalled.
> * Since no other client has submitted a request in the
> * meantime, assume that this client is the only one
> @@ -2674,7 +2674,7 @@ static void i915_gem_cleanup_engine(struct intel_engine_cs *engine)
> * reset it.
> */
> intel_engine_init_global_seqno(engine,
> - engine->timeline->last_submitted_seqno);
> + intel_engine_last_submit(engine));
>
> /*
> * Clear the execlists queue up before freeing the requests, as those
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 7ba40487e345..204093f3eaa5 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1120,7 +1120,7 @@ static void error_record_engine_registers(struct drm_i915_error_state *error,
> ee->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
> ee->acthd = intel_engine_get_active_head(engine);
> ee->seqno = intel_engine_get_seqno(engine);
> - ee->last_seqno = engine->timeline->last_submitted_seqno;
> + ee->last_seqno = intel_engine_last_submit(engine);
> ee->start = I915_READ_START(engine);
> ee->head = I915_READ_HEAD(engine);
> ee->tail = I915_READ_TAIL(engine);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 90d0905592f2..4dda2b1eefdb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3168,7 +3168,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>
> acthd = intel_engine_get_active_head(engine);
> seqno = intel_engine_get_seqno(engine);
> - submit = READ_ONCE(engine->timeline->last_submitted_seqno);
> + submit = intel_engine_last_submit(engine);
>
> if (engine->hangcheck.seqno == seqno) {
> if (i915_seqno_passed(seqno, submit)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 43f61aa24ed7..a089e11ee575 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -496,6 +496,11 @@ static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
> return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
> }
>
> +static inline u32 intel_engine_last_submit(struct intel_engine_cs *engine)
> +{
> + return READ_ONCE(engine->timeline->last_submitted_seqno);
> +}
> +
Don't like that READ_ONCE gets sprinkled all over the place via call
sites. It should be extremely well defined and controlled from where it
is used. Otherwise it suggests READ_ONCE is not really appropriate.
> int init_workarounds_ring(struct intel_engine_cs *engine);
>
> void intel_engine_get_instdone(struct intel_engine_cs *engine,
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-10-31 17:35 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-31 10:26 [PATCH 1/6] drm/i915: Use the full hammer when shutting down the rcu tasks Chris Wilson
2016-10-31 10:26 ` [PATCH 2/6] drm/i915: Avoid accessing request->timeline outside of its lifetime Chris Wilson
2016-10-31 17:35 ` Tvrtko Ursulin [this message]
2016-10-31 21:03 ` Chris Wilson
2016-11-01 8:43 ` Tvrtko Ursulin
2016-10-31 10:26 ` [PATCH 3/6] drm/i915: Track pages pinned due to swizzling quirk Chris Wilson
2016-11-01 8:39 ` Tvrtko Ursulin
2016-11-01 8:48 ` Chris Wilson
2016-11-01 8:52 ` Tvrtko Ursulin
2016-10-31 10:26 ` [PATCH 4/6] drm/i915: Discard objects from mm global_list after being shrunk Chris Wilson
2016-11-01 8:29 ` Tvrtko Ursulin
2016-10-31 10:26 ` [PATCH 5/6] drm/i915: Move the recently scanned objects to the tail after shrinking Chris Wilson
2016-10-31 15:26 ` Joonas Lahtinen
2016-10-31 10:26 ` [PATCH 6/6] drm/i915: Store the vma in an rbtree under the object Chris Wilson
2016-11-01 8:41 ` Tvrtko Ursulin
2016-11-01 8:50 ` Chris Wilson
2016-11-01 8:54 ` Tvrtko Ursulin
2016-11-01 9:06 ` Chris Wilson
2016-11-01 9:20 ` Tvrtko Ursulin
2016-11-01 9:45 ` Chris Wilson
2016-11-01 9:43 ` Tvrtko Ursulin
2016-11-01 9:56 ` Chris Wilson
2016-10-31 11:16 ` ✗ Fi.CI.BAT: failure for series starting with [1/6] drm/i915: Use the full hammer when shutting down the rcu tasks Patchwork
2016-10-31 17:15 ` [PATCH 1/6] " Tvrtko Ursulin
2016-10-31 21:05 ` Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=888199f1-3635-2cc4-e5e1-1057519d9cfb@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.