diff mbox series

[03/37] drm/i915/gt: Free stale request on destroying the virtual engine

Message ID 20200805122231.23313-4-chris@chris-wilson.co.uk
State New, archived
Headers show
Series Replace obj->mm.lock with reservation_ww_class | expand

Commit Message

Chris Wilson Aug. 5, 2020, 12:21 p.m. UTC
Since preempt-to-busy, we may unsubmit a request while it is still on
the HW and completes asynchronously. That means it may be retired and in
the process destroy the virtual engine (as the user has closed their
context), but that engine may still be holding onto the unsubmitted
compelted request. Therefore we need to potentially cleanup the old
request on destroying the virtual engine. We also have to keep the
virtual_engine alive until after the sibling's execlists_dequeue() have
finished peeking into the virtual engines, for which we serialise with
RCU.

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 | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Tvrtko Ursulin Aug. 5, 2020, 3:05 p.m. UTC | #1
On 05/08/2020 13:21, Chris Wilson wrote:
> Since preempt-to-busy, we may unsubmit a request while it is still on
> the HW and completes asynchronously. That means it may be retired and in
> the process destroy the virtual engine (as the user has closed their
> context), but that engine may still be holding onto the unsubmitted
> compelted request. Therefore we need to potentially cleanup the old
> request on destroying the virtual engine. We also have to keep the
> virtual_engine alive until after the sibling's execlists_dequeue() have
> finished peeking into the virtual engines, for which we serialise with
> RCU.
> 
> 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 | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 417f6b0c6c61..cb04bc5474be 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -180,6 +180,7 @@
>   #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
>   
>   struct virtual_engine {
> +	struct rcu_head rcu;
>   	struct intel_engine_cs base;
>   	struct intel_context context;
>   
> @@ -5393,10 +5394,25 @@ static void virtual_context_destroy(struct kref *kref)
>   		container_of(kref, typeof(*ve), context.ref);
>   	unsigned int n;
>   
> -	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> -	GEM_BUG_ON(ve->request);
>   	GEM_BUG_ON(ve->context.inflight);
>   
> +	if (unlikely(ve->request)) {
> +		struct i915_request *old;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&ve->base.active.lock, flags);
> +
> +		old = fetch_and_zero(&ve->request);
> +		if (old) {
> +			GEM_BUG_ON(!i915_request_completed(old));
> +			__i915_request_submit(old);
> +			i915_request_put(old);
> +		}
> +
> +		spin_unlock_irqrestore(&ve->base.active.lock, flags);
> +	}
> +	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> +
>   	for (n = 0; n < ve->num_siblings; n++) {
>   		struct intel_engine_cs *sibling = ve->siblings[n];
>   		struct rb_node *node = &ve->nodes[sibling->id].rb;
> @@ -5422,7 +5438,7 @@ static void virtual_context_destroy(struct kref *kref)
>   	intel_engine_free_request_pool(&ve->base);
>   
>   	kfree(ve->bonds);
> -	kfree(ve);
> +	kfree_rcu(ve, rcu);
>   }
>   
>   static void virtual_engine_initial_hint(struct virtual_engine *ve)
> 

If it would go without the previous patch I think it would simply mean a 
normal kfree here. In both cases it looks okay to me.

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

Regards,

