Re: [PATCH 2/3] drm/scheduler: Don't call wait_event_killable for signaled process.

[Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>]

On 04/30/2018 12:25 PM, Eric W. Biederman wrote:
> Christian König <ckoenig.leichtzumerken@gmail.com> writes:
>
>> Hi Eric,
>>
>> sorry for the late response, was on vacation last week.
>>
>> Am 26.04.2018 um 02:01 schrieb Eric W. Biederman:
>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:
>>>
>>>> On 04/25/2018 01:17 PM, Oleg Nesterov wrote:
>>>>> On 04/25, Andrey Grodzovsky wrote:
>>>>>> here (drm_sched_entity_fini) is also a bad idea, but we still want to be
>>>>>> able to exit immediately
>>>>>> and not wait for GPU jobs completion when the reason for reaching this code
>>>>>> is because of KILL
>>>>>> signal to the user process who opened the device file.
>>>>> Can you hook f_op->flush method?
>> THANKS! That sounds like a really good idea to me and we haven't investigated
>> into that direction yet.
> For the backwards compatibility concerns you cite below the flush method
> seems a much better place to introduce the wait.  You at least really
> will be in a process context for that.  Still might be in exit but at
> least you will be legitimately be in a process.
>
>>>> But this one is called for each task releasing a reference to the the file, so
>>>> not sure I see how this solves the problem.
>>> The big question is why do you need to wait during the final closing a
>>> file?
>> As always it's because of historical reasons. Initially user space pushed
>> commands directly to a hardware queue and when a processes finished we didn't
>> need to wait for anything.
>>
>> Then the GPU scheduler was introduced which delayed pushing the jobs to the
>> hardware queue to a later point in time.
>>
>> This wait was then added to maintain backward compability and not break
>> userspace (but see below).
> That make sense.
>
>>> The wait can be terminated so the wait does not appear to be simply a
>>> matter of correctness.
>> Well when the process is killed we don't care about correctness any more, we
>> just want to get rid of it as quickly as possible (OOM situation etc...).
>>
>> But it is perfectly possible that a process submits some render commands and
>> then calls exit() or terminates because of a SIGTERM, SIGINT etc.. In this case
>> we need to wait here to make sure that all rendering is pushed to the hardware
>> because the scheduler might need resources/settings from the file
>> descriptor.
>>
>> For example if you just remove that wait you could close firefox and get garbage
>> on the screen for a millisecond because the remaining rendering commands where
>> not executed.
>>
>> So what we essentially need is to distinct between a SIGKILL (which means stop
>> processing as soon as possible) and any other reason because then we don't want
>> to annoy the user with garbage on the screen (even if it's just for a few
>> milliseconds).
> I see a couple of issues.
>
> - Running the code in release rather than in flush.
>
> Using flush will catch every close so it should be more backwards
> compatible.  f_op->flush always runs in process context so looking at
> current makes sense.
>
> - Distinguishing between death by SIGKILL and other process exit deaths.
>
> In f_op->flush the code can test "((tsk->flags & PF_EXITING) &&
> (tsk->code == SIGKILL))" to see if it was SIGKILL that terminated
> the process.

What about Oleg's note not to rely on tsk->code == SIGKILL (still not 
clear why ?)
and that this entire check is racy (against what ?) ? Or is it relevant 
to .release hook
only ?

Andrey

>
> - Dealing with stuck queues (where this patchset came in).
>
> For stuck queues you are going to need a timeout instead of the current
> indefinite wait after PF_EXITING is set.  From what you have described a
> few milliseconds should be enough.  If PF_EXITING is not set you can
> still just make the wait killable and skip the timeout if that will give
> a better backwards compatible user experience.
>
> What can't be done is try and catch SIGKILL after a process has called
> do_exit.  A dead process is a dead process.
>
> Eric