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

2018-08-14 Thread Andrey Grodzovsky
On 08/14/2018 11:26 AM, Christian König wrote: Am 14.08.2018 um 17:17 schrieb Andrey Grodzovsky: I assume that this is the only code change and no locks are taken in drm_sched_entity_push_job - What are you talking about? You surely now take looks in drm_sched_entity_push_job(): +    

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

2018-08-14 Thread Christian König
Am 14.08.2018 um 17:17 schrieb Andrey Grodzovsky: I assume that this is the only code change and no locks are taken in drm_sched_entity_push_job - What are you talking about? You surely now take looks in drm_sched_entity_push_job(): +    spin_lock(>rq_lock); +    entity->last_user =

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

2018-08-14 Thread Andrey Grodzovsky
I assume that this is the only code change and no locks are taken in drm_sched_entity_push_job - What happens if process A runs drm_sched_entity_push_job after this code was executed from the  (dying) process B and there are still jobs in the queue (the wait_event terminated prematurely),

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

2018-08-14 Thread Christian König
I would rather like to avoid taking the lock in the hot path. How about this: /* For killed process disable any more IBs enqueue right now */     last_user = cmpxchg(>last_user, current->group_leader, NULL); if ((!last_user || last_user == current->group_leader) &&    

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

2018-08-13 Thread Andrey Grodzovsky
Attached. If the general idea in the patch is OK I can think of a test (and maybe add to libdrm amdgpu tests) to actually simulate this scenario with 2 forked concurrent processes working on same entity's job queue when one is dying while the other keeps pushing to the same queue. For now I

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

2018-08-10 Thread Andrey Grodzovsky
I can take care of this. Andrey On 08/10/2018 09:27 AM, Christian König wrote: Crap, yeah indeed that needs to be protected by some lock. Going to prepare a patch for that, Christian. Am 09.08.2018 um 21:49 schrieb Andrey Grodzovsky: Reviewed-by: Andrey Grodzovsky But I still  have

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

2018-08-10 Thread Christian König
Crap, yeah indeed that needs to be protected by some lock. Going to prepare a patch for that, Christian. Am 09.08.2018 um 21:49 schrieb Andrey Grodzovsky: Reviewed-by: Andrey Grodzovsky But I still  have questions about entity->last_user (didn't notice this before) - Looks to me there

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

2018-08-09 Thread Andrey Grodzovsky
Reviewed-by: Andrey Grodzovsky 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

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

2018-08-06 Thread Nayan Deshmukh
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 < nayan26deshm...@gmail.com> Nayan On Mon, Aug 6, 2018 at 7:43 PM Christian König < ckoenig.leichtzumer...@gmail.com> wrote: > Ping. Any

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

2018-08-06 Thread Christian König
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 --- drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c