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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 22/12/2021 23:29, Matthew Brost wrote:
> Use a lockless list structure for destroyed contexts to avoid hammering
> on global submission spin lock.

Thanks for looking into it quickly!

On the topic of "lockless" yes I agree the llist in principle is not a 
concern. That part looks fine to me. On the actual "integration" (how it 
slots in) with the GuC code I leave one comment below.

> 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);

So here presumably submission lock is still needed for unlinking the 
from guc_id list.

Mechanical flows of the patch looks good to me, but I leave to you and 
John to decide on llist vs keeping the existing doubly linked list. I 
mean agreeing what fits better with the existing locking and data 
structure design.

Regards,

Tvrtko

>   	} else {
>   		__release_guc_id(guc, ce);
>   	}