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

[Tvrtko Ursulin]

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.ursu...@intel.com>
Signed-off-by: Matthew Brost <matthew.br...@intel.com>
---
  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);
        }