[PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Following a try_to_unmap() we may want to remove the userptr and so call
put_pages(). However, try_to_unmap() acquires the page lock and so we
must avoid recursively locking the pages ourselves -- which means that
we cannot safely acquire the lock around set_page_dirty(). Since we
can't be sure of the lock, we have to risk skip dirtying the page, or
else risk calling set_page_dirty() without a lock and so risk fs
corruption.

Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index b9d2bb15e4a6..1ad2047a6dbd 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 		obj->mm.dirty = false;
 
 	for_each_sgt_page(page, sgt_iter, pages) {
-		if (obj->mm.dirty)
+		if (obj->mm.dirty && trylock_page(page)) {
 			/*
 			 * As this may not be anonymous memory (e.g. shmem)
 			 * but exist on a real mapping, we have to lock
@@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 			 * the page reference is not sufficient to
 			 * prevent the inode from being truncated.
 			 * Play safe and take the lock.
+			 *
+			 * However...!
+			 *
+			 * The mmu-notifier can be invalidated for a
+			 * migrate_page, that is alreadying holding the lock
+			 * on the page. Such a try_to_unmap() will result
+			 * in us calling put_pages() and so recursively try
+			 * to lock the page. We avoid that deadlock with
+			 * a trylock_page() and in exchange we risk missing
+			 * some page dirtying.
 			 */
-			set_page_dirty_lock(page);
+			set_page_dirty(page);
+			unlock_page(page);
+		}
 
 		mark_page_accessed(page);
 		put_page(page);
-- 
2.22.0

[PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Chris Wilson]

Push the engine stop into the back reset_prepare (where it already was!)
This allows us to avoid dangerously setting the RING registers to 0 for
logical contexts. If we clear the register on a live context, those
invalid register values are recorded in the logical context state and
replayed (with hilarious results).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c        | 16 +++++-
 drivers/gpu/drm/i915/gt/intel_reset.c      | 58 ----------------------
 drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 40 ++++++++++++++-
 3 files changed, 53 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 9e0992498087..9b87a2fc186c 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -2173,11 +2173,23 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
 	__tasklet_disable_sync_once(&execlists->tasklet);
 	GEM_BUG_ON(!reset_in_progress(execlists));
 
-	intel_engine_stop_cs(engine);
-
 	/* And flush any current direct submission. */
 	spin_lock_irqsave(&engine->active.lock, flags);
 	spin_unlock_irqrestore(&engine->active.lock, flags);
+
+	/*
+	 * We stop engines, otherwise we might get failed reset and a
+	 * dead gpu (on elk). Also as modern gpu as kbl can suffer
+	 * from system hang if batchbuffer is progressing when
+	 * the reset is issued, regardless of READY_TO_RESET ack.
+	 * Thus assume it is best to stop engines on all gens
+	 * where we have a gpu reset.
+	 *
+	 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
+	 *
+	 * FIXME: Wa for more modern gens needs to be validated
+	 */
+	intel_engine_stop_cs(engine);
 }
 
 static void reset_csb_pointers(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 7ddedfb16aa2..55e2ddcbd215 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -135,47 +135,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
 	}
 }
 
-static void gen3_stop_engine(struct intel_engine_cs *engine)
-{
-	struct intel_uncore *uncore = engine->uncore;
-	const u32 base = engine->mmio_base;
-
-	GEM_TRACE("%s\n", engine->name);
-
-	if (intel_engine_stop_cs(engine))
-		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
-
-	intel_uncore_write_fw(uncore,
-			      RING_HEAD(base),
-			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
-	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
-
-	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
-	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
-	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
-
-	/* The ring must be empty before it is disabled */
-	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
-
-	/* Check acts as a post */
-	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
-		GEM_TRACE("%s: ring head [%x] not parked\n",
-			  engine->name,
-			  intel_uncore_read_fw(uncore, RING_HEAD(base)));
-}
-
-static void stop_engines(struct intel_gt *gt, intel_engine_mask_t engine_mask)
-{
-	struct intel_engine_cs *engine;
-	intel_engine_mask_t tmp;
-
-	if (INTEL_GEN(gt->i915) < 3)
-		return;
-
-	for_each_engine_masked(engine, gt->i915, engine_mask, tmp)
-		gen3_stop_engine(engine);
-}
-
 static bool i915_in_reset(struct pci_dev *pdev)
 {
 	u8 gdrst;
@@ -607,23 +566,6 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
 	 */
 	intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
 	for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
-		/*
-		 * We stop engines, otherwise we might get failed reset and a
-		 * dead gpu (on elk). Also as modern gpu as kbl can suffer
-		 * from system hang if batchbuffer is progressing when
-		 * the reset is issued, regardless of READY_TO_RESET ack.
-		 * Thus assume it is best to stop engines on all gens
-		 * where we have a gpu reset.
-		 *
-		 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
-		 *
-		 * WaMediaResetMainRingCleanup:ctg,elk (presumably)
-		 *
-		 * FIXME: Wa for more modern gens needs to be validated
-		 */
-		if (retry)
-			stop_engines(gt, engine_mask);
-
 		GEM_TRACE("engine_mask=%x\n", engine_mask);
 		preempt_disable();
 		ret = reset(gt, engine_mask, retry);
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index f1e571fa2e6d..213df144be15 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -739,7 +739,45 @@ static int xcs_resume(struct intel_engine_cs *engine)
 
 static void reset_prepare(struct intel_engine_cs *engine)
 {
-	intel_engine_stop_cs(engine);
+	struct intel_uncore *uncore = engine->uncore;
+	const u32 base = engine->mmio_base;
+
+	/*
+	 * We stop engines, otherwise we might get failed reset and a
+	 * dead gpu (on elk). Also as modern gpu as kbl can suffer
+	 * from system hang if batchbuffer is progressing when
+	 * the reset is issued, regardless of READY_TO_RESET ack.
+	 * Thus assume it is best to stop engines on all gens
+	 * where we have a gpu reset.
+	 *
+	 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
+	 *
+	 * WaMediaResetMainRingCleanup:ctg,elk (presumably)
+	 *
+	 * FIXME: Wa for more modern gens needs to be validated
+	 */
+	GEM_TRACE("%s\n", engine->name);
+
+	if (intel_engine_stop_cs(engine))
+		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
+
+	intel_uncore_write_fw(uncore,
+			      RING_HEAD(base),
+			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
+	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
+
+	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
+	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
+	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
+
+	/* The ring must be empty before it is disabled */
+	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
+
+	/* Check acts as a post */
+	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
+		GEM_TRACE("%s: ring head [%x] not parked\n",
+			  engine->name,
+			  intel_uncore_read_fw(uncore, RING_HEAD(base)));
 }
 
 static void reset_ring(struct intel_engine_cs *engine, bool stalled)
-- 
2.22.0

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

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Tvrtko Ursulin]

On 16/07/2019 13:49, Chris Wilson wrote:
> Push the engine stop into the back reset_prepare (where it already was!)
> This allows us to avoid dangerously setting the RING registers to 0 for
> logical contexts. If we clear the register on a live context, those
> invalid register values are recorded in the logical context state and
> replayed (with hilarious results).

So essentially statement is gen3_stop_engine is not needed and even 
dangerous with execlists?

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c        | 16 +++++-
>   drivers/gpu/drm/i915/gt/intel_reset.c      | 58 ----------------------
>   drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 40 ++++++++++++++-
>   3 files changed, 53 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 9e0992498087..9b87a2fc186c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2173,11 +2173,23 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
>   	__tasklet_disable_sync_once(&execlists->tasklet);
>   	GEM_BUG_ON(!reset_in_progress(execlists));
>   
> -	intel_engine_stop_cs(engine);
> -
>   	/* And flush any current direct submission. */
>   	spin_lock_irqsave(&engine->active.lock, flags);
>   	spin_unlock_irqrestore(&engine->active.lock, flags);
> +
> +	/*
> +	 * We stop engines, otherwise we might get failed reset and a
> +	 * dead gpu (on elk). Also as modern gpu as kbl can suffer
> +	 * from system hang if batchbuffer is progressing when
> +	 * the reset is issued, regardless of READY_TO_RESET ack.
> +	 * Thus assume it is best to stop engines on all gens
> +	 * where we have a gpu reset.
> +	 *
> +	 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> +	 *
> +	 * FIXME: Wa for more modern gens needs to be validated
> +	 */
> +	intel_engine_stop_cs(engine);
>   }
>   
>   static void reset_csb_pointers(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 7ddedfb16aa2..55e2ddcbd215 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -135,47 +135,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>   	}
>   }
>   
> -static void gen3_stop_engine(struct intel_engine_cs *engine)
> -{
> -	struct intel_uncore *uncore = engine->uncore;
> -	const u32 base = engine->mmio_base;
> -
> -	GEM_TRACE("%s\n", engine->name);
> -
> -	if (intel_engine_stop_cs(engine))
> -		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
> -
> -	intel_uncore_write_fw(uncore,
> -			      RING_HEAD(base),
> -			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
> -	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
> -
> -	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
> -	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
> -	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
> -
> -	/* The ring must be empty before it is disabled */
> -	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
> -
> -	/* Check acts as a post */
> -	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
> -		GEM_TRACE("%s: ring head [%x] not parked\n",
> -			  engine->name,
> -			  intel_uncore_read_fw(uncore, RING_HEAD(base)));
> -}
> -
> -static void stop_engines(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> -{
> -	struct intel_engine_cs *engine;
> -	intel_engine_mask_t tmp;
> -
> -	if (INTEL_GEN(gt->i915) < 3)
> -		return;
> -
> -	for_each_engine_masked(engine, gt->i915, engine_mask, tmp)
> -		gen3_stop_engine(engine);
> -}
> -
>   static bool i915_in_reset(struct pci_dev *pdev)
>   {
>   	u8 gdrst;
> @@ -607,23 +566,6 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>   	 */
>   	intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
>   	for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
> -		/*
> -		 * We stop engines, otherwise we might get failed reset and a
> -		 * dead gpu (on elk). Also as modern gpu as kbl can suffer
> -		 * from system hang if batchbuffer is progressing when
> -		 * the reset is issued, regardless of READY_TO_RESET ack.
> -		 * Thus assume it is best to stop engines on all gens
> -		 * where we have a gpu reset.
> -		 *
> -		 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> -		 *
> -		 * WaMediaResetMainRingCleanup:ctg,elk (presumably)
> -		 *
> -		 * FIXME: Wa for more modern gens needs to be validated
> -		 */
> -		if (retry)
> -			stop_engines(gt, engine_mask);
> -

Only other functional change I see is that we stop retrying to stop the 
engines before reset attempts. I don't know if that is a concern or not.

Regards,

Tvrtko

>   		GEM_TRACE("engine_mask=%x\n", engine_mask);
>   		preempt_disable();
>   		ret = reset(gt, engine_mask, retry);
> diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> index f1e571fa2e6d..213df144be15 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> @@ -739,7 +739,45 @@ static int xcs_resume(struct intel_engine_cs *engine)
>   
>   static void reset_prepare(struct intel_engine_cs *engine)
>   {
> -	intel_engine_stop_cs(engine);
> +	struct intel_uncore *uncore = engine->uncore;
> +	const u32 base = engine->mmio_base;
> +
> +	/*
> +	 * We stop engines, otherwise we might get failed reset and a
> +	 * dead gpu (on elk). Also as modern gpu as kbl can suffer
> +	 * from system hang if batchbuffer is progressing when
> +	 * the reset is issued, regardless of READY_TO_RESET ack.
> +	 * Thus assume it is best to stop engines on all gens
> +	 * where we have a gpu reset.
> +	 *
> +	 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> +	 *
> +	 * WaMediaResetMainRingCleanup:ctg,elk (presumably)
> +	 *
> +	 * FIXME: Wa for more modern gens needs to be validated
> +	 */
> +	GEM_TRACE("%s\n", engine->name);
> +
> +	if (intel_engine_stop_cs(engine))
> +		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
> +
> +	intel_uncore_write_fw(uncore,
> +			      RING_HEAD(base),
> +			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
> +	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
> +
> +	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
> +	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
> +	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
> +
> +	/* The ring must be empty before it is disabled */
> +	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
> +
> +	/* Check acts as a post */
> +	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
> +		GEM_TRACE("%s: ring head [%x] not parked\n",
> +			  engine->name,
> +			  intel_uncore_read_fw(uncore, RING_HEAD(base)));
>   }
>   
>   static void reset_ring(struct intel_engine_cs *engine, bool stalled)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
> 
> On 16/07/2019 13:49, Chris Wilson wrote:
> > Push the engine stop into the back reset_prepare (where it already was!)
> > This allows us to avoid dangerously setting the RING registers to 0 for
> > logical contexts. If we clear the register on a live context, those
> > invalid register values are recorded in the logical context state and
> > replayed (with hilarious results).
> 
> So essentially statement is gen3_stop_engine is not needed and even 
> dangerous with execlists?

Yes. It has been a nuisance in the past, which is why we try to avoid
it. I have come to conclusion that it serves no purpose for execlists
and only makes recovery worse.

> 
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c        | 16 +++++-
> >   drivers/gpu/drm/i915/gt/intel_reset.c      | 58 ----------------------
> >   drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 40 ++++++++++++++-
> >   3 files changed, 53 insertions(+), 61 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 9e0992498087..9b87a2fc186c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -2173,11 +2173,23 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
> >       __tasklet_disable_sync_once(&execlists->tasklet);
> >       GEM_BUG_ON(!reset_in_progress(execlists));
> >   
> > -     intel_engine_stop_cs(engine);
> > -
> >       /* And flush any current direct submission. */
> >       spin_lock_irqsave(&engine->active.lock, flags);
> >       spin_unlock_irqrestore(&engine->active.lock, flags);
> > +
> > +     /*
> > +      * We stop engines, otherwise we might get failed reset and a
> > +      * dead gpu (on elk). Also as modern gpu as kbl can suffer
> > +      * from system hang if batchbuffer is progressing when
> > +      * the reset is issued, regardless of READY_TO_RESET ack.
> > +      * Thus assume it is best to stop engines on all gens
> > +      * where we have a gpu reset.
> > +      *
> > +      * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> > +      *
> > +      * FIXME: Wa for more modern gens needs to be validated
> > +      */
> > +     intel_engine_stop_cs(engine);
> >   }
> >   
> >   static void reset_csb_pointers(struct intel_engine_cs *engine)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> > index 7ddedfb16aa2..55e2ddcbd215 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > @@ -135,47 +135,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
> >       }
> >   }
> >   
> > -static void gen3_stop_engine(struct intel_engine_cs *engine)
> > -{
> > -     struct intel_uncore *uncore = engine->uncore;
> > -     const u32 base = engine->mmio_base;
> > -
> > -     GEM_TRACE("%s\n", engine->name);
> > -
> > -     if (intel_engine_stop_cs(engine))
> > -             GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
> > -
> > -     intel_uncore_write_fw(uncore,
> > -                           RING_HEAD(base),
> > -                           intel_uncore_read_fw(uncore, RING_TAIL(base)));
> > -     intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
> > -
> > -     intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
> > -     intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
> > -     intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
> > -
> > -     /* The ring must be empty before it is disabled */
> > -     intel_uncore_write_fw(uncore, RING_CTL(base), 0);
> > -
> > -     /* Check acts as a post */
> > -     if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
> > -             GEM_TRACE("%s: ring head [%x] not parked\n",
> > -                       engine->name,
> > -                       intel_uncore_read_fw(uncore, RING_HEAD(base)));
> > -}
> > -
> > -static void stop_engines(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> > -{
> > -     struct intel_engine_cs *engine;
> > -     intel_engine_mask_t tmp;
> > -
> > -     if (INTEL_GEN(gt->i915) < 3)
> > -             return;
> > -
> > -     for_each_engine_masked(engine, gt->i915, engine_mask, tmp)
> > -             gen3_stop_engine(engine);
> > -}
> > -
> >   static bool i915_in_reset(struct pci_dev *pdev)
> >   {
> >       u8 gdrst;
> > @@ -607,23 +566,6 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> >        */
> >       intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
> >       for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
> > -             /*
> > -              * We stop engines, otherwise we might get failed reset and a
> > -              * dead gpu (on elk). Also as modern gpu as kbl can suffer
> > -              * from system hang if batchbuffer is progressing when
> > -              * the reset is issued, regardless of READY_TO_RESET ack.
> > -              * Thus assume it is best to stop engines on all gens
> > -              * where we have a gpu reset.
> > -              *
> > -              * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> > -              *
> > -              * WaMediaResetMainRingCleanup:ctg,elk (presumably)
> > -              *
> > -              * FIXME: Wa for more modern gens needs to be validated
> > -              */
> > -             if (retry)
> > -                     stop_engines(gt, engine_mask);
> > -
> 
> Only other functional change I see is that we stop retrying to stop the 
> engines before reset attempts. I don't know if that is a concern or not.

Ah, but we do stop the engine before resets in *reset_prepare. The other
path to arrive is in sanitize where we don't know enough state to safely
tweak the engines. For those, I claim it shouldn't matter as the engines
should be idle and we only need the reset to clear stale context state.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Tvrtko Ursulin]

On 17/07/2019 14:08, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
>>
>> On 16/07/2019 13:49, Chris Wilson wrote:
>>> Push the engine stop into the back reset_prepare (where it already was!)
>>> This allows us to avoid dangerously setting the RING registers to 0 for
>>> logical contexts. If we clear the register on a live context, those
>>> invalid register values are recorded in the logical context state and
>>> replayed (with hilarious results).
>>
>> So essentially statement is gen3_stop_engine is not needed and even
>> dangerous with execlists?
> 
> Yes. It has been a nuisance in the past, which is why we try to avoid
> it. I have come to conclusion that it serves no purpose for execlists
> and only makes recovery worse.
> 
>>
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c        | 16 +++++-
>>>    drivers/gpu/drm/i915/gt/intel_reset.c      | 58 ----------------------
>>>    drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 40 ++++++++++++++-
>>>    3 files changed, 53 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index 9e0992498087..9b87a2fc186c 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -2173,11 +2173,23 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
>>>        __tasklet_disable_sync_once(&execlists->tasklet);
>>>        GEM_BUG_ON(!reset_in_progress(execlists));
>>>    
>>> -     intel_engine_stop_cs(engine);
>>> -
>>>        /* And flush any current direct submission. */
>>>        spin_lock_irqsave(&engine->active.lock, flags);
>>>        spin_unlock_irqrestore(&engine->active.lock, flags);
>>> +
>>> +     /*
>>> +      * We stop engines, otherwise we might get failed reset and a
>>> +      * dead gpu (on elk). Also as modern gpu as kbl can suffer
>>> +      * from system hang if batchbuffer is progressing when
>>> +      * the reset is issued, regardless of READY_TO_RESET ack.
>>> +      * Thus assume it is best to stop engines on all gens
>>> +      * where we have a gpu reset.
>>> +      *
>>> +      * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
>>> +      *
>>> +      * FIXME: Wa for more modern gens needs to be validated
>>> +      */
>>> +     intel_engine_stop_cs(engine);
>>>    }
>>>    
>>>    static void reset_csb_pointers(struct intel_engine_cs *engine)
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
>>> index 7ddedfb16aa2..55e2ddcbd215 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>> @@ -135,47 +135,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>>>        }
>>>    }
>>>    
>>> -static void gen3_stop_engine(struct intel_engine_cs *engine)
>>> -{
>>> -     struct intel_uncore *uncore = engine->uncore;
>>> -     const u32 base = engine->mmio_base;
>>> -
>>> -     GEM_TRACE("%s\n", engine->name);
>>> -
>>> -     if (intel_engine_stop_cs(engine))
>>> -             GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
>>> -
>>> -     intel_uncore_write_fw(uncore,
>>> -                           RING_HEAD(base),
>>> -                           intel_uncore_read_fw(uncore, RING_TAIL(base)));
>>> -     intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
>>> -
>>> -     intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
>>> -     intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
>>> -     intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
>>> -
>>> -     /* The ring must be empty before it is disabled */
>>> -     intel_uncore_write_fw(uncore, RING_CTL(base), 0);
>>> -
>>> -     /* Check acts as a post */
>>> -     if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
>>> -             GEM_TRACE("%s: ring head [%x] not parked\n",
>>> -                       engine->name,
>>> -                       intel_uncore_read_fw(uncore, RING_HEAD(base)));
>>> -}
>>> -
>>> -static void stop_engines(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>>> -{
>>> -     struct intel_engine_cs *engine;
>>> -     intel_engine_mask_t tmp;
>>> -
>>> -     if (INTEL_GEN(gt->i915) < 3)
>>> -             return;
>>> -
>>> -     for_each_engine_masked(engine, gt->i915, engine_mask, tmp)
>>> -             gen3_stop_engine(engine);
>>> -}
>>> -
>>>    static bool i915_in_reset(struct pci_dev *pdev)
>>>    {
>>>        u8 gdrst;
>>> @@ -607,23 +566,6 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>>>         */
>>>        intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
>>>        for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
>>> -             /*
>>> -              * We stop engines, otherwise we might get failed reset and a
>>> -              * dead gpu (on elk). Also as modern gpu as kbl can suffer
>>> -              * from system hang if batchbuffer is progressing when
>>> -              * the reset is issued, regardless of READY_TO_RESET ack.
>>> -              * Thus assume it is best to stop engines on all gens
>>> -              * where we have a gpu reset.
>>> -              *
>>> -              * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
>>> -              *
>>> -              * WaMediaResetMainRingCleanup:ctg,elk (presumably)
>>> -              *
>>> -              * FIXME: Wa for more modern gens needs to be validated
>>> -              */
>>> -             if (retry)
>>> -                     stop_engines(gt, engine_mask);
>>> -
>>
>> Only other functional change I see is that we stop retrying to stop the
>> engines before reset attempts. I don't know if that is a concern or not.
> 
> Ah, but we do stop the engine before resets in *reset_prepare. The other
> path to arrive is in sanitize where we don't know enough state to safely
> tweak the engines. For those, I claim it shouldn't matter as the engines
> should be idle and we only need the reset to clear stale context state.

Yes I know that we do call stop in prepare, just not on the reset retry 
path. It's the above loop, if reset was failing and needed retries 
before we would re-retried stopping engines and now we would not.

Regards,

Tvrtko



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

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:21:50)
> 
> On 17/07/2019 14:08, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
> >>
> >> On 16/07/2019 13:49, Chris Wilson wrote:
> >>> Push the engine stop into the back reset_prepare (where it already was!)
> >>> This allows us to avoid dangerously setting the RING registers to 0 for
> >>> logical contexts. If we clear the register on a live context, those
> >>> invalid register values are recorded in the logical context state and
> >>> replayed (with hilarious results).
> >>
> >> So essentially statement is gen3_stop_engine is not needed and even
> >> dangerous with execlists?
> > 
> > Yes. It has been a nuisance in the past, which is why we try to avoid
> > it. I have come to conclusion that it serves no purpose for execlists
> > and only makes recovery worse.
> > 
> >>
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> ---
> >>>    drivers/gpu/drm/i915/gt/intel_lrc.c        | 16 +++++-
> >>>    drivers/gpu/drm/i915/gt/intel_reset.c      | 58 ----------------------
> >>>    drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 40 ++++++++++++++-
> >>>    3 files changed, 53 insertions(+), 61 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>> index 9e0992498087..9b87a2fc186c 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>> @@ -2173,11 +2173,23 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
> >>>        __tasklet_disable_sync_once(&execlists->tasklet);
> >>>        GEM_BUG_ON(!reset_in_progress(execlists));
> >>>    
> >>> -     intel_engine_stop_cs(engine);
> >>> -
> >>>        /* And flush any current direct submission. */
> >>>        spin_lock_irqsave(&engine->active.lock, flags);
> >>>        spin_unlock_irqrestore(&engine->active.lock, flags);
> >>> +
> >>> +     /*
> >>> +      * We stop engines, otherwise we might get failed reset and a
> >>> +      * dead gpu (on elk). Also as modern gpu as kbl can suffer
> >>> +      * from system hang if batchbuffer is progressing when
> >>> +      * the reset is issued, regardless of READY_TO_RESET ack.
> >>> +      * Thus assume it is best to stop engines on all gens
> >>> +      * where we have a gpu reset.
> >>> +      *
> >>> +      * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> >>> +      *
> >>> +      * FIXME: Wa for more modern gens needs to be validated
> >>> +      */
> >>> +     intel_engine_stop_cs(engine);
> >>>    }
> >>>    
> >>>    static void reset_csb_pointers(struct intel_engine_cs *engine)
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> >>> index 7ddedfb16aa2..55e2ddcbd215 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> >>> @@ -135,47 +135,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
> >>>        }
> >>>    }
> >>>    
> >>> -static void gen3_stop_engine(struct intel_engine_cs *engine)
> >>> -{
> >>> -     struct intel_uncore *uncore = engine->uncore;
> >>> -     const u32 base = engine->mmio_base;
> >>> -
> >>> -     GEM_TRACE("%s\n", engine->name);
> >>> -
> >>> -     if (intel_engine_stop_cs(engine))
> >>> -             GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
> >>> -
> >>> -     intel_uncore_write_fw(uncore,
> >>> -                           RING_HEAD(base),
> >>> -                           intel_uncore_read_fw(uncore, RING_TAIL(base)));
> >>> -     intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
> >>> -
> >>> -     intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
> >>> -     intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
> >>> -     intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
> >>> -
> >>> -     /* The ring must be empty before it is disabled */
> >>> -     intel_uncore_write_fw(uncore, RING_CTL(base), 0);
> >>> -
> >>> -     /* Check acts as a post */
> >>> -     if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
> >>> -             GEM_TRACE("%s: ring head [%x] not parked\n",
> >>> -                       engine->name,
> >>> -                       intel_uncore_read_fw(uncore, RING_HEAD(base)));
> >>> -}
> >>> -
> >>> -static void stop_engines(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> >>> -{
> >>> -     struct intel_engine_cs *engine;
> >>> -     intel_engine_mask_t tmp;
> >>> -
> >>> -     if (INTEL_GEN(gt->i915) < 3)
> >>> -             return;
> >>> -
> >>> -     for_each_engine_masked(engine, gt->i915, engine_mask, tmp)
> >>> -             gen3_stop_engine(engine);
> >>> -}
> >>> -
> >>>    static bool i915_in_reset(struct pci_dev *pdev)
> >>>    {
> >>>        u8 gdrst;
> >>> @@ -607,23 +566,6 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> >>>         */
> >>>        intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
> >>>        for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
> >>> -             /*
> >>> -              * We stop engines, otherwise we might get failed reset and a
> >>> -              * dead gpu (on elk). Also as modern gpu as kbl can suffer
> >>> -              * from system hang if batchbuffer is progressing when
> >>> -              * the reset is issued, regardless of READY_TO_RESET ack.
> >>> -              * Thus assume it is best to stop engines on all gens
> >>> -              * where we have a gpu reset.
> >>> -              *
> >>> -              * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
> >>> -              *
> >>> -              * WaMediaResetMainRingCleanup:ctg,elk (presumably)
> >>> -              *
> >>> -              * FIXME: Wa for more modern gens needs to be validated
> >>> -              */
> >>> -             if (retry)
> >>> -                     stop_engines(gt, engine_mask);
> >>> -
> >>
> >> Only other functional change I see is that we stop retrying to stop the
> >> engines before reset attempts. I don't know if that is a concern or not.
> > 
> > Ah, but we do stop the engine before resets in *reset_prepare. The other
> > path to arrive is in sanitize where we don't know enough state to safely
> > tweak the engines. For those, I claim it shouldn't matter as the engines
> > should be idle and we only need the reset to clear stale context state.
> 
> Yes I know that we do call stop in prepare, just not on the reset retry 
> path. It's the above loop, if reset was failing and needed retries 
> before we would re-retried stopping engines and now we would not.

The engines are still stopped. The functional change is to remove the
dangerous clearing of RING_HEAD/CTL.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Tvrtko Ursulin]

On 17/07/2019 14:30, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:21:50)
>>
>> On 17/07/2019 14:08, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
>>>>
>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>> Push the engine stop into the back reset_prepare (where it already was!)
>>>>> This allows us to avoid dangerously setting the RING registers to 0 for
>>>>> logical contexts. If we clear the register on a live context, those
>>>>> invalid register values are recorded in the logical context state and
>>>>> replayed (with hilarious results).
>>>>
>>>> So essentially statement is gen3_stop_engine is not needed and even
>>>> dangerous with execlists?
>>>
>>> Yes. It has been a nuisance in the past, which is why we try to avoid
>>> it. I have come to conclusion that it serves no purpose for execlists
>>> and only makes recovery worse.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/gt/intel_lrc.c        | 16 +++++-
>>>>>     drivers/gpu/drm/i915/gt/intel_reset.c      | 58 ----------------------
>>>>>     drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 40 ++++++++++++++-
>>>>>     3 files changed, 53 insertions(+), 61 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>>> index 9e0992498087..9b87a2fc186c 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>>> @@ -2173,11 +2173,23 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
>>>>>         __tasklet_disable_sync_once(&execlists->tasklet);
>>>>>         GEM_BUG_ON(!reset_in_progress(execlists));
>>>>>     
>>>>> -     intel_engine_stop_cs(engine);
>>>>> -
>>>>>         /* And flush any current direct submission. */
>>>>>         spin_lock_irqsave(&engine->active.lock, flags);
>>>>>         spin_unlock_irqrestore(&engine->active.lock, flags);
>>>>> +
>>>>> +     /*
>>>>> +      * We stop engines, otherwise we might get failed reset and a
>>>>> +      * dead gpu (on elk). Also as modern gpu as kbl can suffer
>>>>> +      * from system hang if batchbuffer is progressing when
>>>>> +      * the reset is issued, regardless of READY_TO_RESET ack.
>>>>> +      * Thus assume it is best to stop engines on all gens
>>>>> +      * where we have a gpu reset.
>>>>> +      *
>>>>> +      * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
>>>>> +      *
>>>>> +      * FIXME: Wa for more modern gens needs to be validated
>>>>> +      */
>>>>> +     intel_engine_stop_cs(engine);
>>>>>     }
>>>>>     
>>>>>     static void reset_csb_pointers(struct intel_engine_cs *engine)
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>> index 7ddedfb16aa2..55e2ddcbd215 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>> @@ -135,47 +135,6 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>>>>>         }
>>>>>     }
>>>>>     
>>>>> -static void gen3_stop_engine(struct intel_engine_cs *engine)
>>>>> -{
>>>>> -     struct intel_uncore *uncore = engine->uncore;
>>>>> -     const u32 base = engine->mmio_base;
>>>>> -
>>>>> -     GEM_TRACE("%s\n", engine->name);
>>>>> -
>>>>> -     if (intel_engine_stop_cs(engine))
>>>>> -             GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
>>>>> -
>>>>> -     intel_uncore_write_fw(uncore,
>>>>> -                           RING_HEAD(base),
>>>>> -                           intel_uncore_read_fw(uncore, RING_TAIL(base)));
>>>>> -     intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
>>>>> -
>>>>> -     intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
>>>>> -     intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
>>>>> -     intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
>>>>> -
>>>>> -     /* The ring must be empty before it is disabled */
>>>>> -     intel_uncore_write_fw(uncore, RING_CTL(base), 0);
>>>>> -
>>>>> -     /* Check acts as a post */
>>>>> -     if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
>>>>> -             GEM_TRACE("%s: ring head [%x] not parked\n",
>>>>> -                       engine->name,
>>>>> -                       intel_uncore_read_fw(uncore, RING_HEAD(base)));
>>>>> -}
>>>>> -
>>>>> -static void stop_engines(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>>>>> -{
>>>>> -     struct intel_engine_cs *engine;
>>>>> -     intel_engine_mask_t tmp;
>>>>> -
>>>>> -     if (INTEL_GEN(gt->i915) < 3)
>>>>> -             return;
>>>>> -
>>>>> -     for_each_engine_masked(engine, gt->i915, engine_mask, tmp)
>>>>> -             gen3_stop_engine(engine);
>>>>> -}
>>>>> -
>>>>>     static bool i915_in_reset(struct pci_dev *pdev)
>>>>>     {
>>>>>         u8 gdrst;
>>>>> @@ -607,23 +566,6 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
>>>>>          */
>>>>>         intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
>>>>>         for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
>>>>> -             /*
>>>>> -              * We stop engines, otherwise we might get failed reset and a
>>>>> -              * dead gpu (on elk). Also as modern gpu as kbl can suffer
>>>>> -              * from system hang if batchbuffer is progressing when
>>>>> -              * the reset is issued, regardless of READY_TO_RESET ack.
>>>>> -              * Thus assume it is best to stop engines on all gens
>>>>> -              * where we have a gpu reset.
>>>>> -              *
>>>>> -              * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
>>>>> -              *
>>>>> -              * WaMediaResetMainRingCleanup:ctg,elk (presumably)
>>>>> -              *
>>>>> -              * FIXME: Wa for more modern gens needs to be validated
>>>>> -              */
>>>>> -             if (retry)
>>>>> -                     stop_engines(gt, engine_mask);
>>>>> -
>>>>
>>>> Only other functional change I see is that we stop retrying to stop the
>>>> engines before reset attempts. I don't know if that is a concern or not.
>>>
>>> Ah, but we do stop the engine before resets in *reset_prepare. The other
>>> path to arrive is in sanitize where we don't know enough state to safely
>>> tweak the engines. For those, I claim it shouldn't matter as the engines
>>> should be idle and we only need the reset to clear stale context state.
>>
>> Yes I know that we do call stop in prepare, just not on the reset retry
>> path. It's the above loop, if reset was failing and needed retries
>> before we would re-retried stopping engines and now we would not.
> 
> The engines are still stopped. The functional change is to remove the
> dangerous clearing of RING_HEAD/CTL.

Okay for execlists, but for ringbuffer I was simply asking if _one_ of 
the reasons for failed reset could be failure to stop cs. In which case 
removal of stop_engines from the retry loop might be detrimental for 
ringbuffer.

Regards,

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

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:42:15)
> 
> On 17/07/2019 14:30, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-17 14:21:50)
> >>
> >> On 17/07/2019 14:08, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
> >>>>
> >>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>> -             if (retry)
> >>>>> -                     stop_engines(gt, engine_mask);
> >>>>> -
> >>>>
> >>>> Only other functional change I see is that we stop retrying to stop the
> >>>> engines before reset attempts. I don't know if that is a concern or not.
> >>>
> >>> Ah, but we do stop the engine before resets in *reset_prepare. The other
> >>> path to arrive is in sanitize where we don't know enough state to safely
> >>> tweak the engines. For those, I claim it shouldn't matter as the engines
> >>> should be idle and we only need the reset to clear stale context state.
> >>
> >> Yes I know that we do call stop in prepare, just not on the reset retry
> >> path. It's the above loop, if reset was failing and needed retries
> >> before we would re-retried stopping engines and now we would not.
> > 
> > The engines are still stopped. The functional change is to remove the
> > dangerous clearing of RING_HEAD/CTL.
> 
> Okay for execlists, but for ringbuffer I was simply asking if _one_ of 
> the reasons for failed reset could be failure to stop cs. In which case 
> removal of stop_engines from the retry loop might be detrimental for 
> ringbuffer.

