[PATCH] drm/scheduler: Remove entity->rq NULL check

[Andrey Grodzovsky]

Reviewed-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>


But I still  have questions about entity->last_user (didn't notice this 
before) -

Looks to me there is a race condition with it's current usage, let's say 
process A was preempted after doing drm_sched_entity_flush->cmpxchg(...)

now process B working on same entity (forked) is inside 
drm_sched_entity_push_job, he writes his PID to entity->last_user and also

executes drm_sched_rq_add_entity. Now process A runs again and execute 
drm_sched_rq_remove_entity inadvertently causing process B removal

from it's scheduler rq.

Looks to me like instead we should lock together entity->last_user 
accesses and adds/removals of entity to the rq.

Andrey


On 08/06/2018 10:18 AM, Nayan Deshmukh wrote:
> I forgot about this since we started discussing possible scenarios of 
> processes and threads.
>
> In any case, this check is redundant. Acked-by: Nayan Deshmukh 
> <nayan26deshmukh at gmail.com <mailto:nayan26deshmukh at gmail.com>>
>
> Nayan
>
> On Mon, Aug 6, 2018 at 7:43 PM Christian König 
> <ckoenig.leichtzumerken at gmail.com 
> <mailto:ckoenig.leichtzumerken at gmail.com>> wrote:
>
>     Ping. Any objections to that?
>
>     Christian.
>
>     Am 03.08.2018 um 13:08 schrieb Christian König:
>     > That is superflous now.
>     >
>     > Signed-off-by: Christian König <christian.koenig at amd.com
>     <mailto:christian.koenig at amd.com>>
>     > ---
>     >   drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 -----
>     >   1 file changed, 5 deletions(-)
>     >
>     > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>     b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>     > index 85908c7f913e..65078dd3c82c 100644
>     > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>     > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>     > @@ -590,11 +590,6 @@ void drm_sched_entity_push_job(struct
>     drm_sched_job *sched_job,
>     >       if (first) {
>     >               /* Add the entity to the run queue */
>     >               spin_lock(&entity->rq_lock);
>     > -             if (!entity->rq) {
>     > -                     DRM_ERROR("Trying to push to a killed
>     entity\n");
>     > -  spin_unlock(&entity->rq_lock);
>     > -                     return;
>     > -             }
>     >               drm_sched_rq_add_entity(entity->rq, entity);
>     >               spin_unlock(&entity->rq_lock);
>     >               drm_sched_wakeup(entity->rq->sched);
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180809/bef9a17c/attachment-0001.html>