[PATCH 2/2] drm/scheduler: Remove obsolete spinlock.

[Andrey.Grodzovsky@xxxxxxx (Andrey Grodzovsky)]

So are you saying that you expect this to  be already in code for any 
usage of drm_sched_fence_create and drm_sched_entity_push_job ?

lock()

drm_sched_fence_create()

... (some code)

drm_sched_entity_push_job()

unlock()


Andrey

On 05/16/2018 07:23 AM, Christian König wrote:
> drm_sched_fence_create() assigns a sequence number to the fence it 
> creates.
>
> Now drm_sched_fence_create() is called by drm_sched_job_init() to 
> initialize the jobs we want to push on the scheduler queue.
>
> When you now call drm_sched_entity_push_job() without a protecting 
> lock it can happen that you push two (or even more) job with reversed 
> sequence numbers.
>
> Since the sequence numbers are used to determine job completion order 
> reversing them can seriously mess things up.
>
> So the spin lock should be superfluous, if it isn't we have a much 
> larger bug we need to fix.
>
> Christian.
>
> Am 16.05.2018 um 13:15 schrieb Andrey Grodzovsky:
>> Can you please elaborate more, maybe give an example - I don't 
>> understand yet the problematic scenario.
>>
>> Andrey
>>
>>
>> On 05/16/2018 02:50 AM, Christian König wrote:
>>> No, that spinlock is indeed incorrect. I
>>>
>>> See even when we protect the spsc queue with a spinlock that doesn't 
>>> make it correct. It can happen that the jobs pushed to the queue are 
>>> reversed in their sequence order and that can cause severe problems 
>>> in the memory management.
>>>
>>> Christian.
>>>
>>> Am 16.05.2018 um 05:33 schrieb Grodzovsky, Andrey:
>>>> Yeah, that what I am not sure about... It's lockless in a sense of 
>>>> single producer single consumer but not for multiple concurrent 
>>>> producers... So now I think this spinlock should stay there... It 
>>>> just looked useless to me at first sight...
>>>>
>>>> Andrey
>>>>
>>>> ________________________________________
>>>> From: Zhou, David(ChunMing)
>>>> Sent: 15 May 2018 23:04:44
>>>> To: Grodzovsky, Andrey; amd-gfx at lists.freedesktop.org; 
>>>> dri-devel at lists.freedesktop.org
>>>> Cc: Koenig, Christian
>>>> Subject: Re: [PATCH 2/2] drm/scheduler: Remove obsolete spinlock.
>>>>
>>>>
>>>>
>>>> On 2018å¹´05æ??16æ?¥ 03:31, Andrey Grodzovsky wrote:
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>>>> ---
>>>>> Â Â  drivers/gpu/drm/scheduler/gpu_scheduler.c | 4 ----
>>>>>    include/drm/gpu_scheduler.h               | 1 -
>>>>> Â Â  2 files changed, 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
>>>>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> index 1f1dd70..2569a63 100644
>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> @@ -140,7 +140,6 @@ int drm_sched_entity_init(struct 
>>>>> drm_gpu_scheduler *sched,
>>>>> Â Â Â Â Â Â  entity->last_scheduled = NULL;
>>>>>
>>>>> Â Â Â Â Â Â  spin_lock_init(&entity->rq_lock);
>>>>> -Â Â Â Â  spin_lock_init(&entity->queue_lock);
>>>>> Â Â Â Â Â Â  spsc_queue_init(&entity->job_queue);
>>>>>
>>>>> Â Â Â Â Â Â  atomic_set(&entity->fence_seq, 0);
>>>>> @@ -424,11 +423,8 @@ void drm_sched_entity_push_job(struct 
>>>>> drm_sched_job *sched_job,
>>>>>
>>>>> Â Â Â Â Â Â  trace_drm_sched_job(sched_job, entity);
>>>>>
>>>>> -Â Â Â Â  spin_lock(&entity->queue_lock);
>>>>> Â Â Â Â Â Â  first = spsc_queue_push(&entity->job_queue, 
>>>>> &sched_job->queue_node);
>>>>>
>>>>> -Â Â Â Â  spin_unlock(&entity->queue_lock);
>>>> Is your spsc safely to be added simultaneously?
>>>>
>>>> Regards,
>>>> David Zhou
>>>>> -
>>>>> Â Â Â Â Â Â  /* first job wakes up scheduler */
>>>>> Â Â Â Â Â Â  if (first) {
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â  /* Add the entity to the run queue */
>>>>> diff --git a/include/drm/gpu_scheduler.h 
>>>>> b/include/drm/gpu_scheduler.h
>>>>> index 350a62c..683eb65 100644
>>>>> --- a/include/drm/gpu_scheduler.h
>>>>> +++ b/include/drm/gpu_scheduler.h
>>>>> @@ -56,7 +56,6 @@ struct drm_sched_entity {
>>>>>        spinlock_t                      rq_lock;
>>>>>        struct drm_gpu_scheduler        *sched;
>>>>>
>>>>> -     spinlock_t                      queue_lock;
>>>>>        struct spsc_queue               job_queue;
>>>>>
>>>>>        atomic_t                        fence_seq;
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>