For ringbuffer, we do the full shebang in prepare_reset, with a nice
splat if we fail to clear the head. iirc, that has never been an issue,
although one should always reserve judgment for g4x to randomly fail
with head updates. If it helps, we can remove the loop here as I don't
think it accomplishes anything -- the examples I have where it times out
are followed by a hard machine hang.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/gt: Push engine stopping into reset-prepare

[Tvrtko Ursulin]

On 17/07/2019 14:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:42:15)
>>
>> On 17/07/2019 14:30, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:21:50)
>>>>
>>>> On 17/07/2019 14:08, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:04:34)
>>>>>>
>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>> -             if (retry)
>>>>>>> -                     stop_engines(gt, engine_mask);
>>>>>>> -
>>>>>>
>>>>>> Only other functional change I see is that we stop retrying to stop the
>>>>>> engines before reset attempts. I don't know if that is a concern or not.
>>>>>
>>>>> Ah, but we do stop the engine before resets in *reset_prepare. The other
>>>>> path to arrive is in sanitize where we don't know enough state to safely
>>>>> tweak the engines. For those, I claim it shouldn't matter as the engines
>>>>> should be idle and we only need the reset to clear stale context state.
>>>>
>>>> Yes I know that we do call stop in prepare, just not on the reset retry
>>>> path. It's the above loop, if reset was failing and needed retries
>>>> before we would re-retried stopping engines and now we would not.
>>>
>>> The engines are still stopped. The functional change is to remove the
>>> dangerous clearing of RING_HEAD/CTL.
>>
>> Okay for execlists, but for ringbuffer I was simply asking if _one_ of
>> the reasons for failed reset could be failure to stop cs. In which case
>> removal of stop_engines from the retry loop might be detrimental for
>> ringbuffer.
> 
> For ringbuffer, we do the full shebang in prepare_reset, with a nice
> splat if we fail to clear the head. iirc, that has never been an issue,
> although one should always reserve judgment for g4x to randomly fail
> with head updates. If it helps, we can remove the loop here as I don't
> think it accomplishes anything -- the examples I have where it times out
> are followed by a hard machine hang.

No it's fine if you say it was never the issue. I just wanted some 
reassurances on the particular point.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

[PATCH 3/5] drm/i915/execlists: Process interrupted context on reset

[Chris Wilson]

By stopping the rings, we may trigger an arbitration point resulting in
a premature context-switch (i.e. a completion event before the request
is actually complete). This clears the active context before the reset,
but we must remember to rewind the incomplete context for replay upon
resume.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 9b87a2fc186c..7570a9256001 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1419,7 +1419,8 @@ static void process_csb(struct intel_engine_cs *engine)
 			 * coherent (visible from the CPU) before the
 			 * user interrupt and CSB is processed.
 			 */
-			GEM_BUG_ON(!i915_request_completed(*execlists->active));
+			GEM_BUG_ON(!i915_request_completed(*execlists->active) &&
+				   !reset_in_progress(execlists));
 			execlists_schedule_out(*execlists->active++);
 
 			GEM_BUG_ON(execlists->active - execlists->inflight >
@@ -2254,7 +2255,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	 */
 	rq = execlists_active(execlists);
 	if (!rq)
-		return;
+		goto unwind;
 
 	ce = rq->hw_context;
 	GEM_BUG_ON(i915_active_is_idle(&ce->active));
@@ -2331,6 +2332,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	intel_ring_update_space(ce->ring);
 	__execlists_update_reg_state(ce, engine);
 
+unwind:
 	/* Push back any incomplete requests for replay after the reset. */
 	__unwind_incomplete_requests(engine);
 }
-- 
2.22.0

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

Re: [PATCH 3/5] drm/i915/execlists: Process interrupted context on reset

[Tvrtko Ursulin]

On 16/07/2019 13:49, Chris Wilson wrote:
> By stopping the rings, we may trigger an arbitration point resulting in
> a premature context-switch (i.e. a completion event before the request
> is actually complete). This clears the active context before the reset,
> but we must remember to rewind the incomplete context for replay upon
> resume.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 9b87a2fc186c..7570a9256001 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1419,7 +1419,8 @@ static void process_csb(struct intel_engine_cs *engine)
>   			 * coherent (visible from the CPU) before the
>   			 * user interrupt and CSB is processed.
>   			 */
> -			GEM_BUG_ON(!i915_request_completed(*execlists->active));
> +			GEM_BUG_ON(!i915_request_completed(*execlists->active) &&
> +				   !reset_in_progress(execlists));
>   			execlists_schedule_out(*execlists->active++);
>   
>   			GEM_BUG_ON(execlists->active - execlists->inflight >
> @@ -2254,7 +2255,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
>   	 */
>   	rq = execlists_active(execlists);
>   	if (!rq)
> -		return;
> +		goto unwind;
>   
>   	ce = rq->hw_context;
>   	GEM_BUG_ON(i915_active_is_idle(&ce->active));
> @@ -2331,6 +2332,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
>   	intel_ring_update_space(ce->ring);
>   	__execlists_update_reg_state(ce, engine);
>   
> +unwind:
>   	/* Push back any incomplete requests for replay after the reset. */
>   	__unwind_incomplete_requests(engine);
>   }
> 

Sounds plausible.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Shouldn't there be a Fixes: tag to go with it?

Regards,

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

Re: [PATCH 3/5] drm/i915/execlists: Process interrupted context on reset

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:31:00)
> 
> On 16/07/2019 13:49, Chris Wilson wrote:
> > By stopping the rings, we may trigger an arbitration point resulting in
> > a premature context-switch (i.e. a completion event before the request
> > is actually complete). This clears the active context before the reset,
> > but we must remember to rewind the incomplete context for replay upon
> > resume.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 9b87a2fc186c..7570a9256001 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1419,7 +1419,8 @@ static void process_csb(struct intel_engine_cs *engine)
> >                        * coherent (visible from the CPU) before the
> >                        * user interrupt and CSB is processed.
> >                        */
> > -                     GEM_BUG_ON(!i915_request_completed(*execlists->active));
> > +                     GEM_BUG_ON(!i915_request_completed(*execlists->active) &&
> > +                                !reset_in_progress(execlists));
> >                       execlists_schedule_out(*execlists->active++);
> >   
> >                       GEM_BUG_ON(execlists->active - execlists->inflight >
> > @@ -2254,7 +2255,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
> >        */
> >       rq = execlists_active(execlists);
> >       if (!rq)
> > -             return;
> > +             goto unwind;
> >   
> >       ce = rq->hw_context;
> >       GEM_BUG_ON(i915_active_is_idle(&ce->active));
> > @@ -2331,6 +2332,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
> >       intel_ring_update_space(ce->ring);
> >       __execlists_update_reg_state(ce, engine);
> >   
> > +unwind:
> >       /* Push back any incomplete requests for replay after the reset. */
> >       __unwind_incomplete_requests(engine);
> >   }
> > 
> 
> Sounds plausible.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Shouldn't there be a Fixes: tag to go with it?

Yeah, it's rare even by our standards, I think there's a live_hangcheck
failure about once a month that could be the result of this. However,
the result would be an unrecoverable GPU hang as each attempt at
resetting would not see the missing request and so it would remain
perpetually in the engine->active.list until a set-wedged (i.e. suspend
in the user case).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/execlists: Process interrupted context on reset

[Chris Wilson]

Quoting Chris Wilson (2019-07-17 14:40:26)
> Quoting Tvrtko Ursulin (2019-07-17 14:31:00)
> > 
> > On 16/07/2019 13:49, Chris Wilson wrote:
> > > By stopping the rings, we may trigger an arbitration point resulting in
> > > a premature context-switch (i.e. a completion event before the request
> > > is actually complete). This clears the active context before the reset,
> > > but we must remember to rewind the incomplete context for replay upon
> > > resume.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >   drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > index 9b87a2fc186c..7570a9256001 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > > @@ -1419,7 +1419,8 @@ static void process_csb(struct intel_engine_cs *engine)
> > >                        * coherent (visible from the CPU) before the
> > >                        * user interrupt and CSB is processed.
> > >                        */
> > > -                     GEM_BUG_ON(!i915_request_completed(*execlists->active));
> > > +                     GEM_BUG_ON(!i915_request_completed(*execlists->active) &&
> > > +                                !reset_in_progress(execlists));
> > >                       execlists_schedule_out(*execlists->active++);
> > >   
> > >                       GEM_BUG_ON(execlists->active - execlists->inflight >
> > > @@ -2254,7 +2255,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
> > >        */
> > >       rq = execlists_active(execlists);
> > >       if (!rq)
> > > -             return;
> > > +             goto unwind;
> > >   
> > >       ce = rq->hw_context;
> > >       GEM_BUG_ON(i915_active_is_idle(&ce->active));
> > > @@ -2331,6 +2332,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
> > >       intel_ring_update_space(ce->ring);
> > >       __execlists_update_reg_state(ce, engine);
> > >   
> > > +unwind:
> > >       /* Push back any incomplete requests for replay after the reset. */
> > >       __unwind_incomplete_requests(engine);
> > >   }
> > > 
> > 
> > Sounds plausible.
> > 
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Shouldn't there be a Fixes: tag to go with it?
> 
> Yeah, it's rare even by our standards, I think there's a live_hangcheck
> failure about once a month that could be the result of this. However,
> the result would be an unrecoverable GPU hang as each attempt at
> resetting would not see the missing request and so it would remain
> perpetually in the engine->active.list until a set-wedged (i.e. suspend
> in the user case).

Heh, the commit responsible was one that was itself trying to workaround
the effect of stop_engines() setting RING_HEAD=0 :)

commit 1863e3020ab50bd5f68d85719ba26356cc282643
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Apr 11 14:05:15 2019 +0100

    drm/i915/execlists: Always reset the context's RING registers

    During reset, we try and stop the active ring. This has the consequence
    that we often clobber the RING registers within the context image. When
    we find an active request, we update the context image to rerun that
    request (if it was guilty, we replace the hanging user payload with
    NOPs). However, we were ignoring an active context if the request had
    completed, with the consequence that the next submission on that request
    would start with RING_HEAD==0 and not the tail of the previous request,
    causing all requests still in the ring to be rerun. Rare, but
    occasionally seen within CI where we would spot that the context seqno
    would reverse and complain that we were retiring an incomplete request.

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

[PATCH 4/5] drm/i915/execlists: Cancel breadcrumb on preempting the virtual engine

[Chris Wilson]

As we unwind the requests for a preemption event, we return a virtual
request back to its original virtual engine (so that it is available for
execution on any of its siblings). In the process, this means that its
breadcrumb should no longer be associated with the original physical
engine, and so we are forced to decouple it. Previously, as the request
could not complete without our awareness, we would move it to the next
real engine without any danger. However, preempt-to-busy allowed for
requests to continue on the HW and complete in the background as we
unwound, which meant that we could end up retiring the request before
fixing up the breadcrumb link.

[51679.517943] INFO: trying to register non-static key.
[51679.517956] the code is fine but needs lockdep annotation.
[51679.517960] turning off the locking correctness validator.
[51679.517966] CPU: 0 PID: 3270 Comm: kworker/u8:0 Tainted: G     U            5.2.0+ #717
[51679.517971] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017
[51679.518012] Workqueue: i915 retire_work_handler [i915]
[51679.518017] Call Trace:
[51679.518026]  dump_stack+0x67/0x90
[51679.518031]  register_lock_class+0x52c/0x540
[51679.518038]  ? find_held_lock+0x2d/0x90
[51679.518042]  __lock_acquire+0x68/0x1800
[51679.518047]  ? find_held_lock+0x2d/0x90
[51679.518073]  ? __i915_sw_fence_complete+0xff/0x1c0 [i915]
[51679.518079]  lock_acquire+0x90/0x170
[51679.518105]  ? i915_request_cancel_breadcrumb+0x29/0x160 [i915]
[51679.518112]  _raw_spin_lock+0x27/0x40
[51679.518138]  ? i915_request_cancel_breadcrumb+0x29/0x160 [i915]
[51679.518165]  i915_request_cancel_breadcrumb+0x29/0x160 [i915]
[51679.518199]  i915_request_retire+0x43f/0x530 [i915]
[51679.518232]  retire_requests+0x4d/0x60 [i915]
[51679.518263]  i915_retire_requests+0xdf/0x1f0 [i915]
[51679.518294]  retire_work_handler+0x4c/0x60 [i915]
[51679.518301]  process_one_work+0x22c/0x5c0
[51679.518307]  worker_thread+0x37/0x390
[51679.518311]  ? process_one_work+0x5c0/0x5c0
[51679.518316]  kthread+0x116/0x130
[51679.518320]  ? kthread_create_on_node+0x40/0x40
[51679.518325]  ret_from_fork+0x24/0x30
[51679.520177] ------------[ cut here ]------------
[51679.520189] list_del corruption, ffff88883675e2f0->next is LIST_POISON1 (dead000000000100)

Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 7570a9256001..2ae6695f342b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -492,6 +492,19 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
 			list_move(&rq->sched.link, pl);
 			active = rq;
 		} else {
+			/*
+			 * Decouple the virtual breadcrumb before moving it
+			 * back to the virtual engine -- we don't want the
+			 * request to complete in the background and try
+			 * and cancel the breadcrumb on the virtual engine
+			 * (instead of the old engine where it is linked)!
+			 */
+			if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+				     &rq->fence.flags)) {
+				spin_lock(&rq->lock);
+				i915_request_cancel_breadcrumb(rq);
+				spin_unlock(&rq->lock);
+			}
 			rq->engine = owner;
 			owner->submit_request(rq);
 			active = NULL;
-- 
2.22.0

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

Re: [PATCH 4/5] drm/i915/execlists: Cancel breadcrumb on preempting the virtual engine

[Tvrtko Ursulin]

On 16/07/2019 13:49, Chris Wilson wrote:
> As we unwind the requests for a preemption event, we return a virtual
> request back to its original virtual engine (so that it is available for
> execution on any of its siblings). In the process, this means that its
> breadcrumb should no longer be associated with the original physical
> engine, and so we are forced to decouple it. Previously, as the request
> could not complete without our awareness, we would move it to the next
> real engine without any danger. However, preempt-to-busy allowed for
> requests to continue on the HW and complete in the background as we
> unwound, which meant that we could end up retiring the request before
> fixing up the breadcrumb link.
> 
> [51679.517943] INFO: trying to register non-static key.
> [51679.517956] the code is fine but needs lockdep annotation.
> [51679.517960] turning off the locking correctness validator.
> [51679.517966] CPU: 0 PID: 3270 Comm: kworker/u8:0 Tainted: G     U            5.2.0+ #717
> [51679.517971] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017
> [51679.518012] Workqueue: i915 retire_work_handler [i915]
> [51679.518017] Call Trace:
> [51679.518026]  dump_stack+0x67/0x90
> [51679.518031]  register_lock_class+0x52c/0x540
> [51679.518038]  ? find_held_lock+0x2d/0x90
> [51679.518042]  __lock_acquire+0x68/0x1800
> [51679.518047]  ? find_held_lock+0x2d/0x90
> [51679.518073]  ? __i915_sw_fence_complete+0xff/0x1c0 [i915]
> [51679.518079]  lock_acquire+0x90/0x170
> [51679.518105]  ? i915_request_cancel_breadcrumb+0x29/0x160 [i915]
> [51679.518112]  _raw_spin_lock+0x27/0x40
> [51679.518138]  ? i915_request_cancel_breadcrumb+0x29/0x160 [i915]
> [51679.518165]  i915_request_cancel_breadcrumb+0x29/0x160 [i915]
> [51679.518199]  i915_request_retire+0x43f/0x530 [i915]
> [51679.518232]  retire_requests+0x4d/0x60 [i915]
> [51679.518263]  i915_retire_requests+0xdf/0x1f0 [i915]
> [51679.518294]  retire_work_handler+0x4c/0x60 [i915]
> [51679.518301]  process_one_work+0x22c/0x5c0
> [51679.518307]  worker_thread+0x37/0x390
> [51679.518311]  ? process_one_work+0x5c0/0x5c0
> [51679.518316]  kthread+0x116/0x130
> [51679.518320]  ? kthread_create_on_node+0x40/0x40
> [51679.518325]  ret_from_fork+0x24/0x30
> [51679.520177] ------------[ cut here ]------------
> [51679.520189] list_del corruption, ffff88883675e2f0->next is LIST_POISON1 (dead000000000100)
> 
> Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 7570a9256001..2ae6695f342b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -492,6 +492,19 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
>   			list_move(&rq->sched.link, pl);
>   			active = rq;
>   		} else {
> +			/*
> +			 * Decouple the virtual breadcrumb before moving it
> +			 * back to the virtual engine -- we don't want the
> +			 * request to complete in the background and try
> +			 * and cancel the breadcrumb on the virtual engine
> +			 * (instead of the old engine where it is linked)!
> +			 */
> +			if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> +				     &rq->fence.flags)) {
> +				spin_lock(&rq->lock);
> +				i915_request_cancel_breadcrumb(rq);
> +				spin_unlock(&rq->lock);
> +			}
>   			rq->engine = owner;
>   			owner->submit_request(rq);
>   			active = NULL;
> 

Got lost in the maze of DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT & 
I915_FENCE_FLAG_SIGNAL flags but eventually it looked okay. Should get 
re-added to correct engine's breadcrumb list on submit right?

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 4/5] drm/i915/execlists: Cancel breadcrumb on preempting the virtual engine

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:40:24)
> 
> On 16/07/2019 13:49, Chris Wilson wrote:
> > As we unwind the requests for a preemption event, we return a virtual
> > request back to its original virtual engine (so that it is available for
> > execution on any of its siblings). In the process, this means that its
> > breadcrumb should no longer be associated with the original physical
> > engine, and so we are forced to decouple it. Previously, as the request
> > could not complete without our awareness, we would move it to the next
> > real engine without any danger. However, preempt-to-busy allowed for
> > requests to continue on the HW and complete in the background as we
> > unwound, which meant that we could end up retiring the request before
> > fixing up the breadcrumb link.
> > 
> > [51679.517943] INFO: trying to register non-static key.
> > [51679.517956] the code is fine but needs lockdep annotation.
> > [51679.517960] turning off the locking correctness validator.
> > [51679.517966] CPU: 0 PID: 3270 Comm: kworker/u8:0 Tainted: G     U            5.2.0+ #717
> > [51679.517971] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017
> > [51679.518012] Workqueue: i915 retire_work_handler [i915]
> > [51679.518017] Call Trace:
> > [51679.518026]  dump_stack+0x67/0x90
> > [51679.518031]  register_lock_class+0x52c/0x540
> > [51679.518038]  ? find_held_lock+0x2d/0x90
> > [51679.518042]  __lock_acquire+0x68/0x1800
> > [51679.518047]  ? find_held_lock+0x2d/0x90
> > [51679.518073]  ? __i915_sw_fence_complete+0xff/0x1c0 [i915]
> > [51679.518079]  lock_acquire+0x90/0x170
> > [51679.518105]  ? i915_request_cancel_breadcrumb+0x29/0x160 [i915]
> > [51679.518112]  _raw_spin_lock+0x27/0x40
> > [51679.518138]  ? i915_request_cancel_breadcrumb+0x29/0x160 [i915]
> > [51679.518165]  i915_request_cancel_breadcrumb+0x29/0x160 [i915]
> > [51679.518199]  i915_request_retire+0x43f/0x530 [i915]
> > [51679.518232]  retire_requests+0x4d/0x60 [i915]
> > [51679.518263]  i915_retire_requests+0xdf/0x1f0 [i915]
> > [51679.518294]  retire_work_handler+0x4c/0x60 [i915]
> > [51679.518301]  process_one_work+0x22c/0x5c0
> > [51679.518307]  worker_thread+0x37/0x390
> > [51679.518311]  ? process_one_work+0x5c0/0x5c0
> > [51679.518316]  kthread+0x116/0x130
> > [51679.518320]  ? kthread_create_on_node+0x40/0x40
> > [51679.518325]  ret_from_fork+0x24/0x30
> > [51679.520177] ------------[ cut here ]------------
> > [51679.520189] list_del corruption, ffff88883675e2f0->next is LIST_POISON1 (dead000000000100)
> > 
> > Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 7570a9256001..2ae6695f342b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -492,6 +492,19 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> >                       list_move(&rq->sched.link, pl);
> >                       active = rq;
> >               } else {
> > +                     /*
> > +                      * Decouple the virtual breadcrumb before moving it
> > +                      * back to the virtual engine -- we don't want the
> > +                      * request to complete in the background and try
> > +                      * and cancel the breadcrumb on the virtual engine
> > +                      * (instead of the old engine where it is linked)!
> > +                      */
> > +                     if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> > +                                  &rq->fence.flags)) {
> > +                             spin_lock(&rq->lock);
> > +                             i915_request_cancel_breadcrumb(rq);
> > +                             spin_unlock(&rq->lock);
> > +                     }
> >                       rq->engine = owner;
> >                       owner->submit_request(rq);
> >                       active = NULL;
> > 
> 
> Got lost in the maze of DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT & 
> I915_FENCE_FLAG_SIGNAL flags but eventually it looked okay. Should get 
> re-added to correct engine's breadcrumb list on submit right?

Yes. It will also survive if the request completes in the background.

It's a bit ugly, but will do -- we could probably hide the detail in a
local virtual_unwind_request(). Hmm...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Hide unshrinkable context objects from the shrinker

[Chris Wilson]

The shrinker cannot touch objects used by the contexts (logical state
and ring). Currently we mark those as "pin_global" to let the shrinker
skip over them, however, if we remove them from the shrinker lists
entirely, we don't event have to include them in our shrink accounting.

By keeping the unshrinkable objects in our shrinker tracking, we report
a large number of objects available to be shrunk, and leave the shrinker
deeply unsatisfied when we fail to reclaim those. The shrinker will
persist in trying to reclaim the unavailable objects, forcing the system
into a livelock (not even hitting the dread oomkiller).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c   | 11 ++--
 drivers/gpu/drm/i915/gem/i915_gem_object.h   |  4 ++
 drivers/gpu/drm/i915/gem/i915_gem_pages.c    | 13 +----
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 57 ++++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_context.c      |  4 +-
 drivers/gpu/drm/i915/gt/intel_ringbuffer.c   | 17 +++---
 drivers/gpu/drm/i915/i915_debugfs.c          |  3 +-
 drivers/gpu/drm/i915/i915_vma.c              | 15 ++++++
 drivers/gpu/drm/i915/i915_vma.h              |  4 ++
 9 files changed, 97 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index d5197a2a106f..4ea97fca9c35 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -63,6 +63,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 	spin_lock_init(&obj->vma.lock);
 	INIT_LIST_HEAD(&obj->vma.list);
 
