On Mon, Aug 16, 2021 at 06:51:36AM -0700, Matthew Brost wrote: > Lock the xarray and take ref to the context if needed. > > v2: > (Checkpatch) > - Add new line after declaration > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx> > --- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 84 ++++++++++++++++--- > 1 file changed, 73 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index ba19b99173fc..2ecb2f002bed 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -599,8 +599,18 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) > unsigned long index, flags; > bool pending_disable, pending_enable, deregister, destroyed, banned; > > + xa_lock_irqsave(&guc->context_lookup, flags); > xa_for_each(&guc->context_lookup, index, ce) { > - spin_lock_irqsave(&ce->guc_state.lock, flags); > + /* > + * Corner case where the ref count on the object is zero but and > + * deregister G2H was lost. In this case we don't touch the ref > + * count and finish the destroy of the context. > + */ > + bool do_put = kref_get_unless_zero(&ce->ref); This looks really scary, because in another loop below you have an unconditional refcount increase. This means sometimes guc->context_lookup xarray guarantees we hold a full reference on the context, sometimes we don't. So we're right back in "protect the code" O(N^2) review complexity instead of invariant rules about the datastructure, which is linear. Essentially anytime you feel like you have to add a comment to explain what's going on about concurrent stuff you're racing with, you're protecting code, not data. Since guc can't do a hole lot without the guc_id registered and all that, I kinda expected you'd always have a full reference here. If there's intermediate stages (e.g. around unregister) where this is currently not always the case, then those should make sure a full reference is held. Another option would be to threa ->context_lookup as a weak reference that we lazily clean up when the context is finalized. That works too, but probably not with a spinlock (since you most likely have to wait for all pending guc transations to complete), but it's another option. Either way I think standard process is needed here for locking design, i.e. 1. come up with the right invariants ("we always have a full reference when a context is ont he guc->context_lookup xarray") 2. come up with the locks. From the guc side the xa_lock is maybe good enough, but from the context side this doesn't protect against a re-registering racing against a deregistering. So probably needs more rules on top, and then you have a nice lock inversion in a few places like here. 3. document it and roll it out. The other thing is that this is a very tricky iterator, and there's a few copies of it. That is, if this is the right solution. As-is this should be abstracted away into guc_context_iter_begin/next_end() helpers, e.g. like we have for drm_connector_list_iter_begin/end_next as an example. Cheers, Daniel > + > + xa_unlock(&guc->context_lookup); > + > + spin_lock(&ce->guc_state.lock); > > /* > * Once we are at this point submission_disabled() is guaranteed > @@ -616,7 +626,9 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) > banned = context_banned(ce); > init_sched_state(ce); > > - spin_unlock_irqrestore(&ce->guc_state.lock, flags); > + spin_unlock(&ce->guc_state.lock); > + > + GEM_BUG_ON(!do_put && !destroyed); > > if (pending_enable || destroyed || deregister) { > atomic_dec(&guc->outstanding_submission_g2h); > @@ -645,7 +657,12 @@ static void scrub_guc_desc_for_outstanding_g2h(struct intel_guc *guc) > > intel_context_put(ce); > } > + > + if (do_put) > + intel_context_put(ce); > + xa_lock(&guc->context_lookup); > } > + xa_unlock_irqrestore(&guc->context_lookup, flags); > } > > static inline bool > @@ -866,16 +883,26 @@ void intel_guc_submission_reset(struct intel_guc *guc, bool stalled) > { > struct intel_context *ce; > unsigned long index; > + unsigned long flags; > > if (unlikely(!guc_submission_initialized(guc))) { > /* Reset called during driver load? GuC not yet initialised! */ > return; > } > > - xa_for_each(&guc->context_lookup, index, ce) > + xa_lock_irqsave(&guc->context_lookup, flags); > + xa_for_each(&guc->context_lookup, index, ce) { > + intel_context_get(ce); > + xa_unlock(&guc->context_lookup); > + > if (intel_context_is_pinned(ce)) > __guc_reset_context(ce, stalled); > > + intel_context_put(ce); > + xa_lock(&guc->context_lookup); > + } > + xa_unlock_irqrestore(&guc->context_lookup, flags); > + > /* GuC is blown away, drop all references to contexts */ > xa_destroy(&guc->context_lookup); > } > @@ -950,11 +977,21 @@ void intel_guc_submission_cancel_requests(struct intel_guc *guc) > { > struct intel_context *ce; > unsigned long index; > + unsigned long flags; > + > + xa_lock_irqsave(&guc->context_lookup, flags); > + xa_for_each(&guc->context_lookup, index, ce) { > + intel_context_get(ce); > + xa_unlock(&guc->context_lookup); > > - xa_for_each(&guc->context_lookup, index, ce) > if (intel_context_is_pinned(ce)) > guc_cancel_context_requests(ce); > > + intel_context_put(ce); > + xa_lock(&guc->context_lookup); > + } > + xa_unlock_irqrestore(&guc->context_lookup, flags); > + > guc_cancel_sched_engine_requests(guc->sched_engine); > > /* GuC is blown away, drop all references to contexts */ > @@ -2848,21 +2885,26 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine) > struct intel_context *ce; > struct i915_request *rq; > unsigned long index; > + unsigned long flags; > > /* Reset called during driver load? GuC not yet initialised! */ > if (unlikely(!guc_submission_initialized(guc))) > return; > > + xa_lock_irqsave(&guc->context_lookup, flags); > xa_for_each(&guc->context_lookup, index, ce) { > + intel_context_get(ce); > + xa_unlock(&guc->context_lookup); > + > if (!intel_context_is_pinned(ce)) > - continue; > + goto next; > > if (intel_engine_is_virtual(ce->engine)) { > if (!(ce->engine->mask & engine->mask)) > - continue; > + goto next; > } else { > if (ce->engine != engine) > - continue; > + goto next; > } > > list_for_each_entry(rq, &ce->guc_active.requests, sched.link) { > @@ -2872,9 +2914,17 @@ void intel_guc_find_hung_context(struct intel_engine_cs *engine) > intel_engine_set_hung_context(engine, ce); > > /* Can only cope with one hang at a time... */ > - return; > + intel_context_put(ce); > + xa_lock(&guc->context_lookup); > + goto done; > } > +next: > + intel_context_put(ce); > + xa_lock(&guc->context_lookup); > + > } > +done: > + xa_unlock_irqrestore(&guc->context_lookup, flags); > } > > void intel_guc_dump_active_requests(struct intel_engine_cs *engine, > @@ -2890,23 +2940,32 @@ void intel_guc_dump_active_requests(struct intel_engine_cs *engine, > if (unlikely(!guc_submission_initialized(guc))) > return; > > + xa_lock_irqsave(&guc->context_lookup, flags); > xa_for_each(&guc->context_lookup, index, ce) { > + intel_context_get(ce); > + xa_unlock(&guc->context_lookup); > + > if (!intel_context_is_pinned(ce)) > - continue; > + goto next; > > if (intel_engine_is_virtual(ce->engine)) { > if (!(ce->engine->mask & engine->mask)) > - continue; > + goto next; > } else { > if (ce->engine != engine) > - continue; > + goto next; > } > > spin_lock_irqsave(&ce->guc_active.lock, flags); > intel_engine_dump_active_requests(&ce->guc_active.requests, > hung_rq, m); > spin_unlock_irqrestore(&ce->guc_active.lock, flags); > + > +next: > + intel_context_put(ce); > + xa_lock(&guc->context_lookup); > } > + xa_unlock_irqrestore(&guc->context_lookup, flags); > } > > void intel_guc_submission_print_info(struct intel_guc *guc, > @@ -2960,7 +3019,9 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc, > { > struct intel_context *ce; > unsigned long index; > + unsigned long flags; > > + xa_lock_irqsave(&guc->context_lookup, flags); > xa_for_each(&guc->context_lookup, index, ce) { > drm_printf(p, "GuC lrc descriptor %u:\n", ce->guc_id); > drm_printf(p, "\tHW Context Desc: 0x%08x\n", ce->lrc.lrca); > @@ -2979,6 +3040,7 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc, > > guc_log_context_priority(p, ce); > } > + xa_unlock_irqrestore(&guc->context_lookup, flags); > } > > static struct intel_context * > -- > 2.32.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch