Re: [PATCH] drm/i915/guc: Use lockless list for destroyed contexts

[John Harrison <john.c.harrison@xxxxxxxxx>]

On 12/22/2021 15:29, Matthew Brost wrote:
> Use a lockless list structure for destroyed contexts to avoid hammering
> on global submission spin lock.
I thought the guidance was that lockless anything without an explanation 
longer than War And Peace comes with an automatic termination penalty?

Also, I thought the simple suggestion was to just move the entire list 
sideways under the existing lock and then loop through the local list 
safely without requiring locks because it is now local only.

John.


> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_context.c       |  2 -
>   drivers/gpu/drm/i915/gt/intel_context_types.h |  3 +-
>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  3 +-
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 44 +++++--------------
>   4 files changed, 16 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 5d0ec7c49b6a..4aacb4b0418d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -403,8 +403,6 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
>   	ce->guc_id.id = GUC_INVALID_LRC_ID;
>   	INIT_LIST_HEAD(&ce->guc_id.link);
>   
> -	INIT_LIST_HEAD(&ce->destroyed_link);
> -
>   	INIT_LIST_HEAD(&ce->parallel.child_list);
>   
>   	/*
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index 30cd81ad8911..4532d43ec9c0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -9,6 +9,7 @@
>   #include <linux/average.h>
>   #include <linux/kref.h>
>   #include <linux/list.h>
> +#include <linux/llist.h>
>   #include <linux/mutex.h>
>   #include <linux/types.h>
>   
> @@ -224,7 +225,7 @@ struct intel_context {
>   	 * list when context is pending to be destroyed (deregistered with the
>   	 * GuC), protected by guc->submission_state.lock
>   	 */
> -	struct list_head destroyed_link;
> +	struct llist_node destroyed_link;
>   
>   	/** @parallel: sub-structure for parallel submission members */
>   	struct {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index f9240d4baa69..705085058411 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -8,6 +8,7 @@
>   
>   #include <linux/xarray.h>
>   #include <linux/delay.h>
> +#include <linux/llist.h>
>   
>   #include "intel_uncore.h"
>   #include "intel_guc_fw.h"
> @@ -112,7 +113,7 @@ struct intel_guc {
>   		 * @destroyed_contexts: list of contexts waiting to be destroyed
>   		 * (deregistered with the GuC)
>   		 */
> -		struct list_head destroyed_contexts;
> +		struct llist_head destroyed_contexts;
>   		/**
>   		 * @destroyed_worker: worker to deregister contexts, need as we
>   		 * need to take a GT PM reference and can't from destroy
> 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 0a03a30e4c6d..6f7643edc139 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1771,7 +1771,7 @@ int intel_guc_submission_init(struct intel_guc *guc)
>   	spin_lock_init(&guc->submission_state.lock);
>   	INIT_LIST_HEAD(&guc->submission_state.guc_id_list);
>   	ida_init(&guc->submission_state.guc_ids);
> -	INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
> +	init_llist_head(&guc->submission_state.destroyed_contexts);
>   	INIT_WORK(&guc->submission_state.destroyed_worker,
>   		  destroyed_worker_func);
>   
> @@ -2696,26 +2696,18 @@ static void __guc_context_destroy(struct intel_context *ce)
>   	}
>   }
>   
> +#define take_destroyed_contexts(guc) \
> +	llist_del_all(&guc->submission_state.destroyed_contexts)
> +
>   static void guc_flush_destroyed_contexts(struct intel_guc *guc)
>   {
> -	struct intel_context *ce;
> -	unsigned long flags;
> +	struct intel_context *ce, *cn;
>   
>   	GEM_BUG_ON(!submission_disabled(guc) &&
>   		   guc_submission_initialized(guc));
>   
> -	while (!list_empty(&guc->submission_state.destroyed_contexts)) {
> -		spin_lock_irqsave(&guc->submission_state.lock, flags);
> -		ce = list_first_entry_or_null(&guc->submission_state.destroyed_contexts,
> -					      struct intel_context,
> -					      destroyed_link);
> -		if (ce)
> -			list_del_init(&ce->destroyed_link);
> -		spin_unlock_irqrestore(&guc->submission_state.lock, flags);
> -
> -		if (!ce)
> -			break;
> -
> +	llist_for_each_entry_safe(ce, cn, take_destroyed_contexts(guc),
> +				 destroyed_link) {
>   		release_guc_id(guc, ce);
>   		__guc_context_destroy(ce);
>   	}
> @@ -2723,23 +2715,11 @@ static void guc_flush_destroyed_contexts(struct intel_guc *guc)
>   
>   static void deregister_destroyed_contexts(struct intel_guc *guc)
>   {
> -	struct intel_context *ce;
> -	unsigned long flags;
> -
> -	while (!list_empty(&guc->submission_state.destroyed_contexts)) {
> -		spin_lock_irqsave(&guc->submission_state.lock, flags);
> -		ce = list_first_entry_or_null(&guc->submission_state.destroyed_contexts,
> -					      struct intel_context,
> -					      destroyed_link);
> -		if (ce)
> -			list_del_init(&ce->destroyed_link);
> -		spin_unlock_irqrestore(&guc->submission_state.lock, flags);
> -
> -		if (!ce)
> -			break;
> +	struct intel_context *ce, *cn;
>   
> +	llist_for_each_entry_safe(ce, cn, take_destroyed_contexts(guc),
> +				 destroyed_link)
>   		guc_lrc_desc_unpin(ce);
> -	}
>   }
>   
>   static void destroyed_worker_func(struct work_struct *w)
> @@ -2771,8 +2751,8 @@ static void guc_context_destroy(struct kref *kref)
>   	if (likely(!destroy)) {
>   		if (!list_empty(&ce->guc_id.link))
>   			list_del_init(&ce->guc_id.link);
> -		list_add_tail(&ce->destroyed_link,
> -			      &guc->submission_state.destroyed_contexts);
> +		llist_add(&ce->destroyed_link,
> +			  &guc->submission_state.destroyed_contexts);
>   	} else {
>   		__release_guc_id(guc, ce);
>   	}