+	INIT_LIST_HEAD(&obj->mm.link);
+
 	INIT_LIST_HEAD(&obj->lut_list);
 	INIT_LIST_HEAD(&obj->batch_pool_link);
 
@@ -273,14 +275,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	 * or else we may oom whilst there are plenty of deferred
 	 * freed objects.
 	 */
-	if (i915_gem_object_has_pages(obj) &&
-	    i915_gem_object_is_shrinkable(obj)) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&i915->mm.obj_lock, flags);
-		list_del_init(&obj->mm.link);
-		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
-	}
+	i915_gem_object_make_unshrinkable(obj);
 
 	/*
 	 * Since we require blocking on struct_mutex to unbind the freed
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 67aea07ea019..3714cf234d64 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -394,6 +394,10 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 				     unsigned int flags);
 void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
 
+void i915_gem_object_make_unshrinkable(struct drm_i915_gem_object *obj);
+void i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj);
+void i915_gem_object_make_purgeable(struct drm_i915_gem_object *obj);
+
 static inline bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
 {
 	if (obj->cache_dirty)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index b36ad269f4ea..92ad3cc220e3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -153,24 +153,13 @@ static void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj)
 struct sg_table *
 __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 {
-	struct drm_i915_private *i915 = to_i915(obj->base.dev);
 	struct sg_table *pages;
 
 	pages = fetch_and_zero(&obj->mm.pages);
 	if (IS_ERR_OR_NULL(pages))
 		return pages;
 
-	if (i915_gem_object_is_shrinkable(obj)) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&i915->mm.obj_lock, flags);
-
-		list_del(&obj->mm.link);
-		i915->mm.shrink_count--;
-		i915->mm.shrink_memory -= obj->base.size;
-
-		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
-	}
+	i915_gem_object_make_unshrinkable(obj);
 
 	if (obj->mm.mapping) {
 		void *ptr;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index 3f4c6bdcc3c3..14abfd77365f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -530,3 +530,60 @@ void i915_gem_shrinker_taints_mutex(struct drm_i915_private *i915,
 	if (unlock)
 		mutex_release(&i915->drm.struct_mutex.dep_map, 0, _RET_IP_);
 }
+
+void i915_gem_object_make_unshrinkable(struct drm_i915_gem_object *obj)
+{
+	if (!list_empty(&obj->mm.link)) {
+		struct drm_i915_private *i915 = to_i915(obj->base.dev);
+		unsigned long flags;
+
+		spin_lock_irqsave(&i915->mm.obj_lock, flags);
+		GEM_BUG_ON(list_empty(&obj->mm.link));
+
+		list_del_init(&obj->mm.link);
+		i915->mm.shrink_count--;
+		i915->mm.shrink_memory -= obj->base.size;
+
+		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
+	}
+}
+
+void i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj)
+{
+	GEM_BUG_ON(!i915_gem_object_has_pages(obj));
+	GEM_BUG_ON(!list_empty(&obj->mm.link));
+
+	if (i915_gem_object_is_shrinkable(obj)) {
+		struct drm_i915_private *i915 = to_i915(obj->base.dev);
+		unsigned long flags;
+
+		spin_lock_irqsave(&i915->mm.obj_lock, flags);
+		GEM_BUG_ON(!kref_read(&obj->base.refcount));
+
+		list_add_tail(&obj->mm.link, &i915->mm.shrink_list);
+		i915->mm.shrink_count++;
+		i915->mm.shrink_memory += obj->base.size;
+
+		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
+	}
+}
+
+void i915_gem_object_make_purgeable(struct drm_i915_gem_object *obj)
+{
+	GEM_BUG_ON(!i915_gem_object_has_pages(obj));
+	GEM_BUG_ON(!list_empty(&obj->mm.link));
+
+	if (i915_gem_object_is_shrinkable(obj)) {
+		struct drm_i915_private *i915 = to_i915(obj->base.dev);
+		unsigned long flags;
+
+		spin_lock_irqsave(&i915->mm.obj_lock, flags);
+		GEM_BUG_ON(!kref_read(&obj->base.refcount));
+
+		list_add_tail(&obj->mm.link, &i915->mm.purge_list);
+		i915->mm.shrink_count++;
+		i915->mm.shrink_memory += obj->base.size;
+
+		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
+	}
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 1110fc8f657a..f012ec898aba 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -118,7 +118,7 @@ static int __context_pin_state(struct i915_vma *vma)
 	 * And mark it as a globally pinned object to let the shrinker know
 	 * it cannot reclaim the object until we release it.
 	 */
-	vma->obj->pin_global++;
+	i915_vma_make_unshrinkable(vma);
 	vma->obj->mm.dirty = true;
 
 	return 0;
@@ -126,8 +126,8 @@ static int __context_pin_state(struct i915_vma *vma)
 
 static void __context_unpin_state(struct i915_vma *vma)
 {
-	vma->obj->pin_global--;
 	__i915_vma_unpin(vma);
+	i915_vma_make_shrinkable(vma);
 }
 
 static void __intel_context_retire(struct i915_active *active)
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index 213df144be15..7bb0b971aaea 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1238,7 +1238,7 @@ int intel_ring_pin(struct intel_ring *ring)
 		goto err_ring;
 	}
 
-	vma->obj->pin_global++;
+	i915_vma_make_unshrinkable(vma);
 
 	GEM_BUG_ON(ring->vaddr);
 	ring->vaddr = addr;
@@ -1267,6 +1267,8 @@ void intel_ring_reset(struct intel_ring *ring, u32 tail)
 
 void intel_ring_unpin(struct intel_ring *ring)
 {
+	struct i915_vma *vma = ring->vma;
+
 	if (!atomic_dec_and_test(&ring->pin_count))
 		return;
 
@@ -1275,18 +1277,17 @@ void intel_ring_unpin(struct intel_ring *ring)
 	/* Discard any unused bytes beyond that submitted to hw. */
 	intel_ring_reset(ring, ring->tail);
 
-	GEM_BUG_ON(!ring->vma);
-	i915_vma_unset_ggtt_write(ring->vma);
-	if (i915_vma_is_map_and_fenceable(ring->vma))
-		i915_vma_unpin_iomap(ring->vma);
+	i915_vma_unset_ggtt_write(vma);
+	if (i915_vma_is_map_and_fenceable(vma))
+		i915_vma_unpin_iomap(vma);
 	else
-		i915_gem_object_unpin_map(ring->vma->obj);
+		i915_gem_object_unpin_map(vma->obj);
 
 	GEM_BUG_ON(!ring->vaddr);
 	ring->vaddr = NULL;
 
-	ring->vma->obj->pin_global--;
-	i915_vma_unpin(ring->vma);
+	i915_vma_unpin(vma);
+	i915_vma_make_purgeable(vma);
 
 	intel_timeline_unpin(ring->timeline);
 }
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6b84d04a6a28..c43f270085f5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -363,8 +363,9 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
 	struct drm_i915_private *i915 = node_to_i915(m->private);
 	int ret;
 
-	seq_printf(m, "%u shrinkable objects, %llu bytes\n",
+	seq_printf(m, "%u shrinkable [%u free] objects, %llu bytes\n",
 		   i915->mm.shrink_count,
+		   atomic_read(&i915->mm.free_count),
 		   i915->mm.shrink_memory);
 
 	seq_putc(m, '\n');
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index ee73baf29415..1d003ec9da19 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1030,6 +1030,21 @@ int i915_vma_unbind(struct i915_vma *vma)
 	return 0;
 }
 
