Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Clarify type evolution of uabi_node/uabi_engines

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/09/2023 19:20, Mathias Krause wrote:
> Chaining user engines happens in multiple passes during driver
> initialization, mutating its type along the way. It starts off with a
> simple lock-less linked list (struct llist_node/head) populated by
> intel_engine_add_user() which later gets sorted and converted to an
> intermediate regular list (struct list_head) just to be converted once
> more to its final rb-tree structure (struct rb_node/root) in
> intel_engines_driver_register().
> 
> All of these types overlay the uabi_node/uabi_engines members which is
> unfortunate but safe if one takes care about using the rb-tree based
> structure only after the conversion has completed. However, mistakes
> happen and commit 1ec23ed7126e ("drm/i915: Use uabi engines for the
> default engine map") violated that assumption, as the multiple type
> evolution was all to easy hidden behind casts papering over it.
> 
> Make the type evolution of uabi_node/uabi_engines more visible by
> putting all members into an anonymous union and use the correctly typed
> member in its various users. This allows us to drop quite some ugly
> casts and, hopefully, make the evolution of the members better
> recognisable to avoid future mistakes.
> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++-
>   drivers/gpu/drm/i915/gt/intel_engine_user.c  | 17 +++++++----------
>   drivers/gpu/drm/i915/i915_drv.h              | 17 ++++++++++++++++-
>   3 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index a7e677598004..7585fffac60b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -402,7 +402,15 @@ struct intel_engine_cs {
>   
>   	unsigned long context_tag;
>   
> -	struct rb_node uabi_node;
> +	/*
> +	 * The type evolves during initialization, see related comment for
> +	 * struct drm_i915_private's uabi_engines member.
> +	 */
> +	union {
> +		struct llist_node uabi_llist;
> +		struct list_head uabi_list;
> +		struct rb_node uabi_node;
> +	};
>   
>   	struct intel_sseu sseu;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> index dcedff41a825..118164ddbb2e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> @@ -38,8 +38,7 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
>   
>   void intel_engine_add_user(struct intel_engine_cs *engine)
>   {
> -	llist_add((struct llist_node *)&engine->uabi_node,
> -		  (struct llist_head *)&engine->i915->uabi_engines);
> +	llist_add(&engine->uabi_llist, &engine->i915->uabi_engines_llist);
>   }
>   
>   static const u8 uabi_classes[] = {
> @@ -54,9 +53,9 @@ static int engine_cmp(void *priv, const struct list_head *A,
>   		      const struct list_head *B)
>   {
>   	const struct intel_engine_cs *a =
> -		container_of((struct rb_node *)A, typeof(*a), uabi_node);
> +		container_of(A, typeof(*a), uabi_list);
>   	const struct intel_engine_cs *b =
> -		container_of((struct rb_node *)B, typeof(*b), uabi_node);
> +		container_of(B, typeof(*b), uabi_list);
>   
>   	if (uabi_classes[a->class] < uabi_classes[b->class])
>   		return -1;
> @@ -73,7 +72,7 @@ static int engine_cmp(void *priv, const struct list_head *A,
>   
>   static struct llist_node *get_engines(struct drm_i915_private *i915)
>   {
> -	return llist_del_all((struct llist_head *)&i915->uabi_engines);
> +	return llist_del_all(&i915->uabi_engines_llist);
>   }
>   
>   static void sort_engines(struct drm_i915_private *i915,
> @@ -83,9 +82,8 @@ static void sort_engines(struct drm_i915_private *i915,
>   
>   	llist_for_each_safe(pos, next, get_engines(i915)) {
>   		struct intel_engine_cs *engine =
> -			container_of((struct rb_node *)pos, typeof(*engine),
> -				     uabi_node);
> -		list_add((struct list_head *)&engine->uabi_node, engines);
> +			container_of(pos, typeof(*engine), uabi_llist);
> +		list_add(&engine->uabi_list, engines);
>   	}
>   	list_sort(NULL, engines, engine_cmp);
>   }
> @@ -213,8 +211,7 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
>   	p = &i915->uabi_engines.rb_node;
>   	list_for_each_safe(it, next, &engines) {
>   		struct intel_engine_cs *engine =
> -			container_of((struct rb_node *)it, typeof(*engine),
> -				     uabi_node);
> +			container_of(it, typeof(*engine), uabi_list);
>   
>   		if (intel_gt_has_unrecoverable_error(engine->gt))
>   			continue; /* ignore incomplete engines */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a8ce7239bc9..c8690d1d5e51 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -222,7 +222,22 @@ struct drm_i915_private {
>   		bool mchbar_need_disable;
>   	} gmch;
>   
> -	struct rb_root uabi_engines;
> +	/*
> +	 * Chaining user engines happens in multiple stages, starting with a
> +	 * simple lock-less linked list created by intel_engine_add_user(),
> +	 * which later gets sorted and converted to an intermediate regular
> +	 * list, just to be converted once again to its final rb tree structure
> +	 * in intel_engines_driver_register().
> +	 *
> +	 * Make sure to use the right iterator helper, depending on if the code
> +	 * in question runs before or after intel_engines_driver_register() --
> +	 * for_each_uabi_engine() can only be used afterwards!
> +	 */
> +	union {
> +		struct llist_head uabi_engines_llist;
> +		struct list_head uabi_engines_list;
> +		struct rb_root uabi_engines;
> +	};
>   	unsigned int engine_uabi_class_count[I915_LAST_UABI_ENGINE_CLASS + 1];
>   
>   	/* protects the irq masks */

Thanks again!

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

Regards,

Tvrtko