Tvrtko
Chris Wilson Aug. 6, 2020, 10:44 a.m. UTC | #2
Quoting Tvrtko Ursulin (2020-08-05 16:05:28)
> 
> On 05/08/2020 13:21, Chris Wilson wrote:
> > Since preempt-to-busy, we may unsubmit a request while it is still on
> > the HW and completes asynchronously. That means it may be retired and in
> > the process destroy the virtual engine (as the user has closed their
> > context), but that engine may still be holding onto the unsubmitted
> > compelted request. Therefore we need to potentially cleanup the old
> > request on destroying the virtual engine. We also have to keep the
> > virtual_engine alive until after the sibling's execlists_dequeue() have
> > finished peeking into the virtual engines, for which we serialise with
> > RCU.
> > 
> > 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 | 22 +++++++++++++++++++---
> >   1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 417f6b0c6c61..cb04bc5474be 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -180,6 +180,7 @@
> >   #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
> >   
> >   struct virtual_engine {
> > +     struct rcu_head rcu;
> >       struct intel_engine_cs base;
> >       struct intel_context context;
> >   
> > @@ -5393,10 +5394,25 @@ static void virtual_context_destroy(struct kref *kref)
> >               container_of(kref, typeof(*ve), context.ref);
> >       unsigned int n;
> >   
> > -     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> > -     GEM_BUG_ON(ve->request);
> >       GEM_BUG_ON(ve->context.inflight);
> >   
> > +     if (unlikely(ve->request)) {
> > +             struct i915_request *old;
> > +             unsigned long flags;
> > +
> > +             spin_lock_irqsave(&ve->base.active.lock, flags);
> > +
> > +             old = fetch_and_zero(&ve->request);
> > +             if (old) {
> > +                     GEM_BUG_ON(!i915_request_completed(old));
> > +                     __i915_request_submit(old);
> > +                     i915_request_put(old);
> > +             }
> > +
> > +             spin_unlock_irqrestore(&ve->base.active.lock, flags);
> > +     }
> > +     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> > +
> >       for (n = 0; n < ve->num_siblings; n++) {
> >               struct intel_engine_cs *sibling = ve->siblings[n];
> >               struct rb_node *node = &ve->nodes[sibling->id].rb;
> > @@ -5422,7 +5438,7 @@ static void virtual_context_destroy(struct kref *kref)
> >       intel_engine_free_request_pool(&ve->base);
> >   
> >       kfree(ve->bonds);
> > -     kfree(ve);
> > +     kfree_rcu(ve, rcu);
> >   }
> >   
> >   static void virtual_engine_initial_hint(struct virtual_engine *ve)
> > 
> 
> If it would go without the previous patch I think it would simply mean a 
> normal kfree here. In both cases it looks okay to me.

Ah, this rcu predates the other by an untold number of months :)

I'm trying to remember why I thought there was a danger here that the
zombie could be accessed as we destroyed the veng. There's definitely
something precarious with the context unpin being called from retire,
while the stale request is being resubmitted and the final reference
being dropped (with preemption jumping between).

So after that thought, the rcu here is just paper, the final step
required after removing the request, decoupling the virtual engines, is
flushing the tasklet. The problem though is that intel_context_put()
is iirc called from some nasty contexts. An rcu_work is quite tempting,
we have rcu read locks around most of the accessors baring a concurrent
tasklet [hopefully not missing anything], so it's just that tasklet
flush requiring process context during destroy.
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 417f6b0c6c61..cb04bc5474be 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -180,6 +180,7 @@ 
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
 
 struct virtual_engine {
+	struct rcu_head rcu;
 	struct intel_engine_cs base;
 	struct intel_context context;
 
@@ -5393,10 +5394,25 @@  static void virtual_context_destroy(struct kref *kref)
 		container_of(kref, typeof(*ve), context.ref);
 	unsigned int n;
 
-	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
-	GEM_BUG_ON(ve->request);
 	GEM_BUG_ON(ve->context.inflight);
 
+	if (unlikely(ve->request)) {
+		struct i915_request *old;
+		unsigned long flags;
+
+		spin_lock_irqsave(&ve->base.active.lock, flags);
+
+		old = fetch_and_zero(&ve->request);
+		if (old) {
+			GEM_BUG_ON(!i915_request_completed(old));
+			__i915_request_submit(old);
+			i915_request_put(old);
+		}
+
+		spin_unlock_irqrestore(&ve->base.active.lock, flags);
+	}
+	GEM_BUG_ON(!list_empty(virtual_queue(ve)));
+
 	for (n = 0; n < ve->num_siblings; n++) {
 		struct intel_engine_cs *sibling = ve->siblings[n];
 		struct rb_node *node = &ve->nodes[sibling->id].rb;
@@ -5422,7 +5438,7 @@  static void virtual_context_destroy(struct kref *kref)
 	intel_engine_free_request_pool(&ve->base);
 
 	kfree(ve->bonds);
-	kfree(ve);
+	kfree_rcu(ve, rcu);
 }
 
 static void virtual_engine_initial_hint(struct virtual_engine *ve)