+void i915_vma_make_unshrinkable(struct i915_vma *vma)
+{
+	i915_gem_object_make_unshrinkable(vma->obj);
+}
+
+void i915_vma_make_shrinkable(struct i915_vma *vma)
+{
+	i915_gem_object_make_shrinkable(vma->obj);
+}
+
+void i915_vma_make_purgeable(struct i915_vma *vma)
+{
+	i915_gem_object_make_purgeable(vma->obj);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/i915_vma.c"
 #endif
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 4b769db649bf..a24bd6787ef7 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -459,4 +459,8 @@ void i915_vma_parked(struct drm_i915_private *i915);
 struct i915_vma *i915_vma_alloc(void);
 void i915_vma_free(struct i915_vma *vma);
 
+void i915_vma_make_unshrinkable(struct i915_vma *vma);
+void i915_vma_make_shrinkable(struct i915_vma *vma);
+void i915_vma_make_purgeable(struct i915_vma *vma);
+
 #endif
-- 
2.22.0

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

✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915/userptr: Beware recursive lock_page()

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/userptr: Beware recursive lock_page()
URL   : https://patchwork.freedesktop.org/series/63752/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6492 -> Patchwork_13664
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][1] -> [FAIL][2] ([fdo#109485])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][3] -> [DMESG-WARN][4] ([fdo#102614])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u2:          [INCOMPLETE][5] ([fdo#107713] / [fdo#109100]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/fi-icl-u2/igt@gem_ctx_create@basic-files.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/fi-icl-u2/igt@gem_ctx_create@basic-files.html

  * igt@i915_module_load@reload-no-display:
    - {fi-icl-u4}:        [DMESG-WARN][7] ([fdo#105602]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/fi-icl-u4/igt@i915_module_load@reload-no-display.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/fi-icl-u4/igt@i915_module_load@reload-no-display.html

  
  {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#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111046 ]: https://bugs.freedesktop.org/show_bug.cgi?id=111046 


Participating hosts (55 -> 47)
------------------------------

  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6492 -> Patchwork_13664

  CI_DRM_6492: cb320040f8fea17bf02644f3536ebb34cf9cb5e1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5100: 0ea68a1efbfcc4961f2f816ab59e4ad8136c6250 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13664: a6bfd92acc43666e1f9cdbc5c5ac27eeb310a717 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a6bfd92acc43 drm/i915: Hide unshrinkable context objects from the shrinker
53455fa1905e drm/i915/execlists: Cancel breadcrumb on preempting the virtual engine
2f6edb4997f0 drm/i915/execlists: Process interrupted context on reset
f2e625797017 drm/i915/gt: Push engine stopping into reset-prepare
578455bedd5b drm/i915/userptr: Beware recursive lock_page()

== Logs ==

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

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 16/07/2019 13:49, Chris Wilson wrote:
> Following a try_to_unmap() we may want to remove the userptr and so call
> put_pages(). However, try_to_unmap() acquires the page lock and so we
> must avoid recursively locking the pages ourselves -- which means that
> we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.

So if trylock randomly fail we get data corruption in whatever data set 
application is working on, which is what the original patch was trying 
to avoid? Are we able to detect the backing store type so at least we 
don't risk skipping set_page_dirty with anonymous/shmemfs?

Regards,

Tvrtko

> Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index b9d2bb15e4a6..1ad2047a6dbd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   		obj->mm.dirty = false;
>   
>   	for_each_sgt_page(page, sgt_iter, pages) {
> -		if (obj->mm.dirty)
> +		if (obj->mm.dirty && trylock_page(page)) {
>   			/*
>   			 * As this may not be anonymous memory (e.g. shmem)
>   			 * but exist on a real mapping, we have to lock
> @@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   			 * the page reference is not sufficient to
>   			 * prevent the inode from being truncated.
>   			 * Play safe and take the lock.
> +			 *
> +			 * However...!
> +			 *
> +			 * The mmu-notifier can be invalidated for a
> +			 * migrate_page, that is alreadying holding the lock
> +			 * on the page. Such a try_to_unmap() will result
> +			 * in us calling put_pages() and so recursively try
> +			 * to lock the page. We avoid that deadlock with
> +			 * a trylock_page() and in exchange we risk missing
> +			 * some page dirtying.
>   			 */
> -			set_page_dirty_lock(page);
> +			set_page_dirty(page);
> +			unlock_page(page);
> +		}
>   
>   		mark_page_accessed(page);
>   		put_page(page);
> 

Re: [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 16/07/2019 13:49, Chris Wilson wrote:
> Following a try_to_unmap() we may want to remove the userptr and so call
> put_pages(). However, try_to_unmap() acquires the page lock and so we
> must avoid recursively locking the pages ourselves -- which means that
> we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.

So if trylock randomly fail we get data corruption in whatever data set 
application is working on, which is what the original patch was trying 
to avoid? Are we able to detect the backing store type so at least we 
don't risk skipping set_page_dirty with anonymous/shmemfs?

Regards,

Tvrtko

> Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index b9d2bb15e4a6..1ad2047a6dbd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   		obj->mm.dirty = false;
>   
>   	for_each_sgt_page(page, sgt_iter, pages) {
> -		if (obj->mm.dirty)
> +		if (obj->mm.dirty && trylock_page(page)) {
>   			/*
>   			 * As this may not be anonymous memory (e.g. shmem)
>   			 * but exist on a real mapping, we have to lock
> @@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   			 * the page reference is not sufficient to
>   			 * prevent the inode from being truncated.
>   			 * Play safe and take the lock.
> +			 *
> +			 * However...!
> +			 *
> +			 * The mmu-notifier can be invalidated for a
> +			 * migrate_page, that is alreadying holding the lock
> +			 * on the page. Such a try_to_unmap() will result
> +			 * in us calling put_pages() and so recursively try
> +			 * to lock the page. We avoid that deadlock with
> +			 * a trylock_page() and in exchange we risk missing
> +			 * some page dirtying.
>   			 */
> -			set_page_dirty_lock(page);
> +			set_page_dirty(page);
> +			unlock_page(page);
> +		}
>   
>   		mark_page_accessed(page);
>   		put_page(page);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> 
> On 16/07/2019 13:49, Chris Wilson wrote:
> > Following a try_to_unmap() we may want to remove the userptr and so call
> > put_pages(). However, try_to_unmap() acquires the page lock and so we
> > must avoid recursively locking the pages ourselves -- which means that
> > we cannot safely acquire the lock around set_page_dirty(). Since we
> > can't be sure of the lock, we have to risk skip dirtying the page, or
> > else risk calling set_page_dirty() without a lock and so risk fs
> > corruption.
> 
> So if trylock randomly fail we get data corruption in whatever data set 
> application is working on, which is what the original patch was trying 
> to avoid? Are we able to detect the backing store type so at least we 
> don't risk skipping set_page_dirty with anonymous/shmemfs?

page->mapping???

We still have the issue that if there is a mapping we should be taking
the lock, and we may have both a mapping and be inside try_to_unmap().

I think it's lose-lose. The only way to win is not to userptr :)
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 16/07/2019 16:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>
>> On 16/07/2019 13:49, Chris Wilson wrote:
>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>> must avoid recursively locking the pages ourselves -- which means that
>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>> else risk calling set_page_dirty() without a lock and so risk fs
>>> corruption.
>>
>> So if trylock randomly fail we get data corruption in whatever data set
>> application is working on, which is what the original patch was trying
>> to avoid? Are we able to detect the backing store type so at least we
>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> 
> page->mapping???

Would page->mapping work? What is it telling us?

> We still have the issue that if there is a mapping we should be taking
> the lock, and we may have both a mapping and be inside try_to_unmap().

Is this a problem? On a path with mappings we trylock and so solve the 
set_dirty_locked and recursive deadlock issues, and with no mappings 
with always dirty the page and avoid data corruption.

> I think it's lose-lose. The only way to win is not to userptr :)

It's looking more and more like this indeed.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> 
> On 16/07/2019 16:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>
> >> On 16/07/2019 13:49, Chris Wilson wrote:
> >>> Following a try_to_unmap() we may want to remove the userptr and so call
> >>> put_pages(). However, try_to_unmap() acquires the page lock and so we
> >>> must avoid recursively locking the pages ourselves -- which means that
> >>> we cannot safely acquire the lock around set_page_dirty(). Since we
> >>> can't be sure of the lock, we have to risk skip dirtying the page, or
> >>> else risk calling set_page_dirty() without a lock and so risk fs
> >>> corruption.
> >>
> >> So if trylock randomly fail we get data corruption in whatever data set
> >> application is working on, which is what the original patch was trying
> >> to avoid? Are we able to detect the backing store type so at least we
> >> don't risk skipping set_page_dirty with anonymous/shmemfs?
> > 
> > page->mapping???
> 
> Would page->mapping work? What is it telling us?

It basically tells us if there is a fs around; anything that is the most
basic of malloc (even tmpfs/shmemfs has page->mapping).
 
> > We still have the issue that if there is a mapping we should be taking
> > the lock, and we may have both a mapping and be inside try_to_unmap().
> 
> Is this a problem? On a path with mappings we trylock and so solve the 
> set_dirty_locked and recursive deadlock issues, and with no mappings 
> with always dirty the page and avoid data corruption.

The problem as I see it is !page->mapping are likely an insignificant
minority of userptr; as I think even memfd are essentially shmemfs (or
hugetlbfs) and so have mappings.
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 17/07/2019 14:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>
>> On 16/07/2019 16:37, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>
>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>>>> must avoid recursively locking the pages ourselves -- which means that
>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>> corruption.
>>>>
>>>> So if trylock randomly fail we get data corruption in whatever data set
>>>> application is working on, which is what the original patch was trying
>>>> to avoid? Are we able to detect the backing store type so at least we
>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>
>>> page->mapping???
>>
>> Would page->mapping work? What is it telling us?
> 
> It basically tells us if there is a fs around; anything that is the most
> basic of malloc (even tmpfs/shmemfs has page->mapping).

Normal malloc so anonymous pages? Or you meant everything _apart_ from 
the most basic malloc?

>>> We still have the issue that if there is a mapping we should be taking
>>> the lock, and we may have both a mapping and be inside try_to_unmap().
>>
>> Is this a problem? On a path with mappings we trylock and so solve the
>> set_dirty_locked and recursive deadlock issues, and with no mappings
>> with always dirty the page and avoid data corruption.
> 
> The problem as I see it is !page->mapping are likely an insignificant
> minority of userptr; as I think even memfd are essentially shmemfs (or
> hugetlbfs) and so have mappings.

Better then nothing, no? If easy to do..

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> 
> On 17/07/2019 14:17, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>
> >> On 16/07/2019 16:37, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>
> >>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>> Following a try_to_unmap() we may want to remove the userptr and so call
> >>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
> >>>>> must avoid recursively locking the pages ourselves -- which means that
> >>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
> >>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
> >>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>> corruption.
> >>>>
> >>>> So if trylock randomly fail we get data corruption in whatever data set
> >>>> application is working on, which is what the original patch was trying
> >>>> to avoid? Are we able to detect the backing store type so at least we
> >>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>
> >>> page->mapping???
> >>
> >> Would page->mapping work? What is it telling us?
> > 
> > It basically tells us if there is a fs around; anything that is the most
> > basic of malloc (even tmpfs/shmemfs has page->mapping).
> 
> Normal malloc so anonymous pages? Or you meant everything _apart_ from 
> the most basic malloc?

Aye missed the not.

> >>> We still have the issue that if there is a mapping we should be taking
> >>> the lock, and we may have both a mapping and be inside try_to_unmap().
> >>
> >> Is this a problem? On a path with mappings we trylock and so solve the
> >> set_dirty_locked and recursive deadlock issues, and with no mappings
> >> with always dirty the page and avoid data corruption.
> > 
> > The problem as I see it is !page->mapping are likely an insignificant
> > minority of userptr; as I think even memfd are essentially shmemfs (or
> > hugetlbfs) and so have mappings.
> 
> Better then nothing, no? If easy to do..

Actually, I erring on the opposite side. Peeking at mm/ internals does
not bode confidence and feels indefensible. I'd much rather throw my
hands up and say "this is the best we can do with the API provided,
please tell us what we should have done." To which the answer is
probably to not have used gup in the first place :|
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 17/07/2019 14:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>
>> On 17/07/2019 14:17, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>
>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>
>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>>>>>> must avoid recursively locking the pages ourselves -- which means that
>>>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>> corruption.
>>>>>>
>>>>>> So if trylock randomly fail we get data corruption in whatever data set
>>>>>> application is working on, which is what the original patch was trying
>>>>>> to avoid? Are we able to detect the backing store type so at least we
>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>
>>>>> page->mapping???
>>>>
>>>> Would page->mapping work? What is it telling us?
>>>
>>> It basically tells us if there is a fs around; anything that is the most
>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>
>> Normal malloc so anonymous pages? Or you meant everything _apart_ from
>> the most basic malloc?
> 
> Aye missed the not.
> 
>>>>> We still have the issue that if there is a mapping we should be taking
>>>>> the lock, and we may have both a mapping and be inside try_to_unmap().
>>>>
>>>> Is this a problem? On a path with mappings we trylock and so solve the
>>>> set_dirty_locked and recursive deadlock issues, and with no mappings
>>>> with always dirty the page and avoid data corruption.
>>>
>>> The problem as I see it is !page->mapping are likely an insignificant
>>> minority of userptr; as I think even memfd are essentially shmemfs (or
>>> hugetlbfs) and so have mappings.
>>
>> Better then nothing, no? If easy to do..
> 
> Actually, I erring on the opposite side. Peeking at mm/ internals does
> not bode confidence and feels indefensible. I'd much rather throw my
> hands up and say "this is the best we can do with the API provided,
> please tell us what we should have done." To which the answer is
> probably to not have used gup in the first place :|

"""
/*
 * set_page_dirty() is racy if the caller has no reference against
 * page->mapping->host, and if the page is unlocked.  This is because another
 * CPU could truncate the page off the mapping and then free the mapping.
 *
 * Usually, the page _is_ locked, or the caller is a user-space process which
 * holds a reference on the inode by having an open file.
 *
 * In other cases, the page should be locked before running set_page_dirty().
 */
int set_page_dirty_lock(struct page *page)
"""

Could we hold a reference to page->mapping->host while having pages and then would be okay to call plain set_page_dirty?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
> 
> On 17/07/2019 14:35, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> >>
> >> On 17/07/2019 14:17, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>>>
> >>>> On 16/07/2019 16:37, Chris Wilson wrote:
> >>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>>>
> >>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
> >>>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
> >>>>>>> must avoid recursively locking the pages ourselves -- which means that
> >>>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
> >>>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
> >>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>>>> corruption.
> >>>>>>
> >>>>>> So if trylock randomly fail we get data corruption in whatever data set
> >>>>>> application is working on, which is what the original patch was trying
> >>>>>> to avoid? Are we able to detect the backing store type so at least we
> >>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>>>
> >>>>> page->mapping???
> >>>>
> >>>> Would page->mapping work? What is it telling us?
> >>>
> >>> It basically tells us if there is a fs around; anything that is the most
> >>> basic of malloc (even tmpfs/shmemfs has page->mapping).
> >>
> >> Normal malloc so anonymous pages? Or you meant everything _apart_ from
> >> the most basic malloc?
> > 
> > Aye missed the not.
> > 
> >>>>> We still have the issue that if there is a mapping we should be taking
> >>>>> the lock, and we may have both a mapping and be inside try_to_unmap().
> >>>>
> >>>> Is this a problem? On a path with mappings we trylock and so solve the
> >>>> set_dirty_locked and recursive deadlock issues, and with no mappings
> >>>> with always dirty the page and avoid data corruption.
> >>>
> >>> The problem as I see it is !page->mapping are likely an insignificant
> >>> minority of userptr; as I think even memfd are essentially shmemfs (or
> >>> hugetlbfs) and so have mappings.
> >>
> >> Better then nothing, no? If easy to do..
> > 
> > Actually, I erring on the opposite side. Peeking at mm/ internals does
> > not bode confidence and feels indefensible. I'd much rather throw my
> > hands up and say "this is the best we can do with the API provided,
> > please tell us what we should have done." To which the answer is
> > probably to not have used gup in the first place :|
> 
> """
> /*
>  * set_page_dirty() is racy if the caller has no reference against
>  * page->mapping->host, and if the page is unlocked.  This is because another
>  * CPU could truncate the page off the mapping and then free the mapping.
>  *
>  * Usually, the page _is_ locked, or the caller is a user-space process which
>  * holds a reference on the inode by having an open file.
>  *
>  * In other cases, the page should be locked before running set_page_dirty().
>  */
> int set_page_dirty_lock(struct page *page)
> """
> 
> Could we hold a reference to page->mapping->host while having pages and then would be okay to call plain set_page_dirty?

We would then be hitting the warnings in ext4 for unlocked pages again.
Essentially the argument is whether or not that warn is valid, to which I
think requires inner knowledge of vfs + ext4. To hold a reference on the
host would require us tracking page->mapping (reasonable since we
already hooked into mmu and so will get an invalidate + fresh gup on
any changes), plus iterating over all to acquire the extra reference if
applicable -- and I have no idea what the side-effects of that would be.
Could well be positive side-effects. Just feels like wandering even
further off the beaten path without a map. Good news hmm is just around
the corner (which will probably prohibit this use-case) :|
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 17/07/2019 15:06, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>
>> On 17/07/2019 14:35, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>
>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>
>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>
>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr and so call
>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock and so we
>>>>>>>>> must avoid recursively locking the pages ourselves -- which means that
>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). Since we
>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the page, or
>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>> corruption.
>>>>>>>>
>>>>>>>> So if trylock randomly fail we get data corruption in whatever data set
>>>>>>>> application is working on, which is what the original patch was trying
>>>>>>>> to avoid? Are we able to detect the backing store type so at least we
>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>
>>>>>>> page->mapping???
>>>>>>
>>>>>> Would page->mapping work? What is it telling us?
>>>>>
>>>>> It basically tells us if there is a fs around; anything that is the most
>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>
>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ from
>>>> the most basic malloc?
>>>
>>> Aye missed the not.
>>>
>>>>>>> We still have the issue that if there is a mapping we should be taking
>>>>>>> the lock, and we may have both a mapping and be inside try_to_unmap().
>>>>>>
>>>>>> Is this a problem? On a path with mappings we trylock and so solve the
>>>>>> set_dirty_locked and recursive deadlock issues, and with no mappings
>>>>>> with always dirty the page and avoid data corruption.
>>>>>
>>>>> The problem as I see it is !page->mapping are likely an insignificant
>>>>> minority of userptr; as I think even memfd are essentially shmemfs (or
>>>>> hugetlbfs) and so have mappings.
>>>>
>>>> Better then nothing, no? If easy to do..
>>>
>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>> not bode confidence and feels indefensible. I'd much rather throw my
>>> hands up and say "this is the best we can do with the API provided,
>>> please tell us what we should have done." To which the answer is
>>> probably to not have used gup in the first place :|
>>
>> """
>> /*
>>   * set_page_dirty() is racy if the caller has no reference against
>>   * page->mapping->host, and if the page is unlocked.  This is because another
>>   * CPU could truncate the page off the mapping and then free the mapping.
>>   *
>>   * Usually, the page _is_ locked, or the caller is a user-space process which
>>   * holds a reference on the inode by having an open file.
>>   *
>>   * In other cases, the page should be locked before running set_page_dirty().
>>   */
>> int set_page_dirty_lock(struct page *page)
>> """
>>
>> Could we hold a reference to page->mapping->host while having pages and then would be okay to call plain set_page_dirty?
> 
> We would then be hitting the warnings in ext4 for unlocked pages again.

Ah true..

> Essentially the argument is whether or not that warn is valid, to which I
> think requires inner knowledge of vfs + ext4. To hold a reference on the
> host would require us tracking page->mapping (reasonable since we
> already hooked into mmu and so will get an invalidate + fresh gup on
> any changes), plus iterating over all to acquire the extra reference if
> applicable -- and I have no idea what the side-effects of that would be.
> Could well be positive side-effects. Just feels like wandering even
> further off the beaten path without a map. Good news hmm is just around
> the corner (which will probably prohibit this use-case) :|

... can we reach out to someone more knowledgeable in mm matters to 
recommend us what to do?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Lionel Landwerlin]

On 17/07/2019 21:09, Tvrtko Ursulin wrote:
>
> On 17/07/2019 15:06, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>>
>>> On 17/07/2019 14:35, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>>
>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>>
>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>>
>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr 
>>>>>>>>>> and so call
>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock 
>>>>>>>>>> and so we
>>>>>>>>>> must avoid recursively locking the pages ourselves -- which 
>>>>>>>>>> means that
>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). 
>>>>>>>>>> Since we
>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the 
>>>>>>>>>> page, or
>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>>> corruption.
>>>>>>>>>
>>>>>>>>> So if trylock randomly fail we get data corruption in whatever 
>>>>>>>>> data set
>>>>>>>>> application is working on, which is what the original patch 
>>>>>>>>> was trying
>>>>>>>>> to avoid? Are we able to detect the backing store type so at 
>>>>>>>>> least we
>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>>
>>>>>>>> page->mapping???
>>>>>>>
>>>>>>> Would page->mapping work? What is it telling us?
>>>>>>
>>>>>> It basically tells us if there is a fs around; anything that is 
>>>>>> the most
>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>>
>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ 
>>>>> from
>>>>> the most basic malloc?
>>>>
>>>> Aye missed the not.
>>>>
>>>>>>>> We still have the issue that if there is a mapping we should be 
>>>>>>>> taking
>>>>>>>> the lock, and we may have both a mapping and be inside 
>>>>>>>> try_to_unmap().
>>>>>>>
>>>>>>> Is this a problem? On a path with mappings we trylock and so 
>>>>>>> solve the
>>>>>>> set_dirty_locked and recursive deadlock issues, and with no 
>>>>>>> mappings
>>>>>>> with always dirty the page and avoid data corruption.
>>>>>>
>>>>>> The problem as I see it is !page->mapping are likely an 
>>>>>> insignificant
>>>>>> minority of userptr; as I think even memfd are essentially 
>>>>>> shmemfs (or
>>>>>> hugetlbfs) and so have mappings.
>>>>>
>>>>> Better then nothing, no? If easy to do..
>>>>
>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>>> not bode confidence and feels indefensible. I'd much rather throw my
>>>> hands up and say "this is the best we can do with the API provided,
>>>> please tell us what we should have done." To which the answer is
>>>> probably to not have used gup in the first place :|
>>>
>>> """
>>> /*
>>>   * set_page_dirty() is racy if the caller has no reference against
>>>   * page->mapping->host, and if the page is unlocked. This is 
>>> because another
>>>   * CPU could truncate the page off the mapping and then free the 
>>> mapping.
>>>   *
>>>   * Usually, the page _is_ locked, or the caller is a user-space 
>>> process which
>>>   * holds a reference on the inode by having an open file.
>>>   *
>>>   * In other cases, the page should be locked before running 
>>> set_page_dirty().
>>>   */
>>> int set_page_dirty_lock(struct page *page)
>>> """
>>>
>>> Could we hold a reference to page->mapping->host while having pages 
>>> and then would be okay to call plain set_page_dirty?
>>
>> We would then be hitting the warnings in ext4 for unlocked pages again.
>
> Ah true..
>
>> Essentially the argument is whether or not that warn is valid, to 
>> which I
>> think requires inner knowledge of vfs + ext4. To hold a reference on the
>> host would require us tracking page->mapping (reasonable since we
>> already hooked into mmu and so will get an invalidate + fresh gup on
>> any changes), plus iterating over all to acquire the extra reference if
>> applicable -- and I have no idea what the side-effects of that would be.
>> Could well be positive side-effects. Just feels like wandering even
>> further off the beaten path without a map. Good news hmm is just around
>> the corner (which will probably prohibit this use-case) :|
>
> ... can we reach out to someone more knowledgeable in mm matters to 
> recommend us what to do?
>
> Regards,
>
> Tvrtko


Just a reminder to not let this slip.
We run into userptr bugs in CI quite regularly.

Thanks,

-Lionel

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Lionel Landwerlin (2019-07-26 14:38:40)
> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
> >
> > On 17/07/2019 15:06, Chris Wilson wrote:
> >> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
> >>>
> >>> On 17/07/2019 14:35, Chris Wilson wrote:
> >>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> >>>>>
> >>>>> On 17/07/2019 14:17, Chris Wilson wrote:
> >>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>>>>>>
> >>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
> >>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>>>>>>
> >>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr 
> >>>>>>>>>> and so call
> >>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock 
> >>>>>>>>>> and so we
> >>>>>>>>>> must avoid recursively locking the pages ourselves -- which 
> >>>>>>>>>> means that
> >>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). 
> >>>>>>>>>> Since we
> >>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the 
> >>>>>>>>>> page, or
> >>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>>>>>>> corruption.
> >>>>>>>>>
> >>>>>>>>> So if trylock randomly fail we get data corruption in whatever 
> >>>>>>>>> data set
> >>>>>>>>> application is working on, which is what the original patch 
> >>>>>>>>> was trying
> >>>>>>>>> to avoid? Are we able to detect the backing store type so at 
> >>>>>>>>> least we
> >>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>>>>>>
> >>>>>>>> page->mapping???
> >>>>>>>
> >>>>>>> Would page->mapping work? What is it telling us?
> >>>>>>
> >>>>>> It basically tells us if there is a fs around; anything that is 
> >>>>>> the most
> >>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
> >>>>>
> >>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ 
> >>>>> from
> >>>>> the most basic malloc?
> >>>>
> >>>> Aye missed the not.
> >>>>
> >>>>>>>> We still have the issue that if there is a mapping we should be 
> >>>>>>>> taking
> >>>>>>>> the lock, and we may have both a mapping and be inside 
> >>>>>>>> try_to_unmap().
> >>>>>>>
> >>>>>>> Is this a problem? On a path with mappings we trylock and so 
> >>>>>>> solve the
> >>>>>>> set_dirty_locked and recursive deadlock issues, and with no 
> >>>>>>> mappings
> >>>>>>> with always dirty the page and avoid data corruption.
> >>>>>>
> >>>>>> The problem as I see it is !page->mapping are likely an 
> >>>>>> insignificant
> >>>>>> minority of userptr; as I think even memfd are essentially 
> >>>>>> shmemfs (or
> >>>>>> hugetlbfs) and so have mappings.
> >>>>>
> >>>>> Better then nothing, no? If easy to do..
> >>>>
> >>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
> >>>> not bode confidence and feels indefensible. I'd much rather throw my
> >>>> hands up and say "this is the best we can do with the API provided,
> >>>> please tell us what we should have done." To which the answer is
> >>>> probably to not have used gup in the first place :|
> >>>
> >>> """
> >>> /*
> >>>   * set_page_dirty() is racy if the caller has no reference against
> >>>   * page->mapping->host, and if the page is unlocked. This is 
> >>> because another
> >>>   * CPU could truncate the page off the mapping and then free the 
> >>> mapping.
> >>>   *
> >>>   * Usually, the page _is_ locked, or the caller is a user-space 
> >>> process which
> >>>   * holds a reference on the inode by having an open file.
> >>>   *
> >>>   * In other cases, the page should be locked before running 
> >>> set_page_dirty().
> >>>   */
> >>> int set_page_dirty_lock(struct page *page)
> >>> """
> >>>
> >>> Could we hold a reference to page->mapping->host while having pages 
> >>> and then would be okay to call plain set_page_dirty?
> >>
> >> We would then be hitting the warnings in ext4 for unlocked pages again.
> >
> > Ah true..
> >
> >> Essentially the argument is whether or not that warn is valid, to 
> >> which I
> >> think requires inner knowledge of vfs + ext4. To hold a reference on the
> >> host would require us tracking page->mapping (reasonable since we
> >> already hooked into mmu and so will get an invalidate + fresh gup on
> >> any changes), plus iterating over all to acquire the extra reference if
> >> applicable -- and I have no idea what the side-effects of that would be.
> >> Could well be positive side-effects. Just feels like wandering even
> >> further off the beaten path without a map. Good news hmm is just around
> >> the corner (which will probably prohibit this use-case) :|
> >
> > ... can we reach out to someone more knowledgeable in mm matters to 
> > recommend us what to do?
> >
> > Regards,
> >
> > Tvrtko
> 
> 
> Just a reminder to not let this slip.
> We run into userptr bugs in CI quite regularly.

Remind away. Revert or trylock, there doesn't seem to be a good answer.
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 09/09/2019 14:52, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-07-26 14:38:40)
>> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
>>>
>>> On 17/07/2019 15:06, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>>>>
>>>>> On 17/07/2019 14:35, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>>>>
>>>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>>>>
>>>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>>>>
>>>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr
>>>>>>>>>>>> and so call
>>>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock
>>>>>>>>>>>> and so we
>>>>>>>>>>>> must avoid recursively locking the pages ourselves -- which
>>>>>>>>>>>> means that
>>>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty().
>>>>>>>>>>>> Since we
>>>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the
>>>>>>>>>>>> page, or
>>>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>>>>> corruption.
>>>>>>>>>>>
>>>>>>>>>>> So if trylock randomly fail we get data corruption in whatever
>>>>>>>>>>> data set
>>>>>>>>>>> application is working on, which is what the original patch
>>>>>>>>>>> was trying
>>>>>>>>>>> to avoid? Are we able to detect the backing store type so at
>>>>>>>>>>> least we
>>>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>>>>
>>>>>>>>>> page->mapping???
>>>>>>>>>
>>>>>>>>> Would page->mapping work? What is it telling us?
>>>>>>>>
>>>>>>>> It basically tells us if there is a fs around; anything that is
>>>>>>>> the most
>>>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>>>>
>>>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_
>>>>>>> from
>>>>>>> the most basic malloc?
>>>>>>
>>>>>> Aye missed the not.
>>>>>>
>>>>>>>>>> We still have the issue that if there is a mapping we should be
>>>>>>>>>> taking
>>>>>>>>>> the lock, and we may have both a mapping and be inside
>>>>>>>>>> try_to_unmap().
>>>>>>>>>
>>>>>>>>> Is this a problem? On a path with mappings we trylock and so
>>>>>>>>> solve the
>>>>>>>>> set_dirty_locked and recursive deadlock issues, and with no
>>>>>>>>> mappings
>>>>>>>>> with always dirty the page and avoid data corruption.
>>>>>>>>
>>>>>>>> The problem as I see it is !page->mapping are likely an
>>>>>>>> insignificant
>>>>>>>> minority of userptr; as I think even memfd are essentially
>>>>>>>> shmemfs (or
>>>>>>>> hugetlbfs) and so have mappings.
>>>>>>>
>>>>>>> Better then nothing, no? If easy to do..
>>>>>>
>>>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>>>>> not bode confidence and feels indefensible. I'd much rather throw my
>>>>>> hands up and say "this is the best we can do with the API provided,
>>>>>> please tell us what we should have done." To which the answer is
>>>>>> probably to not have used gup in the first place :|
>>>>>
>>>>> """
>>>>> /*
>>>>>    * set_page_dirty() is racy if the caller has no reference against
>>>>>    * page->mapping->host, and if the page is unlocked. This is
>>>>> because another
>>>>>    * CPU could truncate the page off the mapping and then free the
>>>>> mapping.
>>>>>    *
>>>>>    * Usually, the page _is_ locked, or the caller is a user-space
>>>>> process which
>>>>>    * holds a reference on the inode by having an open file.
>>>>>    *
>>>>>    * In other cases, the page should be locked before running
>>>>> set_page_dirty().
>>>>>    */
>>>>> int set_page_dirty_lock(struct page *page)
>>>>> """
>>>>>
>>>>> Could we hold a reference to page->mapping->host while having pages
>>>>> and then would be okay to call plain set_page_dirty?
>>>>
>>>> We would then be hitting the warnings in ext4 for unlocked pages again.
>>>
>>> Ah true..
>>>
>>>> Essentially the argument is whether or not that warn is valid, to
>>>> which I
>>>> think requires inner knowledge of vfs + ext4. To hold a reference on the
>>>> host would require us tracking page->mapping (reasonable since we
>>>> already hooked into mmu and so will get an invalidate + fresh gup on
>>>> any changes), plus iterating over all to acquire the extra reference if
>>>> applicable -- and I have no idea what the side-effects of that would be.
>>>> Could well be positive side-effects. Just feels like wandering even
>>>> further off the beaten path without a map. Good news hmm is just around
>>>> the corner (which will probably prohibit this use-case) :|
>>>
>>> ... can we reach out to someone more knowledgeable in mm matters to
>>> recommend us what to do?
>>>
>>> Regards,
>>>
>>> Tvrtko
>>
>>
>> Just a reminder to not let this slip.
>> We run into userptr bugs in CI quite regularly.
> 
> Remind away. Revert or trylock, there doesn't seem to be a good answer.

Rock and a hard place. Data corruption for userptr users (with either 
trylock or no lock) or a deadlock (with the lock). I honestly can't 
decide what is worse. Tiny preference to deadlock rather than silent 
corruption. Misguided? Don't know really..

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-11 12:31:32)
> 
> On 09/09/2019 14:52, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2019-07-26 14:38:40)
> >> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
> >>>
> >>> On 17/07/2019 15:06, Chris Wilson wrote:
> >>>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
> >>>>>
> >>>>> On 17/07/2019 14:35, Chris Wilson wrote:
> >>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
> >>>>>>>
> >>>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
> >>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
> >>>>>>>>>
> >>>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
> >>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
> >>>>>>>>>>>
> >>>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
> >>>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr
> >>>>>>>>>>>> and so call
> >>>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock
> >>>>>>>>>>>> and so we
> >>>>>>>>>>>> must avoid recursively locking the pages ourselves -- which
> >>>>>>>>>>>> means that
> >>>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty().
> >>>>>>>>>>>> Since we
> >>>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the
> >>>>>>>>>>>> page, or
> >>>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
> >>>>>>>>>>>> corruption.
> >>>>>>>>>>>
> >>>>>>>>>>> So if trylock randomly fail we get data corruption in whatever
> >>>>>>>>>>> data set
> >>>>>>>>>>> application is working on, which is what the original patch
> >>>>>>>>>>> was trying
> >>>>>>>>>>> to avoid? Are we able to detect the backing store type so at
> >>>>>>>>>>> least we
> >>>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
> >>>>>>>>>>
> >>>>>>>>>> page->mapping???
> >>>>>>>>>
> >>>>>>>>> Would page->mapping work? What is it telling us?
> >>>>>>>>
> >>>>>>>> It basically tells us if there is a fs around; anything that is
> >>>>>>>> the most
> >>>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
> >>>>>>>
> >>>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_
> >>>>>>> from
> >>>>>>> the most basic malloc?
> >>>>>>
> >>>>>> Aye missed the not.
> >>>>>>
> >>>>>>>>>> We still have the issue that if there is a mapping we should be
> >>>>>>>>>> taking
> >>>>>>>>>> the lock, and we may have both a mapping and be inside
> >>>>>>>>>> try_to_unmap().
> >>>>>>>>>
> >>>>>>>>> Is this a problem? On a path with mappings we trylock and so
> >>>>>>>>> solve the
> >>>>>>>>> set_dirty_locked and recursive deadlock issues, and with no
> >>>>>>>>> mappings
> >>>>>>>>> with always dirty the page and avoid data corruption.
> >>>>>>>>
> >>>>>>>> The problem as I see it is !page->mapping are likely an
> >>>>>>>> insignificant
> >>>>>>>> minority of userptr; as I think even memfd are essentially
> >>>>>>>> shmemfs (or
> >>>>>>>> hugetlbfs) and so have mappings.
> >>>>>>>
> >>>>>>> Better then nothing, no? If easy to do..
> >>>>>>
> >>>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
> >>>>>> not bode confidence and feels indefensible. I'd much rather throw my
> >>>>>> hands up and say "this is the best we can do with the API provided,
> >>>>>> please tell us what we should have done." To which the answer is
> >>>>>> probably to not have used gup in the first place :|
> >>>>>
> >>>>> """
> >>>>> /*
> >>>>>    * set_page_dirty() is racy if the caller has no reference against
> >>>>>    * page->mapping->host, and if the page is unlocked. This is
> >>>>> because another
> >>>>>    * CPU could truncate the page off the mapping and then free the
> >>>>> mapping.
> >>>>>    *
> >>>>>    * Usually, the page _is_ locked, or the caller is a user-space
> >>>>> process which
> >>>>>    * holds a reference on the inode by having an open file.
> >>>>>    *
> >>>>>    * In other cases, the page should be locked before running
> >>>>> set_page_dirty().
> >>>>>    */
> >>>>> int set_page_dirty_lock(struct page *page)
> >>>>> """
> >>>>>
> >>>>> Could we hold a reference to page->mapping->host while having pages
> >>>>> and then would be okay to call plain set_page_dirty?
> >>>>
> >>>> We would then be hitting the warnings in ext4 for unlocked pages again.
> >>>
> >>> Ah true..
> >>>
> >>>> Essentially the argument is whether or not that warn is valid, to
> >>>> which I
> >>>> think requires inner knowledge of vfs + ext4. To hold a reference on the
> >>>> host would require us tracking page->mapping (reasonable since we
> >>>> already hooked into mmu and so will get an invalidate + fresh gup on
> >>>> any changes), plus iterating over all to acquire the extra reference if
> >>>> applicable -- and I have no idea what the side-effects of that would be.
> >>>> Could well be positive side-effects. Just feels like wandering even
> >>>> further off the beaten path without a map. Good news hmm is just around
> >>>> the corner (which will probably prohibit this use-case) :|
> >>>
> >>> ... can we reach out to someone more knowledgeable in mm matters to
> >>> recommend us what to do?
> >>>
> >>> Regards,
> >>>
> >>> Tvrtko
> >>
> >>
> >> Just a reminder to not let this slip.
> >> We run into userptr bugs in CI quite regularly.
> > 
> > Remind away. Revert or trylock, there doesn't seem to be a good answer.
> 
> Rock and a hard place. Data corruption for userptr users (with either 
> trylock or no lock) or a deadlock (with the lock). I honestly can't 
> decide what is worse. Tiny preference to deadlock rather than silent 
> corruption. Misguided? Don't know really..

The deadlock is pretty easy to hit as soon as the system is under
mempressure and it tries to free pages as we do the userptr gup...
(Hah, easy in theory, but not in CI.)
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Tvrtko Ursulin]

