[RFC 1/4] drm/sched: Add locking to drm_sched_entity_modify_sched

[Christian König]

Am 09.09.24 um 15:27 schrieb Tvrtko Ursulin:
>
> On 09/09/2024 13:46, Philipp Stanner wrote:
>> On Mon, 2024-09-09 at 13:37 +0100, Tvrtko Ursulin wrote:
>>> [SNIP]
>>>>> That could also be a great opportunity for improving the lock
>>>>> naming:
>>>>
>>>> Well that comment made me laugh because I point out the same when
>>>> the
>>>> scheduler came out ~8years ago and nobody cared about it since
>>>> then.
>>>>
>>>> But yeah completely agree :)
>>>
>>> Maybe, but we need to keep in sight the fact some of these fixes may
>>> be
>>> good to backport. In which case re-naming exercises are best left to
>>> follow.
>>
>> My argument basically. It's good if fixes and other improvements are
>> separated, in general, unless there is a practical / good reason not
>> to.
>
> Ah cool, I am happy to add follow up patches after the fixes.

+1

>
>>> Also..
>>>
>>>>> void drm_sched_rq_update_fifo(struct drm_sched_entity *entity,
>>>>> ktime_t
>>>>> ts)
>>>>> {
>>>>>      /*
>>>>>       * Both locks need to be grabbed, one to protect from entity-
>>>>>> rq
>>>>> change
>>>>>       * for entity from within concurrent
>>>>> drm_sched_entity_select_rq
>>>>> and the
>>>>>       * other to update the rb tree structure.
>>>>>       */
>>>>>      spin_lock(&entity->rq_lock);
>>>>>      spin_lock(&entity->rq->lock);
>>>
>>> .. I agree this is quite unredable and my initial reaction was a
>>> similar
>>> ugh. However.. What names would you guys suggest and for what to make
>>> this better and not lessen the logic of naming each individually?
>>
>> According to the documentation, drm_sched_rq.lock does not protect the
>> entire runque, but "@lock: to modify the entities list."
>>
>> So I would keep drm_sched_entity.rq_lock as it is, because it indeed
>> protects the entire runqueue.
>
> Agreed on entity->rq_lock.

I would just name that lock since it should be a protection of fields in 
the drm_sched_entity structure.

That those fields are the rq and priority member should not necessary 
have an influence on the name of the lock protecting it.

Only when we have multiple locks in the same structure then we need to 
start giving them distinct names.

>
>> And drm_sched_rq.lock could be named "entities_lock" or
>> "entities_list_lock" or something. That's debatable, but it should be
>> something that highlights that this lock is not for locking the entire
>> runque as the one in the entity apparently is.
>
> AFAICT it also protects rq->current_entity and rq->rb_tree_root in 
> which case it is a bit more tricky. Only rq->sched is outside its 
> scope. Hm. Maybe just re-arrange the struct to be like:
>
> struct drm_sched_rq {
>     struct drm_gpu_scheduler    *sched;
>
>     spinlock_t            lock;
>     /* Following members are protected by the @lock: */
>     struct list_head        entities;
>     struct drm_sched_entity        *current_entity;
>     struct rb_root_cached        rb_tree_root;
> };
>
> I have no ideas for better naming. But this would be inline with 
> Christian's suggestion for tidying the order in drm_sched_entity.

+1

Yeah I mean see the other structure we have in DRM and general Linux 
kernel. The stuff that is static is usually grouped together since that 
is good for cache locality and documentation at the same time.

>
> I am also not sure what is the point of setting rq->current_entity in 
> drm_sched_rq_select_entity_fifo().

No idea either, Luben could answer that.

Christian.

>
> Regards,
>
> Tvrtko
>
>>
>>
>> Cheers,
>> P.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>> [...]
>>>>>
>>>>>
>>>>> P.
>>>>>
>>>>>
>>>>>> Then audit the code if all users of rq and priority actually
>>>>>> hold the
>>>>>> correct locks while reading and writing them.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> P.
>>>>>>>
>>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>>>>> Fixes: b37aced31eb0 ("drm/scheduler: implement a function
>>>>>>>> to
>>>>>>>> modify
>>>>>>>> sched list")
>>>>>>>> Cc: Christian König <christian.koenig at amd.com>
>>>>>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>>>>>> Cc: Luben Tuikov <ltuikov89 at gmail.com>
>>>>>>>> Cc: Matthew Brost <matthew.brost at intel.com>
>>>>>>>> Cc: David Airlie <airlied at gmail.com>
>>>>>>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>>>>>>> Cc: dri-devel at lists.freedesktop.org
>>>>>>>> Cc: <stable at vger.kernel.org> # v5.7+
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/scheduler/sched_entity.c | 2 ++
>>>>>>>>     1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> index 58c8161289fe..ae8be30472cd 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> @@ -133,8 +133,10 @@ void
>>>>>>>> drm_sched_entity_modify_sched(struct
>>>>>>>> drm_sched_entity *entity,
>>>>>>>>     {
>>>>>>>>         WARN_ON(!num_sched_list || !sched_list);
>>>>>>>> +    spin_lock(&entity->rq_lock);
>>>>>>>>         entity->sched_list = sched_list;
>>>>>>>>         entity->num_sched_list = num_sched_list;
>>>>>>>> +    spin_unlock(&entity->rq_lock);
>>>>>>>>     }
>>>>>>>>     EXPORT_SYMBOL(drm_sched_entity_modify_sched);
>>>>
>>>
>>