<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 14.08.2018 um 17:17 schrieb Andrey Grodzovsky: </div> <blockquote type="cite" cite="mid:2f197b16-4c60-6b6a-0b36-7e60b9e5fc33@amd.com"> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> I assume that this is the only code change and no locks are taken in drm_sched_entity_push_job - </blockquote> What are you talking about? You surely now take looks in drm_sched_entity_push_job(): <blockquote type="cite">+ spin_lock(&entity->rq_lock); + entity->last_user = current->group_leader; + if (list_empty(&entity->list)) </blockquote> <blockquote type="cite" cite="mid:2f197b16-4c60-6b6a-0b36-7e60b9e5fc33@amd.com"> 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), the entity already removed from rq , but bool 'first' in drm_sched_entity_push_job will return false and so the entity will not be reinserted back into rq entity list and no wake up trigger will happen for process A pushing a new job. </blockquote> Thought about this as well, but in this case I would say: Shit happens! The dying process did some command submission and because of this the entity was killed as well when the process died and that is legitimate. <blockquote type="cite" cite="mid:2f197b16-4c60-6b6a-0b36-7e60b9e5fc33@amd.com"> Another issue bellow - Andrey <div class="moz-cite-prefix">On 08/14/2018 03:05 AM, Christian König wrote: </div> <blockquote type="cite" cite="mid:0fa473f5-155a-223e-fbb6-37147fd47a17@gmail.com"> <div class="moz-cite-prefix">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(&entity->last_user, current->group_leader, NULL); if ((!last_user || last_user == current->group_leader) && (current->flags & PF_EXITING) && (current->exit_code == SIGKILL)) { grab_lock(); drm_sched_rq_remove_entity(entity->rq, entity); if (READ_ONCE(&entity->last_user) != NULL) </div> </blockquote> This condition is true because just exactly now process A did drm_sched_entity_push_job->WRITE_ONCE(entity->last_user, current->group_leader); and so the line bellow executed and entity reinserted into rq. Let's say also that the entity job queue is empty now. For process A bool 'first' will be true and hence also drm_sched_entity_push_job->drm_sched_rq_add_entity(entity->rq, entity) will take place causing double insertion of the entity queue into rq list. </blockquote> Calling drm_sched_rq_add_entity() is harmless, it is protected against double insertion. But thinking more about it your idea of adding a killed or finished flag becomes more and more appealing to have a consistent handling here. Christian. <blockquote type="cite" cite="mid:2f197b16-4c60-6b6a-0b36-7e60b9e5fc33@amd.com"> Andrey <blockquote type="cite" cite="mid:0fa473f5-155a-223e-fbb6-37147fd47a17@gmail.com"> <div class="moz-cite-prefix"> drm_sched_rq_add_entity(entity->rq, entity); drop_lock(); } Christian. Am 13.08.2018 um 18:43 schrieb Andrey Grodzovsky: </div> <blockquote type="cite" cite="mid:82109a00-aebf-1e5f-5346-eef541a361df@amd.com"> 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 only tested it with normal boot and ruining multiple glxgears concurrently - which doesn't really test this code path since i think each of them works on it's own FD. Andrey <div class="moz-cite-prefix">On 08/10/2018 09:27 AM, Christian König wrote: </div> <blockquote type="cite" cite="mid:5bf40a54-18f9-98fd-a3df-dd0b8da0a424@gmail.com"> <div class="moz-cite-prefix">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: </div> <blockquote type="cite" cite="mid:54621fc1-7246-f1bf-26bb-a16c4daf249f@amd.com"> Reviewed-by: Andrey Grodzovsky <a class="moz-txt-link-rfc2396E" href="mailto:andrey.grodzovsky@amd.com" moz-do-not-send="true"><andrey.grodzovsky@amd.com></a> 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 <div class="moz-cite-prefix">On 08/06/2018 10:18 AM, Nayan Deshmukh wrote: </div> <blockquote type="cite" cite="mid:CAFd4ddzyvHPHepAgs=mjyWVj0WDV_pQbE9x7aHwNZ_zcME6fqQ@mail.gmail.com"> <div dir="ltr"> <div> <div>I forgot about this since we started discussing possible scenarios of processes and threads. </div> In any case, this check is redundant. Acked-by: Nayan Deshmukh <<a href="mailto:nayan26deshmukh@gmail.com" moz-do-not-send="true">nayan26deshmukh@gmail.com</a>> </div> Nayan </div> <div class="gmail_quote"> <div dir="ltr">On Mon, Aug 6, 2018 at 7:43 PM Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com" moz-do-not-send="true">ckoenig.leichtzumerken@gmail.com</a>> wrote: </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <<a href="mailto:christian.koenig@amd.com" target="_blank" moz-do-not-send="true">christian.koenig@amd.com</a>> > --- > 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); </blockquote> </div> </blockquote> </blockquote> </blockquote> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ dri-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org" moz-do-not-send="true">dri-devel@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" moz-do-not-send="true">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </pre> </blockquote> </blockquote> </blockquote> </body> </html>