On 11/09/2019 12:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-11 12:31:32)
>>
>> On 09/09/2019 14:52, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2019-07-26 14:38:40)
>>>> On 17/07/2019 21:09, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 17/07/2019 15:06, Chris Wilson wrote:
>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:46:15)
>>>>>>>
>>>>>>> On 17/07/2019 14:35, Chris Wilson wrote:
>>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55)
>>>>>>>>>
>>>>>>>>> On 17/07/2019 14:17, Chris Wilson wrote:
>>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00)
>>>>>>>>>>>
>>>>>>>>>>> On 16/07/2019 16:37, Chris Wilson wrote:
>>>>>>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22)
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote:
>>>>>>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr
>>>>>>>>>>>>>> and so call
>>>>>>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock
>>>>>>>>>>>>>> and so we
>>>>>>>>>>>>>> must avoid recursively locking the pages ourselves -- which
>>>>>>>>>>>>>> means that
>>>>>>>>>>>>>> we cannot safely acquire the lock around set_page_dirty().
>>>>>>>>>>>>>> Since we
>>>>>>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the
>>>>>>>>>>>>>> page, or
>>>>>>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs
>>>>>>>>>>>>>> corruption.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So if trylock randomly fail we get data corruption in whatever
>>>>>>>>>>>>> data set
>>>>>>>>>>>>> application is working on, which is what the original patch
>>>>>>>>>>>>> was trying
>>>>>>>>>>>>> to avoid? Are we able to detect the backing store type so at
>>>>>>>>>>>>> least we
>>>>>>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs?
>>>>>>>>>>>>
>>>>>>>>>>>> page->mapping???
>>>>>>>>>>>
>>>>>>>>>>> Would page->mapping work? What is it telling us?
>>>>>>>>>>
>>>>>>>>>> It basically tells us if there is a fs around; anything that is
>>>>>>>>>> the most
>>>>>>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping).
>>>>>>>>>
>>>>>>>>> Normal malloc so anonymous pages? Or you meant everything _apart_
>>>>>>>>> from
>>>>>>>>> the most basic malloc?
>>>>>>>>
>>>>>>>> Aye missed the not.
>>>>>>>>
>>>>>>>>>>>> We still have the issue that if there is a mapping we should be
>>>>>>>>>>>> taking
>>>>>>>>>>>> the lock, and we may have both a mapping and be inside
>>>>>>>>>>>> try_to_unmap().
>>>>>>>>>>>
>>>>>>>>>>> Is this a problem? On a path with mappings we trylock and so
>>>>>>>>>>> solve the
>>>>>>>>>>> set_dirty_locked and recursive deadlock issues, and with no
>>>>>>>>>>> mappings
>>>>>>>>>>> with always dirty the page and avoid data corruption.
>>>>>>>>>>
>>>>>>>>>> The problem as I see it is !page->mapping are likely an
>>>>>>>>>> insignificant
>>>>>>>>>> minority of userptr; as I think even memfd are essentially
>>>>>>>>>> shmemfs (or
>>>>>>>>>> hugetlbfs) and so have mappings.
>>>>>>>>>
>>>>>>>>> Better then nothing, no? If easy to do..
>>>>>>>>
>>>>>>>> Actually, I erring on the opposite side. Peeking at mm/ internals does
>>>>>>>> not bode confidence and feels indefensible. I'd much rather throw my
>>>>>>>> hands up and say "this is the best we can do with the API provided,
>>>>>>>> please tell us what we should have done." To which the answer is
>>>>>>>> probably to not have used gup in the first place :|
>>>>>>>
>>>>>>> """
>>>>>>> /*
>>>>>>>     * set_page_dirty() is racy if the caller has no reference against
>>>>>>>     * page->mapping->host, and if the page is unlocked. This is
>>>>>>> because another
>>>>>>>     * CPU could truncate the page off the mapping and then free the
>>>>>>> mapping.
>>>>>>>     *
>>>>>>>     * Usually, the page _is_ locked, or the caller is a user-space
>>>>>>> process which
>>>>>>>     * holds a reference on the inode by having an open file.
>>>>>>>     *
>>>>>>>     * In other cases, the page should be locked before running
>>>>>>> set_page_dirty().
>>>>>>>     */
>>>>>>> int set_page_dirty_lock(struct page *page)
>>>>>>> """
>>>>>>>
>>>>>>> Could we hold a reference to page->mapping->host while having pages
>>>>>>> and then would be okay to call plain set_page_dirty?
>>>>>>
>>>>>> We would then be hitting the warnings in ext4 for unlocked pages again.
>>>>>
>>>>> Ah true..
>>>>>
>>>>>> Essentially the argument is whether or not that warn is valid, to
>>>>>> which I
>>>>>> think requires inner knowledge of vfs + ext4. To hold a reference on the
>>>>>> host would require us tracking page->mapping (reasonable since we
>>>>>> already hooked into mmu and so will get an invalidate + fresh gup on
>>>>>> any changes), plus iterating over all to acquire the extra reference if
>>>>>> applicable -- and I have no idea what the side-effects of that would be.
>>>>>> Could well be positive side-effects. Just feels like wandering even
>>>>>> further off the beaten path without a map. Good news hmm is just around
>>>>>> the corner (which will probably prohibit this use-case) :|
>>>>>
>>>>> ... can we reach out to someone more knowledgeable in mm matters to
>>>>> recommend us what to do?
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>
>>>>
>>>> Just a reminder to not let this slip.
>>>> We run into userptr bugs in CI quite regularly.
>>>
>>> Remind away. Revert or trylock, there doesn't seem to be a good answer.
>>
>> Rock and a hard place. Data corruption for userptr users (with either
>> trylock or no lock) or a deadlock (with the lock). I honestly can't
>> decide what is worse. Tiny preference to deadlock rather than silent
>> corruption. Misguided? Don't know really..
> 
> The deadlock is pretty easy to hit as soon as the system is under
> mempressure and it tries to free pages as we do the userptr gup...
> (Hah, easy in theory, but not in CI.)

