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 29/09/2023 12:00, Tvrtko Ursulin wrote:
> 
> 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>

Patches pushed to drm-intel-gt-next.

Regards,

Tvrtko