I know what's the answer! Push the policy to userspace! :D

echo 1 > /sys/class/drm/card0/userptr_corrupt_or_deadlock

Am I joking or not? Wish I knew! :)

Regards,

Tvrtko

✓ Fi.CI.IGT: success for series starting with [1/5] drm/i915/userptr: Beware recursive lock_page()

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/userptr: Beware recursive lock_page()
URL   : https://patchwork.freedesktop.org/series/63752/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6492_full -> Patchwork_13664_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          [PASS][1] -> [SKIP][2] ([fdo#109271])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-snb7/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-snb6/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x42-sliding:
    - shard-skl:          [PASS][3] -> [FAIL][4] ([fdo#103232])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-128x42-sliding.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-128x42-sliding.html

  * igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic:
    - shard-skl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#105541])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl7/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl7/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html

  * igt@kms_flip@2x-plain-flip-ts-check:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([fdo#100368])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-glk2/igt@kms_flip@2x-plain-flip-ts-check.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-glk9/igt@kms_flip@2x-plain-flip-ts-check.html

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][9] -> [FAIL][10] ([fdo#103167]) +6 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-glk:          [PASS][11] -> [FAIL][12] ([fdo#103167]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-glk7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-glk8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-wc:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#103167]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl7/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-wc.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl8/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-wc.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#108145]) +1 similar issue
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([fdo#108145] / [fdo#110403])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109642] / [fdo#111068])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-iclb8/igt@kms_psr2_su@frontbuffer.html

  
#### Possible fixes ####

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-apl:          [DMESG-WARN][21] ([fdo#108566]) -> [PASS][22] +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-apl5/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-apl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
    - shard-hsw:          [FAIL][23] ([fdo#103355]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-hsw5/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-hsw7/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-skl:          [INCOMPLETE][25] ([fdo#109507]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl7/igt@kms_flip@flip-vs-suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl8/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
    - shard-iclb:         [FAIL][27] ([fdo#103167]) -> [PASS][28] +7 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [FAIL][29] ([fdo#108145]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [FAIL][31] ([fdo#103166]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-iclb1/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [SKIP][33] ([fdo#109441]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-iclb3/igt@kms_psr@psr2_primary_page_flip.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][35] ([fdo#99912]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-apl3/igt@kms_setmode@basic.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-apl6/igt@kms_setmode@basic.html

  * igt@perf@blocking:
    - shard-skl:          [FAIL][37] ([fdo#110728]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-skl6/igt@perf@blocking.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-skl2/igt@perf@blocking.html

  
#### Warnings ####

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [DMESG-WARN][39] ([fdo#107724]) -> [SKIP][40] ([fdo#109349])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6492/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13664/shard-iclb1/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (9 -> 10)
------------------------------

  Additional (1): pig-skl-6260u 


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

  * Linux: CI_DRM_6492 -> Patchwork_13664

  CI_DRM_6492: cb320040f8fea17bf02644f3536ebb34cf9cb5e1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5100: 0ea68a1efbfcc4961f2f816ab59e4ad8136c6250 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13664: a6bfd92acc43666e1f9cdbc5c5ac27eeb310a717 @ 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_13664/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Chris Wilson (2019-07-16 13:49:27)
> Following a try_to_unmap() we may want to remove the userptr and so call
> put_pages(). However, try_to_unmap() acquires the page lock and so we
> must avoid recursively locking the pages ourselves -- which means that
> we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.
> 
> Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index b9d2bb15e4a6..1ad2047a6dbd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>                 obj->mm.dirty = false;
>  
>         for_each_sgt_page(page, sgt_iter, pages) {
> -               if (obj->mm.dirty)
> +               if (obj->mm.dirty && trylock_page(page)) {
>                         /*
>                          * As this may not be anonymous memory (e.g. shmem)
>                          * but exist on a real mapping, we have to lock
> @@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>                          * the page reference is not sufficient to
>                          * prevent the inode from being truncated.
>                          * Play safe and take the lock.
> +                        *
> +                        * However...!
> +                        *
> +                        * The mmu-notifier can be invalidated for a
> +                        * migrate_page, that is alreadying holding the lock
> +                        * on the page. Such a try_to_unmap() will result
> +                        * in us calling put_pages() and so recursively try
> +                        * to lock the page. We avoid that deadlock with
> +                        * a trylock_page() and in exchange we risk missing
> +                        * some page dirtying.
>                          */
> -                       set_page_dirty_lock(page);
> +                       set_page_dirty(page);
> +                       unlock_page(page);
> +               }

It really seems like we have no choice but to only call set_page_dirty()
while under the page lock, and the only way we can guarantee that
without recursion is with a trylock...

https://bugs.freedesktop.org/show_bug.cgi?id=112012
-Chris

Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page()

[Chris Wilson]

Quoting Chris Wilson (2019-07-16 13:49:27)
> Following a try_to_unmap() we may want to remove the userptr and so call
> put_pages(). However, try_to_unmap() acquires the page lock and so we
> must avoid recursively locking the pages ourselves -- which means that
> we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.
> 
> Reported-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index b9d2bb15e4a6..1ad2047a6dbd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -672,7 +672,7 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>                 obj->mm.dirty = false;
>  
>         for_each_sgt_page(page, sgt_iter, pages) {
> -               if (obj->mm.dirty)
> +               if (obj->mm.dirty && trylock_page(page)) {
>                         /*
>                          * As this may not be anonymous memory (e.g. shmem)
>                          * but exist on a real mapping, we have to lock
> @@ -680,8 +680,20 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>                          * the page reference is not sufficient to
>                          * prevent the inode from being truncated.
>                          * Play safe and take the lock.
> +                        *
> +                        * However...!
> +                        *
> +                        * The mmu-notifier can be invalidated for a
> +                        * migrate_page, that is alreadying holding the lock
> +                        * on the page. Such a try_to_unmap() will result
> +                        * in us calling put_pages() and so recursively try
> +                        * to lock the page. We avoid that deadlock with
> +                        * a trylock_page() and in exchange we risk missing
> +                        * some page dirtying.
>                          */
> -                       set_page_dirty_lock(page);
> +                       set_page_dirty(page);
> +                       unlock_page(page);
> +               }

It really seems like we have no choice but to only call set_page_dirty()
while under the page lock, and the only way we can guarantee that
without recursion is with a trylock...

https://bugs.freedesktop.org/show_bug.cgi?id=112012
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx