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

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

[Michel Dänzer]

Adding the dri-devel list, since this is driver independent code.


On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
> Avoid calling wait_event_killable when you are possibly being called
> from get_signal routine since in that case you end up in a deadlock
> where you are alreay blocked in singla processing any trying to wait

Multiple typos here, "[...] already blocked in signal processing and [...]"?


> on a new signal.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 088ff2b..09fd258 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>  		return;
>  	/**
>  	 * The client will not queue more IBs during this fini, consume existing
> -	 * queued IBs or discard them on SIGKILL
> +	 * queued IBs or discard them when in death signal state since
> +	 * wait_event_killable can't receive signals in that state.
>  	*/
> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
> +	if (current->flags & PF_SIGNALED)
>  		entity->fini_status = -ERESTARTSYS;
>  	else
>  		entity->fini_status = wait_event_killable(sched->job_scheduled,
> 


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

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

[Andrey Grodzovsky]

On 04/24/2018 11:46 AM, Michel Dänzer wrote:
> Adding the dri-devel list, since this is driver independent code.

Thanks, so many addresses that this one slipped out...
>
>
> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>> Avoid calling wait_event_killable when you are possibly being called
>> from get_signal routine since in that case you end up in a deadlock
>> where you are alreay blocked in singla processing any trying to wait
> Multiple typos here, "[...] already blocked in signal processing and [...]"?

I don't understand where are the typos.

Andrey

>
>
>> on a new signal.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index 088ff2b..09fd258 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>   		return;
>>   	/**
>>   	 * The client will not queue more IBs during this fini, consume existing
>> -	 * queued IBs or discard them on SIGKILL
>> +	 * queued IBs or discard them when in death signal state since
>> +	 * wait_event_killable can't receive signals in that state.
>>   	*/
>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>> +	if (current->flags & PF_SIGNALED)
>>   		entity->fini_status = -ERESTARTSYS;
>>   	else
>>   		entity->fini_status = wait_event_killable(sched->job_scheduled,
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Andrey Grodzovsky]

On 04/24/2018 11:46 AM, Michel Dänzer wrote:
> Adding the dri-devel list, since this is driver independent code.

Thanks, so many addresses that this one slipped out...
>
>
> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>> Avoid calling wait_event_killable when you are possibly being called
>> from get_signal routine since in that case you end up in a deadlock
>> where you are alreay blocked in singla processing any trying to wait
> Multiple typos here, "[...] already blocked in signal processing and [...]"?

I don't understand where are the typos.

Andrey

>
>
>> on a new signal.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index 088ff2b..09fd258 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>   		return;
>>   	/**
>>   	 * The client will not queue more IBs during this fini, consume existing
>> -	 * queued IBs or discard them on SIGKILL
>> +	 * queued IBs or discard them when in death signal state since
>> +	 * wait_event_killable can't receive signals in that state.
>>   	*/
>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>> +	if (current->flags & PF_SIGNALED)
>>   		entity->fini_status = -ERESTARTSYS;
>>   	else
>>   		entity->fini_status = wait_event_killable(sched->job_scheduled,
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
> 
> Adding the dri-devel list, since this is driver independent code.
> 
> 
> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
> > Avoid calling wait_event_killable when you are possibly being called
> > from get_signal routine since in that case you end up in a deadlock
> > where you are alreay blocked in singla processing any trying to wait
> 
> Multiple typos here, "[...] already blocked in signal processing and [...]"?
> 
> 
> > on a new signal.
> > 
> > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > ---
> >  drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > index 088ff2b..09fd258 100644
> > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
> >  		return;
> >  	/**
> >  	 * The client will not queue more IBs during this fini, consume existing
> > -	 * queued IBs or discard them on SIGKILL
> > +	 * queued IBs or discard them when in death signal state since
> > +	 * wait_event_killable can't receive signals in that state.
> >  	*/
> > -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
> > +	if (current->flags & PF_SIGNALED)

You want fatal_signal_pending() here, instead of inventing your own broken
version.
> >  		entity->fini_status = -ERESTARTSYS;
> >  	else
> >  		entity->fini_status = wait_event_killable(sched->job_scheduled,

But really this smells like a bug in wait_event_killable, since
wait_event_interruptible does not suffer from the same bug. It will return
immediately when there's a signal pending.

I think this should be fixed in core code, not papered over in some
subsystem.
-Daniel

> > 
> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Eric W. Biederman]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>> 
>> Adding the dri-devel list, since this is driver independent code.
>> 
>> 
>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>> > Avoid calling wait_event_killable when you are possibly being called
>> > from get_signal routine since in that case you end up in a deadlock
>> > where you are alreay blocked in singla processing any trying to wait
>> 
>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>> 
>> 
>> > on a new signal.
>> > 
>> > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> > ---
>> >  drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> > index 088ff2b..09fd258 100644
>> > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> > @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>> >  		return;
>> >  	/**
>> >  	 * The client will not queue more IBs during this fini, consume existing
>> > -	 * queued IBs or discard them on SIGKILL
>> > +	 * queued IBs or discard them when in death signal state since
>> > +	 * wait_event_killable can't receive signals in that state.
>> >  	*/
>> > -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>> > +	if (current->flags & PF_SIGNALED)
>
> You want fatal_signal_pending() here, instead of inventing your own broken
> version.
>> >  		entity->fini_status = -ERESTARTSYS;
>> >  	else
>> >  		entity->fini_status = wait_event_killable(sched->job_scheduled,
>
> But really this smells like a bug in wait_event_killable, since
> wait_event_interruptible does not suffer from the same bug. It will return
> immediately when there's a signal pending.
>
> I think this should be fixed in core code, not papered over in some
> subsystem.

PF_SIGNALED does not mean a signal has been sent.  PF_SIGNALED means
the process was killed by a signal.

Neither of interruptible or killable makes sense after the process has
been killed.

Eric

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

[Andrey Grodzovsky]

On 04/24/2018 03:44 PM, Daniel Vetter wrote:
> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>> Adding the dri-devel list, since this is driver independent code.
>>
>>
>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>> Avoid calling wait_event_killable when you are possibly being called
>>> from get_signal routine since in that case you end up in a deadlock
>>> where you are alreay blocked in singla processing any trying to wait
>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>
>>
>>> on a new signal.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> index 088ff2b..09fd258 100644
>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>   		return;
>>>   	/**
>>>   	 * The client will not queue more IBs during this fini, consume existing
>>> -	 * queued IBs or discard them on SIGKILL
>>> +	 * queued IBs or discard them when in death signal state since
>>> +	 * wait_event_killable can't receive signals in that state.
>>>   	*/
>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>> +	if (current->flags & PF_SIGNALED)
> You want fatal_signal_pending() here, instead of inventing your own broken
> version.

I rely on current->flags & PF_SIGNALED because this being set from 
within get_signal,
meaning I am within signal processing  in which case I want to avoid any 
signal based wait for that task,
 From what i see in the code, task_struct.pending.signal is being set 
for other threads in same
group (zap_other_threads) or for other scenarios, those task are still 
able to receive signals
so calling wait_event_killable there will not have problem.
>>>   		entity->fini_status = -ERESTARTSYS;
>>>   	else
>>>   		entity->fini_status = wait_event_killable(sched->job_scheduled,
> But really this smells like a bug in wait_event_killable, since
> wait_event_interruptible does not suffer from the same bug. It will return
> immediately when there's a signal pending.

Even when wait_event_interruptible is called as following - 
...->do_signal->get_signal->....->wait_event_interruptible ?
I haven't tried it but wait_event_interruptible is very much alike to 
wait_event_killable so I would assume it will also
not be interrupted if called like that. (Will give it a try just out of 
curiosity anyway)

Andrey

>
> I think this should be fixed in core code, not papered over in some
> subsystem.
> -Daniel
>
>>
>> -- 
>> Earthling Michel Dänzer               |               http://www.amd.com
>> Libre software enthusiast             |             Mesa and X developer
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Eric W. Biederman]

Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:

> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
>> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>>> Adding the dri-devel list, since this is driver independent code.
>>>
>>>
>>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>>> Avoid calling wait_event_killable when you are possibly being called
>>>> from get_signal routine since in that case you end up in a deadlock
>>>> where you are alreay blocked in singla processing any trying to wait
>>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>>
>>>
>>>> on a new signal.
>>>>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> index 088ff2b..09fd258 100644
>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>>   		return;
>>>>   	/**
>>>>   	 * The client will not queue more IBs during this fini, consume existing
>>>> -	 * queued IBs or discard them on SIGKILL
>>>> +	 * queued IBs or discard them when in death signal state since
>>>> +	 * wait_event_killable can't receive signals in that state.
>>>>   	*/
>>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>>> +	if (current->flags & PF_SIGNALED)
>> You want fatal_signal_pending() here, instead of inventing your own broken
>> version.
>
> I rely on current->flags & PF_SIGNALED because this being set from
> within get_signal,

It doesn't mean that.  Unless you are called by do_coredump (you
aren't).  The closing of files does not happen in do_coredump.
Which means you are being called from do_exit.
In fact you are being called after exit_files which closes
the files.  The actual __fput processing happens in task_work_run.

> meaning I am within signal processing  in which case I want to avoid
> any signal based wait for that task,
> From what i see in the code, task_struct.pending.signal is being set
> for other threads in same
> group (zap_other_threads) or for other scenarios, those task are still
> able to receive signals
> so calling wait_event_killable there will not have problem.

Excpet that you are geing called after from do_exit and after exit_files
which is after exit_signal.  Which means that PF_EXITING has been set.
Which implies that the kernel signal handling machinery has already
started being torn down.

Not as much as I would like to happen at that point as we are still
left with some old CLONE_PTHREAD messes in the code that need to be
cleaned up.

Still given the fact you are task_work_run it is quite possible even
release_task has been run on that task before the f_op->release method
is called.  So you simply can not count on signals working.

Which in practice leaves a timeout for ending your wait.  That code can
legitimately be in a context that is neither interruptible nor killable.

>>>>   		entity->fini_status = -ERESTARTSYS;
>>>>   	else
>>>>   		entity->fini_status = wait_event_killable(sched->job_scheduled,
>> But really this smells like a bug in wait_event_killable, since
>> wait_event_interruptible does not suffer from the same bug. It will return
>> immediately when there's a signal pending.
>
> Even when wait_event_interruptible is called as following - 
> ...->do_signal->get_signal->....->wait_event_interruptible ?
> I haven't tried it but wait_event_interruptible is very much alike to
> wait_event_killable so I would assume it will also
> not be interrupted if called like that. (Will give it a try just out
> of curiosity anyway)

As PF_EXITING is set want_signal should fail and the signal state of the
task should not be updatable by signals.

Eric

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

[Andrey Grodzovsky]

On 04/24/2018 05:21 PM, Eric W. Biederman wrote:
> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:
>
>> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
>>> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>>>> Adding the dri-devel list, since this is driver independent code.
>>>>
>>>>
>>>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>>>> Avoid calling wait_event_killable when you are possibly being called
>>>>> from get_signal routine since in that case you end up in a deadlock
>>>>> where you are alreay blocked in singla processing any trying to wait
>>>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>>>
>>>>
>>>>> on a new signal.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> index 088ff2b..09fd258 100644
>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>>>    		return;
>>>>>    	/**
>>>>>    	 * The client will not queue more IBs during this fini, consume existing
>>>>> -	 * queued IBs or discard them on SIGKILL
>>>>> +	 * queued IBs or discard them when in death signal state since
>>>>> +	 * wait_event_killable can't receive signals in that state.
>>>>>    	*/
>>>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>>>> +	if (current->flags & PF_SIGNALED)
>>> You want fatal_signal_pending() here, instead of inventing your own broken
>>> version.
>> I rely on current->flags & PF_SIGNALED because this being set from
>> within get_signal,
> It doesn't mean that.  Unless you are called by do_coredump (you
> aren't).

Looking in latest code here
https://elixir.bootlin.com/linux/v4.17-rc2/source/kernel/signal.c#L2449
i see that current->flags |= PF_SIGNALED; is out side of
if (sig_kernel_coredump(signr)) {...} scope

Andrey

> The closing of files does not happen in do_coredump.
> Which means you are being called from do_exit.
> In fact you are being called after exit_files which closes
> the files.  The actual __fput processing happens in task_work_run.
>
>> meaning I am within signal processing  in which case I want to avoid
>> any signal based wait for that task,
>>  From what i see in the code, task_struct.pending.signal is being set
>> for other threads in same
>> group (zap_other_threads) or for other scenarios, those task are still
>> able to receive signals
>> so calling wait_event_killable there will not have problem.
> Excpet that you are geing called after from do_exit and after exit_files
> which is after exit_signal.  Which means that PF_EXITING has been set.
> Which implies that the kernel signal handling machinery has already
> started being torn down.
>
> Not as much as I would like to happen at that point as we are still
> left with some old CLONE_PTHREAD messes in the code that need to be
> cleaned up.
>
> Still given the fact you are task_work_run it is quite possible even
> release_task has been run on that task before the f_op->release method
> is called.  So you simply can not count on signals working.
>
> Which in practice leaves a timeout for ending your wait.  That code can
> legitimately be in a context that is neither interruptible nor killable.
>
>>>>>    		entity->fini_status = -ERESTARTSYS;
>>>>>    	else
>>>>>    		entity->fini_status = wait_event_killable(sched->job_scheduled,
>>> But really this smells like a bug in wait_event_killable, since
>>> wait_event_interruptible does not suffer from the same bug. It will return
>>> immediately when there's a signal pending.
>> Even when wait_event_interruptible is called as following -
>> ...->do_signal->get_signal->....->wait_event_interruptible ?
>> I haven't tried it but wait_event_interruptible is very much alike to
>> wait_event_killable so I would assume it will also
>> not be interrupted if called like that. (Will give it a try just out
>> of curiosity anyway)
> As PF_EXITING is set want_signal should fail and the signal state of the
> task should not be updatable by signals.
>
> Eric
>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Tue, Apr 24, 2018 at 05:02:40PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
> > On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
> > > Adding the dri-devel list, since this is driver independent code.
> > > 
> > > 
> > > On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
> > > > Avoid calling wait_event_killable when you are possibly being called
> > > > from get_signal routine since in that case you end up in a deadlock
> > > > where you are alreay blocked in singla processing any trying to wait
> > > Multiple typos here, "[...] already blocked in signal processing and [...]"?
> > > 
> > > 
> > > > on a new signal.
> > > > 
> > > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > > ---
> > > >   drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
> > > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > index 088ff2b..09fd258 100644
> > > > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
> > > >   		return;
> > > >   	/**
> > > >   	 * The client will not queue more IBs during this fini, consume existing
> > > > -	 * queued IBs or discard them on SIGKILL
> > > > +	 * queued IBs or discard them when in death signal state since
> > > > +	 * wait_event_killable can't receive signals in that state.
> > > >   	*/
> > > > -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
> > > > +	if (current->flags & PF_SIGNALED)
> > You want fatal_signal_pending() here, instead of inventing your own broken
> > version.
> 
> I rely on current->flags & PF_SIGNALED because this being set from within
> get_signal,
> meaning I am within signal processing  in which case I want to avoid any
> signal based wait for that task,
> From what i see in the code, task_struct.pending.signal is being set for
> other threads in same
> group (zap_other_threads) or for other scenarios, those task are still able
> to receive signals
> so calling wait_event_killable there will not have problem.
> > > >   		entity->fini_status = -ERESTARTSYS;
> > > >   	else
> > > >   		entity->fini_status = wait_event_killable(sched->job_scheduled,
> > But really this smells like a bug in wait_event_killable, since
> > wait_event_interruptible does not suffer from the same bug. It will return
> > immediately when there's a signal pending.
> 
> Even when wait_event_interruptible is called as following -
> ...->do_signal->get_signal->....->wait_event_interruptible ?
> I haven't tried it but wait_event_interruptible is very much alike to
> wait_event_killable so I would assume it will also
> not be interrupted if called like that. (Will give it a try just out of
> curiosity anyway)

wait_event_killabel doesn't check for fatal_signal_pending before calling
schedule, so definitely has a nice race there.

But if you're sure that you really need to check PF_SIGNALED, then I'm
honestly not clear on what you're trying to pull off here. Your sparse
explanation of what happens isn't enough, since I have no idea how you can
get from get_signal() to the above wait_event_killable callsite.
-Daniel

> 
> Andrey
> 
> > 
> > I think this should be fixed in core code, not papered over in some
> > subsystem.
> > -Daniel
> > 
> > > 
> > > -- 
> > > Earthling Michel Dänzer               |               http://www.amd.com
> > > Libre software enthusiast             |             Mesa and X developer
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric W. Biederman]

Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:

> On 04/24/2018 05:21 PM, Eric W. Biederman wrote:
>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:
>>
>>> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
>>>> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>>>>> Adding the dri-devel list, since this is driver independent code.
>>>>>
>>>>>
>>>>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>>>>> Avoid calling wait_event_killable when you are possibly being called
>>>>>> from get_signal routine since in that case you end up in a deadlock
>>>>>> where you are alreay blocked in singla processing any trying to wait
>>>>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>>>>
>>>>>
>>>>>> on a new signal.
>>>>>>
>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>> index 088ff2b..09fd258 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>>>>    		return;
>>>>>>    	/**
>>>>>>    	 * The client will not queue more IBs during this fini, consume existing
>>>>>> -	 * queued IBs or discard them on SIGKILL
>>>>>> +	 * queued IBs or discard them when in death signal state since
>>>>>> +	 * wait_event_killable can't receive signals in that state.
>>>>>>    	*/
>>>>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>>>>> +	if (current->flags & PF_SIGNALED)
>>>> You want fatal_signal_pending() here, instead of inventing your own broken
>>>> version.
>>> I rely on current->flags & PF_SIGNALED because this being set from
>>> within get_signal,
>> It doesn't mean that.  Unless you are called by do_coredump (you
>> aren't).
>
> Looking in latest code here
> https://elixir.bootlin.com/linux/v4.17-rc2/source/kernel/signal.c#L2449
> i see that current->flags |= PF_SIGNALED; is out side of
> if (sig_kernel_coredump(signr)) {...} scope

In small words.  You showed me the backtrace and I have read
the code.

PF_SIGNALED means you got killed by a signal.
get_signal
  do_coredump
  do_group_exit
    do_exit
       exit_signals
          sets PF_EXITING
       exit_mm
          calls fput on mmaps
             calls sched_task_work
       exit_files
          calls fput on open files
             calls sched_task_work
       exit_task_work
          task_work_run
             /* you are here */

So strictly speaking you are inside of get_signal it is not
meaningful to speak of yourself as within get_signal.

I am a little surprised to see task_work_run called so early.
I was mostly expecting it to happen when the dead task was
scheduling away, like normally happens.

Testing for PF_SIGNALED does not give you anything at all
that testing for PF_EXITING (the flag that signal handling
is shutdown) does not get you.

There is no point in distinguishing PF_SIGNALED from any other
path to do_exit.  do_exit never returns.

The task is dead.

Blocking indefinitely while shutting down a task is a bad idea.
Blocking indefinitely while closing a file descriptor is a bad idea.

The task has been killed it can't get more dead.  SIGKILL is meaningless
at this point.

So you need a timeout, or not to wait at all.


Eric

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

[Daniel Vetter]

On Tue, Apr 24, 2018 at 05:37:08PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 04/24/2018 05:21 PM, Eric W. Biederman wrote:
> > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:
> > 
> > > On 04/24/2018 03:44 PM, Daniel Vetter wrote:
> > > > On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
> > > > > Adding the dri-devel list, since this is driver independent code.
> > > > > 
> > > > > 
> > > > > On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
> > > > > > Avoid calling wait_event_killable when you are possibly being called
> > > > > > from get_signal routine since in that case you end up in a deadlock
> > > > > > where you are alreay blocked in singla processing any trying to wait
> > > > > Multiple typos here, "[...] already blocked in signal processing and [...]"?
> > > > > 
> > > > > 
> > > > > > on a new signal.
> > > > > > 
> > > > > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
> > > > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > > > index 088ff2b..09fd258 100644
> > > > > > --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > > > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> > > > > > @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
> > > > > >    		return;
> > > > > >    	/**
> > > > > >    	 * The client will not queue more IBs during this fini, consume existing
> > > > > > -	 * queued IBs or discard them on SIGKILL
> > > > > > +	 * queued IBs or discard them when in death signal state since
> > > > > > +	 * wait_event_killable can't receive signals in that state.
> > > > > >    	*/
> > > > > > -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
> > > > > > +	if (current->flags & PF_SIGNALED)
> > > > You want fatal_signal_pending() here, instead of inventing your own broken
> > > > version.
> > > I rely on current->flags & PF_SIGNALED because this being set from
> > > within get_signal,
> > It doesn't mean that.  Unless you are called by do_coredump (you
> > aren't).
> 
> Looking in latest code here
> https://elixir.bootlin.com/linux/v4.17-rc2/source/kernel/signal.c#L2449
> i see that current->flags |= PF_SIGNALED; is out side of
> if (sig_kernel_coredump(signr)) {...} scope

Ok I read some more about this, and I guess you go through process exit
and then eventually close. But I'm not sure.

The code in drm_sched_entity_fini also looks strange: You unpark the
scheduler thread before you remove all the IBs. At least from the comment
that doesn't sound like what you want to do.

But in general, PF_SIGNALED is really something deeply internal to the
core (used for some book-keeping and accounting). The drm scheduler is the
only thing looking at it, so smells like a layering violation. I suspect
(but without knowing what you're actually trying to achive here can't be
sure) you want to look at something else.

E.g. PF_EXITING seems to be used in a lot more places to cancel stuff
that's no longer relevant when a task exits, not PF_SIGNALED. There's the
TIF_MEMDIE flag if you're hacking around issues with the oom-killer.

This here on the other hand looks really fragile, and probably only does
what you want to do by accident.
-Daniel

> 
> Andrey
> 
> > The closing of files does not happen in do_coredump.
> > Which means you are being called from do_exit.
> > In fact you are being called after exit_files which closes
> > the files.  The actual __fput processing happens in task_work_run.
> > 
> > > meaning I am within signal processing  in which case I want to avoid
> > > any signal based wait for that task,
> > >  From what i see in the code, task_struct.pending.signal is being set
> > > for other threads in same
> > > group (zap_other_threads) or for other scenarios, those task are still
> > > able to receive signals
> > > so calling wait_event_killable there will not have problem.
> > Excpet that you are geing called after from do_exit and after exit_files
> > which is after exit_signal.  Which means that PF_EXITING has been set.
> > Which implies that the kernel signal handling machinery has already
> > started being torn down.
> > 
> > Not as much as I would like to happen at that point as we are still
> > left with some old CLONE_PTHREAD messes in the code that need to be
> > cleaned up.
> > 
> > Still given the fact you are task_work_run it is quite possible even
> > release_task has been run on that task before the f_op->release method
> > is called.  So you simply can not count on signals working.
> > 
> > Which in practice leaves a timeout for ending your wait.  That code can
> > legitimately be in a context that is neither interruptible nor killable.
> > 
> > > > > >    		entity->fini_status = -ERESTARTSYS;
> > > > > >    	else
> > > > > >    		entity->fini_status = wait_event_killable(sched->job_scheduled,
> > > > But really this smells like a bug in wait_event_killable, since
> > > > wait_event_interruptible does not suffer from the same bug. It will return
> > > > immediately when there's a signal pending.
> > > Even when wait_event_interruptible is called as following -
> > > ...->do_signal->get_signal->....->wait_event_interruptible ?
> > > I haven't tried it but wait_event_interruptible is very much alike to
> > > wait_event_killable so I would assume it will also
> > > not be interrupted if called like that. (Will give it a try just out
> > > of curiosity anyway)
> > As PF_EXITING is set want_signal should fail and the signal state of the
> > task should not be updatable by signals.
> > 
> > Eric
> > 
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Andrey Grodzovsky]

On 04/25/2018 03:14 AM, Daniel Vetter wrote:
> On Tue, Apr 24, 2018 at 05:37:08PM -0400, Andrey Grodzovsky wrote:
>>
>> On 04/24/2018 05:21 PM, Eric W. Biederman wrote:
>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:
>>>
>>>> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
>>>>> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>>>>>> Adding the dri-devel list, since this is driver independent code.
>>>>>>
>>>>>>
>>>>>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>>>>>> Avoid calling wait_event_killable when you are possibly being called
>>>>>>> from get_signal routine since in that case you end up in a deadlock
>>>>>>> where you are alreay blocked in singla processing any trying to wait
>>>>>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>>>>>
>>>>>>
>>>>>>> on a new signal.
>>>>>>>
>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>>>>>     1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>> index 088ff2b..09fd258 100644
>>>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>>>>>     		return;
>>>>>>>     	/**
>>>>>>>     	 * The client will not queue more IBs during this fini, consume existing
>>>>>>> -	 * queued IBs or discard them on SIGKILL
>>>>>>> +	 * queued IBs or discard them when in death signal state since
>>>>>>> +	 * wait_event_killable can't receive signals in that state.
>>>>>>>     	*/
>>>>>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>>>>>> +	if (current->flags & PF_SIGNALED)
>>>>> You want fatal_signal_pending() here, instead of inventing your own broken
>>>>> version.
>>>> I rely on current->flags & PF_SIGNALED because this being set from
>>>> within get_signal,
>>> It doesn't mean that.  Unless you are called by do_coredump (you
>>> aren't).
>> Looking in latest code here
>> https://elixir.bootlin.com/linux/v4.17-rc2/source/kernel/signal.c#L2449
>> i see that current->flags |= PF_SIGNALED; is out side of
>> if (sig_kernel_coredump(signr)) {...} scope
> Ok I read some more about this, and I guess you go through process exit
> and then eventually close. But I'm not sure.
>
> The code in drm_sched_entity_fini also looks strange: You unpark the
> scheduler thread before you remove all the IBs. At least from the comment
> that doesn't sound like what you want to do.

I think it should be safe for the dying scheduler entity since before 
that (in drm_sched_entity_do_release) we set it's runqueue to NULL
so no new jobs will be dequeued form it by the scheduler thread.

>
> But in general, PF_SIGNALED is really something deeply internal to the
> core (used for some book-keeping and accounting). The drm scheduler is the
> only thing looking at it, so smells like a layering violation. I suspect
> (but without knowing what you're actually trying to achive here can't be
> sure) you want to look at something else.
>
> E.g. PF_EXITING seems to be used in a lot more places to cancel stuff
> that's no longer relevant when a task exits, not PF_SIGNALED. There's the
> TIF_MEMDIE flag if you're hacking around issues with the oom-killer.
>
> This here on the other hand looks really fragile, and probably only does
> what you want to do by accident.
> -Daniel

Yes , that what Eric also said and in the V2 patches i will try  to 
change PF_EXITING

Another issue is changing wait_event_killable to wait_event_timeout 
where I need to understand
what TO value is acceptable for all the drivers using the scheduler, or 
maybe it should come as a property
of drm_sched_entity.

Andrey
>
>> Andrey
>>
>>> The closing of files does not happen in do_coredump.
>>> Which means you are being called from do_exit.
>>> In fact you are being called after exit_files which closes
>>> the files.  The actual __fput processing happens in task_work_run.
>>>
>>>> meaning I am within signal processing  in which case I want to avoid
>>>> any signal based wait for that task,
>>>>   From what i see in the code, task_struct.pending.signal is being set
>>>> for other threads in same
>>>> group (zap_other_threads) or for other scenarios, those task are still
>>>> able to receive signals
>>>> so calling wait_event_killable there will not have problem.
>>> Excpet that you are geing called after from do_exit and after exit_files
>>> which is after exit_signal.  Which means that PF_EXITING has been set.
>>> Which implies that the kernel signal handling machinery has already
>>> started being torn down.
>>>
>>> Not as much as I would like to happen at that point as we are still
>>> left with some old CLONE_PTHREAD messes in the code that need to be
>>> cleaned up.
>>>
>>> Still given the fact you are task_work_run it is quite possible even
>>> release_task has been run on that task before the f_op->release method
>>> is called.  So you simply can not count on signals working.
>>>
>>> Which in practice leaves a timeout for ending your wait.  That code can
>>> legitimately be in a context that is neither interruptible nor killable.
>>>
>>>>>>>     		entity->fini_status = -ERESTARTSYS;
>>>>>>>     	else
>>>>>>>     		entity->fini_status = wait_event_killable(sched->job_scheduled,
>>>>> But really this smells like a bug in wait_event_killable, since
>>>>> wait_event_interruptible does not suffer from the same bug. It will return
>>>>> immediately when there's a signal pending.
>>>> Even when wait_event_interruptible is called as following -
>>>> ...->do_signal->get_signal->....->wait_event_interruptible ?
>>>> I haven't tried it but wait_event_interruptible is very much alike to
>>>> wait_event_killable so I would assume it will also
>>>> not be interrupted if called like that. (Will give it a try just out
>>>> of curiosity anyway)
>>> As PF_EXITING is set want_signal should fail and the signal state of the
>>> task should not be updatable by signals.
>>>
>>> Eric
>>>
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Oleg Nesterov]

On 04/24, Daniel Vetter wrote:
>
> wait_event_killabel doesn't check for fatal_signal_pending before calling
> schedule, so definitely has a nice race there.

This is fine. See the signal_pending_state() check in __schedule().

And this doesn't differ from wait_event_interruptible(), it too doesn't
check signal_pending(), we rely on schedule() which must not block if the
caller is signalled/killed.

The problem is that it is not clear what should fatal_signal_pending() or
even signal_pending() mean after exit_signals().

Oleg.

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

[Daniel Vetter]

On Wed, Apr 25, 2018 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/24, Daniel Vetter wrote:
>>
>> wait_event_killabel doesn't check for fatal_signal_pending before calling
>> schedule, so definitely has a nice race there.
>
> This is fine. See the signal_pending_state() check in __schedule().
>
> And this doesn't differ from wait_event_interruptible(), it too doesn't
> check signal_pending(), we rely on schedule() which must not block if the
> caller is signalled/killed.
>
> The problem is that it is not clear what should fatal_signal_pending() or
> even signal_pending() mean after exit_signals().

Uh, I was totally thrown off in all the wait_event* macros and somehow
landed in the _locked variants, which all need to recheck before they
drop the lock, for efficiency reasons. See do_wait_intr().

Sorry for the confusion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Andrey Grodzovsky]

On 04/24/2018 05:40 PM, Daniel Vetter wrote:
> On Tue, Apr 24, 2018 at 05:02:40PM -0400, Andrey Grodzovsky wrote:
>>
>> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
>>> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>>>> Adding the dri-devel list, since this is driver independent code.
>>>>
>>>>
>>>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>>>> Avoid calling wait_event_killable when you are possibly being called
>>>>> from get_signal routine since in that case you end up in a deadlock
>>>>> where you are alreay blocked in singla processing any trying to wait
>>>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>>>
>>>>
>>>>> on a new signal.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> index 088ff2b..09fd258 100644
>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>>>    		return;
>>>>>    	/**
>>>>>    	 * The client will not queue more IBs during this fini, consume existing
>>>>> -	 * queued IBs or discard them on SIGKILL
>>>>> +	 * queued IBs or discard them when in death signal state since
>>>>> +	 * wait_event_killable can't receive signals in that state.
>>>>>    	*/
>>>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>>>> +	if (current->flags & PF_SIGNALED)
>>> You want fatal_signal_pending() here, instead of inventing your own broken
>>> version.
>> I rely on current->flags & PF_SIGNALED because this being set from within
>> get_signal,
>> meaning I am within signal processing  in which case I want to avoid any
>> signal based wait for that task,
>>  From what i see in the code, task_struct.pending.signal is being set for
>> other threads in same
>> group (zap_other_threads) or for other scenarios, those task are still able
>> to receive signals
>> so calling wait_event_killable there will not have problem.
>>>>>    		entity->fini_status = -ERESTARTSYS;
>>>>>    	else
>>>>>    		entity->fini_status = wait_event_killable(sched->job_scheduled,
>>> But really this smells like a bug in wait_event_killable, since
>>> wait_event_interruptible does not suffer from the same bug. It will return
>>> immediately when there's a signal pending.
>> Even when wait_event_interruptible is called as following -
>> ...->do_signal->get_signal->....->wait_event_interruptible ?
>> I haven't tried it but wait_event_interruptible is very much alike to
>> wait_event_killable so I would assume it will also
>> not be interrupted if called like that. (Will give it a try just out of
>> curiosity anyway)
> wait_event_killabel doesn't check for fatal_signal_pending before calling
> schedule, so definitely has a nice race there.
>
> But if you're sure that you really need to check PF_SIGNALED, then I'm
> honestly not clear on what you're trying to pull off here. Your sparse
> explanation of what happens isn't enough, since I have no idea how you can
> get from get_signal() to the above wait_event_killable callsite.

Fatal signal will trigger process termination during which all FDs are 
released, including DRM's.

See here -

[<0>] drm_sched_entity_fini+0x10a/0x3a0 [gpu_sched]
[<0>] amdgpu_ctx_do_release+0x129/0x170 [amdgpu]
[<0>] amdgpu_ctx_mgr_fini+0xd5/0xe0 [amdgpu]
[<0>] amdgpu_driver_postclose_kms+0xcd/0x440 [amdgpu]
[<0>] drm_release+0x414/0x5b0 [drm]
[<0>] __fput+0x176/0x350
[<0>] task_work_run+0xa1/0xc0

(From Eric's explanation above is triggered by do_exit->exit_files)
...
[<0>] do_exit+0x48f/0x1280
[<0>] do_group_exit+0x89/0x140
[<0>] get_signal+0x375/0x8f0
[<0>] do_signal+0x79/0xaa0
[<0>] exit_to_usermode_loop+0x83/0xd0
[<0>] do_syscall_64+0x244/0x270
[<0>] entry_SYSCALL_64_after_hwframe+0x3d/0xa2

Andrey

> -Daniel
>
>> Andrey
>>
>>> I think this should be fixed in core code, not papered over in some
>>> subsystem.
>>> -Daniel
>>>
>>>> -- 
>>>> Earthling Michel Dänzer               |               http://www.amd.com
>>>> Libre software enthusiast             |             Mesa and X developer
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Oleg Nesterov]

On 04/25, Daniel Vetter wrote:
>
> On Wed, Apr 25, 2018 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 04/24, Daniel Vetter wrote:
> >>
> >> wait_event_killabel doesn't check for fatal_signal_pending before calling
> >> schedule, so definitely has a nice race there.
> >
> > This is fine. See the signal_pending_state() check in __schedule().
> >
> > And this doesn't differ from wait_event_interruptible(), it too doesn't
> > check signal_pending(), we rely on schedule() which must not block if the
> > caller is signalled/killed.
> >
> > The problem is that it is not clear what should fatal_signal_pending() or
> > even signal_pending() mean after exit_signals().
>
> Uh, I was totally thrown off in all the wait_event* macros and somehow
> landed in the _locked variants, which all need to recheck before they
> drop the lock, for efficiency reasons. See do_wait_intr().

Just in case, note that do_wait_intr() has to check signal_pending() for
completely differerent reason. We need to return non-zero code to stop the
main loop in __wait_event_interruptible_locked(); unlike ___wait_event()
it doesn't check signal_pending() itself.

Oleg.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric W. Biederman]

Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:

> On 04/25/2018 03:14 AM, Daniel Vetter wrote:
>> On Tue, Apr 24, 2018 at 05:37:08PM -0400, Andrey Grodzovsky wrote:
>>>
>>> On 04/24/2018 05:21 PM, Eric W. Biederman wrote:
>>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:
>>>>
>>>>> On 04/24/2018 03:44 PM, Daniel Vetter wrote:
>>>>>> On Tue, Apr 24, 2018 at 05:46:52PM +0200, Michel Dänzer wrote:
>>>>>>> Adding the dri-devel list, since this is driver independent code.
>>>>>>>
>>>>>>>
>>>>>>> On 2018-04-24 05:30 PM, Andrey Grodzovsky wrote:
>>>>>>>> Avoid calling wait_event_killable when you are possibly being called
>>>>>>>> from get_signal routine since in that case you end up in a deadlock
>>>>>>>> where you are alreay blocked in singla processing any trying to wait
>>>>>>> Multiple typos here, "[...] already blocked in signal processing and [...]"?
>>>>>>>
>>>>>>>
>>>>>>>> on a new signal.
>>>>>>>>
>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/scheduler/gpu_scheduler.c | 5 +++--
>>>>>>>>     1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> index 088ff2b..09fd258 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>>>>>>> @@ -227,9 +227,10 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>>>>>>>>     		return;
>>>>>>>>     	/**
>>>>>>>>     	 * The client will not queue more IBs during this fini, consume existing
>>>>>>>> -	 * queued IBs or discard them on SIGKILL
>>>>>>>> +	 * queued IBs or discard them when in death signal state since
>>>>>>>> +	 * wait_event_killable can't receive signals in that state.
>>>>>>>>     	*/
>>>>>>>> -	if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
>>>>>>>> +	if (current->flags & PF_SIGNALED)
>>>>>> You want fatal_signal_pending() here, instead of inventing your own broken
>>>>>> version.
>>>>> I rely on current->flags & PF_SIGNALED because this being set from
>>>>> within get_signal,
>>>> It doesn't mean that.  Unless you are called by do_coredump (you
>>>> aren't).
>>> Looking in latest code here
>>> https://elixir.bootlin.com/linux/v4.17-rc2/source/kernel/signal.c#L2449
>>> i see that current->flags |= PF_SIGNALED; is out side of
>>> if (sig_kernel_coredump(signr)) {...} scope
>> Ok I read some more about this, and I guess you go through process exit
>> and then eventually close. But I'm not sure.
>>
>> The code in drm_sched_entity_fini also looks strange: You unpark the
>> scheduler thread before you remove all the IBs. At least from the comment
>> that doesn't sound like what you want to do.
>
> I think it should be safe for the dying scheduler entity since before that (in
> drm_sched_entity_do_release) we set it's runqueue to NULL
> so no new jobs will be dequeued form it by the scheduler thread.
>
>>
>> But in general, PF_SIGNALED is really something deeply internal to the
>> core (used for some book-keeping and accounting). The drm scheduler is the
>> only thing looking at it, so smells like a layering violation. I suspect
>> (but without knowing what you're actually trying to achive here can't be
>> sure) you want to look at something else.
>>
>> E.g. PF_EXITING seems to be used in a lot more places to cancel stuff
>> that's no longer relevant when a task exits, not PF_SIGNALED. There's the
>> TIF_MEMDIE flag if you're hacking around issues with the oom-killer.
>>
>> This here on the other hand looks really fragile, and probably only does
>> what you want to do by accident.
>> -Daniel
>
> Yes , that what Eric also said and in the V2 patches i will try  to change
> PF_EXITING
>
> Another issue is changing wait_event_killable to wait_event_timeout where I need
> to understand
> what TO value is acceptable for all the drivers using the scheduler, or maybe it
> should come as a property
> of drm_sched_entity.

It would not surprise me if you could pick a large value like 1 second
and issue a warning if that time outever triggers.  It sounds like the
condition where we wait indefinitely today is because something went
wrong in the driver.

Eric

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

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]



On 04/25/2018 11:29 AM, Eric W. Biederman wrote:
>> Another issue is changing wait_event_killable to wait_event_timeout where I need
>> to understand
>> what TO value is acceptable for all the drivers using the scheduler, or maybe it
>> should come as a property
>> of drm_sched_entity.
> It would not surprise me if you could pick a large value like 1 second
> and issue a warning if that time outever triggers.  It sounds like the
> condition where we wait indefinitely today is because something went
> wrong in the driver.

We wait here for all GPU jobs in flight which belong to the dying entity 
to complete. The driver submits
the GPU jobs but the content of the job might be is not under driver's 
control and could take
long time to finish or even hang (e.g. graphic or compute shader) , I 
guess that why originally the wait is indefinite.

Andrey

>
> Eric


[-- Attachment #1.2: Type: text/html, Size: 1486 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric W. Biederman]

Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> writes:

> On 04/25/2018 11:29 AM, Eric W. Biederman wrote:
>
>>  Another issue is changing wait_event_killable to wait_event_timeout where I need
>> to understand
>> what TO value is acceptable for all the drivers using the scheduler, or maybe it
>> should come as a property
>> of drm_sched_entity.
>>
>> It would not surprise me if you could pick a large value like 1 second
>> and issue a warning if that time outever triggers.  It sounds like the
>> condition where we wait indefinitely today is because something went
>> wrong in the driver.
>
> We wait here for all GPU jobs in flight which belong to the dying entity to complete. The driver submits
> the GPU jobs but the content of the job might be is not under driver's control and could take 
> long time to finish or even hang (e.g. graphic or compute shader) , I
> guess that why originally the wait is indefinite.


I am ignorant of what user space expect or what the semantics of the
susbsystem are here, so I might be completely off base.  But this wait
for a long time behavior I would expect much more from f_op->flush or a
f_op->fsync method.

fsync so it could be obtained without closing the file descriptor.
flush so that you could get a return value out to close.

But I honestly don't know semantically what your userspace applications
expect and/or require so I can really only say.  Those of weird semantics.

Eric

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

[Andrey Grodzovsky]

Hi Michele and others, I am trying to implement the approach bellow to 
resolve AMDGPU's hang when commands are stuck in pipe during process exit.

I noticed that once I implemented the file_operation.flush callback  
then during run of X, i see the flush callback gets called not only for 
Xorg process but for other

processes such as 'xkbcomp' and even 'sh', it seems like Xorg passes his 
FDs to children, Christian mentioned he remembered a discussion to 
always set FD_CLOEXEC flag when opening the hardware device file, so

we suspect a bug in Xorg with regard to this behavior.

Any advise on this would be very helpful.


Andrey


On 05/02/2018 07:48 AM, Christian König wrote:
> I suggest the following approach:
> 1. Implement the flush callback and call the function to wait for the 
> scheduler to push everything to the hardware (maybe rename the 
> scheduler function to flush as well).
>
> 2. Change the scheduler to test for PF_EXITING, if it's set use 
> wait_event_timeout() if it isn't set use wait_event_killable().
>
> When the wait times out or is killed set a flag so that the _fini 
> function knows that. Alternatively you could cleanup the _fini 
> function to work in all cases, e.g. both when there are still jobs on 
> the queue and when the queue is empty. For this you need to add 
> something like a struct completion to the main loop to remove this 
> start()/stop() of the kernel thread.
>
> Christian. 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
> Hi Michele and others, I am trying to implement the approach bellow to
> resolve AMDGPU's hang when commands are stuck in pipe during process exit.
> 
> I noticed that once I implemented the file_operation.flush callback 
> then during run of X, i see the flush callback gets called not only for
> Xorg process but for other
> 
> processes such as 'xkbcomp' and even 'sh', it seems like Xorg passes his
> FDs to children, Christian mentioned he remembered a discussion to
> always set FD_CLOEXEC flag when opening the hardware device file, so
> 
> we suspect a bug in Xorg with regard to this behavior.

Try the libdrm patch below.

Note that the X server passes DRM file descriptors to DRI3 clients.


diff --git a/xf86drm.c b/xf86drm.c
index 3a9d0ed2..c09437b0 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -405,7 +405,7 @@ wait_for_udev:
     }
 #endif

-    fd = open(buf, O_RDWR, 0);
+    fd = open(buf, O_RDWR | O_CLOEXEC, 0);
     drmMsg("drmOpenDevice: open result is %d, (%s)\n",
            fd, fd < 0 ? strerror(errno) : "OK");
     if (fd >= 0)
@@ -425,7 +425,7 @@ wait_for_udev:
             chmod(buf, devmode);
         }
     }
-    fd = open(buf, O_RDWR, 0);
+    fd = open(buf, O_RDWR | O_CLOEXEC, 0);
     drmMsg("drmOpenDevice: open result is %d, (%s)\n",
            fd, fd < 0 ? strerror(errno) : "OK");
     if (fd >= 0)
@@ -474,7 +474,7 @@ static int drmOpenMinor(int minor, int create, int type)
     };

     sprintf(buf, dev_name, DRM_DIR_NAME, minor);
-    if ((fd = open(buf, O_RDWR, 0)) >= 0)
+    if ((fd = open(buf, O_RDWR | O_CLOEXEC, 0)) >= 0)
         return fd;
     return -errno;
 }



-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

Thanks Michel, will give it a try.

BTW, just out of interest, how the FDs are passed to clients ? Using 
sockets ? Can you point me to the code which does it ?

Andrey


On 05/17/2018 10:48 AM, Michel Dänzer wrote:
> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>> Hi Michele and others, I am trying to implement the approach bellow to
>> resolve AMDGPU's hang when commands are stuck in pipe during process exit.
>>
>> I noticed that once I implemented the file_operation.flush callback
>> then during run of X, i see the flush callback gets called not only for
>> Xorg process but for other
>>
>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg passes his
>> FDs to children, Christian mentioned he remembered a discussion to
>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>
>> we suspect a bug in Xorg with regard to this behavior.
> Try the libdrm patch below.
>
> Note that the X server passes DRM file descriptors to DRI3 clients.
>
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 3a9d0ed2..c09437b0 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -405,7 +405,7 @@ wait_for_udev:
>       }
>   #endif
>
> -    fd = open(buf, O_RDWR, 0);
> +    fd = open(buf, O_RDWR | O_CLOEXEC, 0);
>       drmMsg("drmOpenDevice: open result is %d, (%s)\n",
>              fd, fd < 0 ? strerror(errno) : "OK");
>       if (fd >= 0)
> @@ -425,7 +425,7 @@ wait_for_udev:
>               chmod(buf, devmode);
>           }
>       }
> -    fd = open(buf, O_RDWR, 0);
> +    fd = open(buf, O_RDWR | O_CLOEXEC, 0);
>       drmMsg("drmOpenDevice: open result is %d, (%s)\n",
>              fd, fd < 0 ? strerror(errno) : "OK");
>       if (fd >= 0)
> @@ -474,7 +474,7 @@ static int drmOpenMinor(int minor, int create, int type)
>       };
>
>       sprintf(buf, dev_name, DRM_DIR_NAME, minor);
> -    if ((fd = open(buf, O_RDWR, 0)) >= 0)
> +    if ((fd = open(buf, O_RDWR | O_CLOEXEC, 0)) >= 0)
>           return fd;
>       return -errno;
>   }
>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 2018-05-17 05:33 PM, Andrey Grodzovsky wrote:
> 
> BTW, just out of interest, how the FDs are passed to clients ? Using
> sockets ?

Yes, via the socket used for the X11 display connection.


> Can you point me to the code which does it ?

xserver/dri3/dri3_request.c:dri3_send_open_reply() =>
xserver/os/io.c:WriteFdToClient()

Note that since dri3_send_open_reply passes TRUE for WriteFdToClient's
do_close parameter, the file descriptor is closed in the Xorg process
after sending it to the client.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

On 05/17/2018 10:48 AM, Michel Dänzer wrote:
> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>> Hi Michele and others, I am trying to implement the approach bellow to
>> resolve AMDGPU's hang when commands are stuck in pipe during process exit.
>>
>> I noticed that once I implemented the file_operation.flush callback
>> then during run of X, i see the flush callback gets called not only for
>> Xorg process but for other
>>
>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg passes his
>> FDs to children, Christian mentioned he remembered a discussion to
>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>
>> we suspect a bug in Xorg with regard to this behavior.
> Try the libdrm patch below.
>
> Note that the X server passes DRM file descriptors to DRI3 clients.

Tried it, didn't help. I still see other processes calling .flush for 
/dev/dri/card0

Thanks,
Andrey

>
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 3a9d0ed2..c09437b0 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -405,7 +405,7 @@ wait_for_udev:
>       }
>   #endif
>
> -    fd = open(buf, O_RDWR, 0);
> +    fd = open(buf, O_RDWR | O_CLOEXEC, 0);
>       drmMsg("drmOpenDevice: open result is %d, (%s)\n",
>              fd, fd < 0 ? strerror(errno) : "OK");
>       if (fd >= 0)
> @@ -425,7 +425,7 @@ wait_for_udev:
>               chmod(buf, devmode);
>           }
>       }
> -    fd = open(buf, O_RDWR, 0);
> +    fd = open(buf, O_RDWR | O_CLOEXEC, 0);
>       drmMsg("drmOpenDevice: open result is %d, (%s)\n",
>              fd, fd < 0 ? strerror(errno) : "OK");
>       if (fd >= 0)
> @@ -474,7 +474,7 @@ static int drmOpenMinor(int minor, int create, int type)
>       };
>
>       sprintf(buf, dev_name, DRM_DIR_NAME, minor);
> -    if ((fd = open(buf, O_RDWR, 0)) >= 0)
> +    if ((fd = open(buf, O_RDWR | O_CLOEXEC, 0)) >= 0)
>           return fd;
>       return -errno;
>   }
>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 2018-05-17 09:05 PM, Andrey Grodzovsky wrote:
> On 05/17/2018 10:48 AM, Michel Dänzer wrote:
>> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>>> Hi Michele and others, I am trying to implement the approach bellow to
>>> resolve AMDGPU's hang when commands are stuck in pipe during process
>>> exit.
>>>
>>> I noticed that once I implemented the file_operation.flush callback
>>> then during run of X, i see the flush callback gets called not only for
>>> Xorg process but for other
>>>
>>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg passes his
>>> FDs to children, Christian mentioned he remembered a discussion to
>>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>>
>>> we suspect a bug in Xorg with regard to this behavior.
>> Try the libdrm patch below.
>>
>> Note that the X server passes DRM file descriptors to DRI3 clients.
> 
> Tried it, didn't help. I still see other processes calling .flush for
> /dev/dri/card0

Try the attached xserver patch on top. With these patches, I no longer
see any DRM file descriptors being opened without O_CLOEXEC running Xorg
-pogo in strace.


Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush
callback being called from multiple processes is an issue, maybe the
flush callback isn't appropriate after all.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xserver-DRM-always-CLOEXEC.diff --]
[-- Type: text/x-patch; name="xserver-DRM-always-CLOEXEC.diff", Size: 1271 bytes --]

diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
index 5d8906d63..306541f33 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -200,12 +200,12 @@ open_hw(const char *dev)
     int fd;
 
     if (dev)
-        fd = open(dev, O_RDWR, 0);
+        fd = open(dev, O_RDWR | O_CLOEXEC, 0);
     else {
         dev = getenv("KMSDEVICE");
-        if ((NULL == dev) || ((fd = open(dev, O_RDWR, 0)) == -1)) {
+        if ((NULL == dev) || ((fd = open(dev, O_RDWR | O_CLOEXEC, 0)) == -1)) {
             dev = "/dev/dri/card0";
-            fd = open(dev, O_RDWR, 0);
+            fd = open(dev, O_RDWR | O_CLOEXEC, 0);
         }
     }
     if (fd == -1)
diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 11af52c46..70374ace8 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -43,7 +43,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path, int delayed_index)
     }
 
     if (fd == -1)
-        fd = open(path, O_RDWR, O_CLOEXEC);
+        fd = open(path, O_RDWR | O_CLOEXEC, 0);
 
     if (fd == -1)
         return FALSE;

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush
> callback being called from multiple processes is an issue, maybe the
> flush callback isn't appropriate after all.

Userspace could also grab a reference just by opening /proc/$pid/fd/*.

The idea is just that when any process which used the fd is killed by a 
signal we drop the remaining jobs from being submitted to the hardware.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 2018-05-18 11:42 AM, Christian König wrote:
> 
>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush
>> callback being called from multiple processes is an issue, maybe the
>> flush callback isn't appropriate after all.
> 
> Userspace could also grab a reference just by opening /proc/$pid/fd/*.
> 
> The idea is just that when any process which used the fd is killed by a
> signal we drop the remaining jobs from being submitted to the hardware.

This must only affect jobs submitted by the killed process, not those
submitted by other processes.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

Am 18.05.2018 um 16:44 schrieb Michel Dänzer:
> On 2018-05-18 11:42 AM, Christian König wrote:
>>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush
>>> callback being called from multiple processes is an issue, maybe the
>>> flush callback isn't appropriate after all.
>> Userspace could also grab a reference just by opening /proc/$pid/fd/*.
>>
>> The idea is just that when any process which used the fd is killed by a
>> signal we drop the remaining jobs from being submitted to the hardware.
> This must only affect jobs submitted by the killed process, not those
> submitted by other processes.

Yeah, that's exactly the plan here.

For additional security we could safe the pid of the job submitter, but 
since this should basically not happen in normal operation I would 
rather like to avoid that.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

On 05/18/2018 10:50 AM, Christian König wrote:
> Am 18.05.2018 um 16:44 schrieb Michel Dänzer:
>> On 2018-05-18 11:42 AM, Christian König wrote:
>>>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the 
>>>> flush
>>>> callback being called from multiple processes is an issue, maybe the
>>>> flush callback isn't appropriate after all.
>>> Userspace could also grab a reference just by opening /proc/$pid/fd/*.
>>>
>>> The idea is just that when any process which used the fd is killed by a
>>> signal we drop the remaining jobs from being submitted to the hardware.
>> This must only affect jobs submitted by the killed process, not those
>> submitted by other processes.
>
> Yeah, that's exactly the plan here.

I don't see how it's gong to happen -
.flush is being called for any terminating process regardless if he 
submitted jobs
or just accidentally (or not)  has the device file FD in his private 
file table. So here
we going to have a problem with that requirement. If a process is being 
killed and .flush is
executed I don't have any way to know which  amdgpu_ctx to chose to 
terminate it's pending jobs.
The only info i have from .flush caller is the process id.
As it's now in amdgpu_ctx_mgr_entity_fini and 
amdgpu_ctx_mgr_entity_cleanup we are going to iterate
all the contextes from the context manager list and terminate them all, 
which sounds wrong to me indeed.
I can save the pid of the context creator on the context structure so i 
can match during .flush call, but in case some one
creates the context but passes the context id to another process for 
actual job submission this approach won't work either.

Am I messing something here ?

Andrey

>
> For additional security we could safe the pid of the job submitter, 
> but since this should basically not happen in normal operation I would 
> rather like to avoid that.
>
> Christian.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

Am 18.05.2018 um 17:02 schrieb Andrey Grodzovsky:
>
>
> On 05/18/2018 10:50 AM, Christian König wrote:
>> Am 18.05.2018 um 16:44 schrieb Michel Dänzer:
>>> On 2018-05-18 11:42 AM, Christian König wrote:
>>>>> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the 
>>>>> flush
>>>>> callback being called from multiple processes is an issue, maybe the
>>>>> flush callback isn't appropriate after all.
>>>> Userspace could also grab a reference just by opening /proc/$pid/fd/*.
>>>>
>>>> The idea is just that when any process which used the fd is killed 
>>>> by a
>>>> signal we drop the remaining jobs from being submitted to the 
>>>> hardware.
>>> This must only affect jobs submitted by the killed process, not those
>>> submitted by other processes.
>>
>> Yeah, that's exactly the plan here.
>
> I don't see how it's gong to happen -
> .flush is being called for any terminating process regardless if he 
> submitted jobs
> or just accidentally (or not)  has the device file FD in his private 
> file table. So here
> we going to have a problem with that requirement. If a process is 
> being killed and .flush is
> executed I don't have any way to know which  amdgpu_ctx to chose to 
> terminate it's pending jobs.
> The only info i have from .flush caller is the process id.
> As it's now in amdgpu_ctx_mgr_entity_fini and 
> amdgpu_ctx_mgr_entity_cleanup we are going to iterate
> all the contextes from the context manager list and terminate them 
> all, which sounds wrong to me indeed.
> I can save the pid of the context creator on the context structure so 
> i can match during .flush call, but in case some one
> creates the context but passes the context id to another process for 
> actual job submission this approach won't work either.
>
> Am I messing something here ?

Your analyses is correct, it's just that I think that this case should 
not happen.

What can happen is that the fd is passed accidentally to child processes 
and those child processes are then killed, but passing the fd to child 
processes is a bug in the first place.

When somebody on purpose opens the fd and kills the process then it 
breaks and he can keep the pieces. I mean to open the fd you need to be 
privileged anyway.

What we could do to completely fix the issue:
1. Note for each submitted job which process (pid) it submitted.
2. During flush wait or kill only jobs of the current process.

But I think that this is overkill.

Christian.

>
> Andrey
>
>>
>> For additional security we could safe the pid of the job submitter, 
>> but since this should basically not happen in normal operation I 
>> would rather like to avoid that.
>>
>> Christian.
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]



On 05/18/2018 04:46 AM, Michel Dänzer wrote:
> On 2018-05-17 09:05 PM, Andrey Grodzovsky wrote:
>> On 05/17/2018 10:48 AM, Michel Dänzer wrote:
>>> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>>>> Hi Michele and others, I am trying to implement the approach bellow to
>>>> resolve AMDGPU's hang when commands are stuck in pipe during process
>>>> exit.
>>>>
>>>> I noticed that once I implemented the file_operation.flush callback
>>>> then during run of X, i see the flush callback gets called not only for
>>>> Xorg process but for other
>>>>
>>>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg passes his
>>>> FDs to children, Christian mentioned he remembered a discussion to
>>>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>>>
>>>> we suspect a bug in Xorg with regard to this behavior.
>>> Try the libdrm patch below.
>>>
>>> Note that the X server passes DRM file descriptors to DRI3 clients.
>> Tried it, didn't help. I still see other processes calling .flush for
>> /dev/dri/card0
> Try the attached xserver patch on top. With these patches, I no longer
> see any DRM file descriptors being opened without O_CLOEXEC running Xorg
> -pogo in strace.

Thanks for the patch, unfortunately this is my first time  building xorg
form source and I hit some blocks with dependencies. I wonder if you
could quickly apply to amdgpu the attached small patch and run xinit from
command line. In case the FD is not passed any more you will only see
Xorg print in dmeg afterwards, otherwise 'sh' and 'xkbcomp' will also 
get printed.

Andrey

>
> Anyway, the kernel can't rely on userspace using O_CLOEXEC. If the flush
> callback being called from multiple processes is an issue, maybe the
> flush callback isn't appropriate after all.
>
>


[-- Attachment #2: test_flush.patch --]
[-- Type: text/x-patch, Size: 681 bytes --]

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b0bf2f2..1f63712 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -855,9 +855,18 @@ static const struct dev_pm_ops amdgpu_pm_ops = {
 	.runtime_idle = amdgpu_pmops_runtime_idle,
 };
 
+static int amdgpu_flush(struct file *f, fl_owner_t id) {
+
+	DRM_ERROR("%s\n", current->comm);
+
+       return 0;
+}
+
+
 static const struct file_operations amdgpu_driver_kms_fops = {
 	.owner = THIS_MODULE,
 	.open = drm_open,
+	.flush = amdgpu_flush,
 	.release = drm_release,
 	.unlocked_ioctl = amdgpu_drm_ioctl,
 	.mmap = amdgpu_mmap,

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 2018-05-22 05:49 PM, Andrey Grodzovsky wrote:
> On 05/18/2018 04:46 AM, Michel Dänzer wrote:
>> On 2018-05-17 09:05 PM, Andrey Grodzovsky wrote:
>>> On 05/17/2018 10:48 AM, Michel Dänzer wrote:
>>>> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>>>>> Hi Michele and others, I am trying to implement the approach bellow to
>>>>> resolve AMDGPU's hang when commands are stuck in pipe during process
>>>>> exit.
>>>>>
>>>>> I noticed that once I implemented the file_operation.flush callback
>>>>> then during run of X, i see the flush callback gets called not only
>>>>> for
>>>>> Xorg process but for other
>>>>>
>>>>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg
>>>>> passes his
>>>>> FDs to children, Christian mentioned he remembered a discussion to
>>>>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>>>>
>>>>> we suspect a bug in Xorg with regard to this behavior.
>>>> Try the libdrm patch below.
>>>>
>>>> Note that the X server passes DRM file descriptors to DRI3 clients.
>>> Tried it, didn't help. I still see other processes calling .flush for
>>> /dev/dri/card0
>> Try the attached xserver patch on top. With these patches, I no longer
>> see any DRM file descriptors being opened without O_CLOEXEC running Xorg
>> -pogo in strace.
> 
> Thanks for the patch, unfortunately this is my first time  building xorg
> form source and I hit some blocks with dependencies. I wonder if you
> could quickly apply to amdgpu the attached small patch and run xinit from
> command line. In case the FD is not passed any more you will only see
> Xorg print in dmeg afterwards, otherwise 'sh' and 'xkbcomp' will also
> get printed.

As I said above, with these patches I don't see any DRM file descriptors
being opened without O_CLOEXEC, so they aren't getting passed to sh or
xkbcomp.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

On 05/22/2018 12:09 PM, Michel Dänzer wrote:
> On 2018-05-22 05:49 PM, Andrey Grodzovsky wrote:
>> On 05/18/2018 04:46 AM, Michel Dänzer wrote:
>>> On 2018-05-17 09:05 PM, Andrey Grodzovsky wrote:
>>>> On 05/17/2018 10:48 AM, Michel Dänzer wrote:
>>>>> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>>>>>> Hi Michele and others, I am trying to implement the approach bellow to
>>>>>> resolve AMDGPU's hang when commands are stuck in pipe during process
>>>>>> exit.
>>>>>>
>>>>>> I noticed that once I implemented the file_operation.flush callback
>>>>>> then during run of X, i see the flush callback gets called not only
>>>>>> for
>>>>>> Xorg process but for other
>>>>>>
>>>>>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg
>>>>>> passes his
>>>>>> FDs to children, Christian mentioned he remembered a discussion to
>>>>>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>>>>>
>>>>>> we suspect a bug in Xorg with regard to this behavior.
>>>>> Try the libdrm patch below.
>>>>>
>>>>> Note that the X server passes DRM file descriptors to DRI3 clients.
>>>> Tried it, didn't help. I still see other processes calling .flush for
>>>> /dev/dri/card0
>>> Try the attached xserver patch on top. With these patches, I no longer
>>> see any DRM file descriptors being opened without O_CLOEXEC running Xorg
>>> -pogo in strace.
>> Thanks for the patch, unfortunately this is my first time  building xorg
>> form source and I hit some blocks with dependencies. I wonder if you
>> could quickly apply to amdgpu the attached small patch and run xinit from
>> command line. In case the FD is not passed any more you will only see
>> Xorg print in dmeg afterwards, otherwise 'sh' and 'xkbcomp' will also
>> get printed.
> As I said above, with these patches I don't see any DRM file descriptors
> being opened without O_CLOEXEC, so they aren't getting passed to sh or
> xkbcomp.

OK then, please put me on CC when you send this patch for review.

Andrey

>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 2018-05-22 06:30 PM, Andrey Grodzovsky wrote:
> On 05/22/2018 12:09 PM, Michel Dänzer wrote:
>> On 2018-05-22 05:49 PM, Andrey Grodzovsky wrote:
>>> On 05/18/2018 04:46 AM, Michel Dänzer wrote:
>>>> On 2018-05-17 09:05 PM, Andrey Grodzovsky wrote:
>>>>> On 05/17/2018 10:48 AM, Michel Dänzer wrote:
>>>>>> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>>>>>>> Hi Michele and others, I am trying to implement the approach
>>>>>>> bellow to
>>>>>>> resolve AMDGPU's hang when commands are stuck in pipe during process
>>>>>>> exit.
>>>>>>>
>>>>>>> I noticed that once I implemented the file_operation.flush callback
>>>>>>> then during run of X, i see the flush callback gets called not only
>>>>>>> for
>>>>>>> Xorg process but for other
>>>>>>>
>>>>>>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg
>>>>>>> passes his
>>>>>>> FDs to children, Christian mentioned he remembered a discussion to
>>>>>>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>>>>>>
>>>>>>> we suspect a bug in Xorg with regard to this behavior.
>>>>>> Try the libdrm patch below.
>>>>>>
>>>>>> Note that the X server passes DRM file descriptors to DRI3 clients.
>>>>> Tried it, didn't help. I still see other processes calling .flush for
>>>>> /dev/dri/card0
>>>> Try the attached xserver patch on top. With these patches, I no longer
>>>> see any DRM file descriptors being opened without O_CLOEXEC running
>>>> Xorg
>>>> -pogo in strace.
>>> Thanks for the patch, unfortunately this is my first time  building xorg
>>> form source and I hit some blocks with dependencies. I wonder if you
>>> could quickly apply to amdgpu the attached small patch and run xinit
>>> from
>>> command line. In case the FD is not passed any more you will only see
>>> Xorg print in dmeg afterwards, otherwise 'sh' and 'xkbcomp' will also
>>> get printed.
>> As I said above, with these patches I don't see any DRM file descriptors
>> being opened without O_CLOEXEC, so they aren't getting passed to sh or
>> xkbcomp.
> 
> OK then, please put me on CC when you send this patch for review.

Both patches have already landed on the respective Git master branches.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Grodzovsky]

On 05/22/2018 12:33 PM, Michel Dänzer wrote:
> On 2018-05-22 06:30 PM, Andrey Grodzovsky wrote:
>> On 05/22/2018 12:09 PM, Michel Dänzer wrote:
>>> On 2018-05-22 05:49 PM, Andrey Grodzovsky wrote:
>>>> On 05/18/2018 04:46 AM, Michel Dänzer wrote:
>>>>> On 2018-05-17 09:05 PM, Andrey Grodzovsky wrote:
>>>>>> On 05/17/2018 10:48 AM, Michel Dänzer wrote:
>>>>>>> On 2018-05-17 01:18 PM, Andrey Grodzovsky wrote:
>>>>>>>> Hi Michele and others, I am trying to implement the approach
>>>>>>>> bellow to
>>>>>>>> resolve AMDGPU's hang when commands are stuck in pipe during process
>>>>>>>> exit.
>>>>>>>>
>>>>>>>> I noticed that once I implemented the file_operation.flush callback
>>>>>>>> then during run of X, i see the flush callback gets called not only
>>>>>>>> for
>>>>>>>> Xorg process but for other
>>>>>>>>
>>>>>>>> processes such as 'xkbcomp' and even 'sh', it seems like Xorg
>>>>>>>> passes his
>>>>>>>> FDs to children, Christian mentioned he remembered a discussion to
>>>>>>>> always set FD_CLOEXEC flag when opening the hardware device file, so
>>>>>>>>
>>>>>>>> we suspect a bug in Xorg with regard to this behavior.
>>>>>>> Try the libdrm patch below.
>>>>>>>
>>>>>>> Note that the X server passes DRM file descriptors to DRI3 clients.
>>>>>> Tried it, didn't help. I still see other processes calling .flush for
>>>>>> /dev/dri/card0
>>>>> Try the attached xserver patch on top. With these patches, I no longer
>>>>> see any DRM file descriptors being opened without O_CLOEXEC running
>>>>> Xorg
>>>>> -pogo in strace.
>>>> Thanks for the patch, unfortunately this is my first time  building xorg
>>>> form source and I hit some blocks with dependencies. I wonder if you
>>>> could quickly apply to amdgpu the attached small patch and run xinit
>>>> from
>>>> command line. In case the FD is not passed any more you will only see
>>>> Xorg print in dmeg afterwards, otherwise 'sh' and 'xkbcomp' will also
>>>> get printed.
>>> As I said above, with these patches I don't see any DRM file descriptors
>>> being opened without O_CLOEXEC, so they aren't getting passed to sh or
>>> xkbcomp.
>> OK then, please put me on CC when you send this patch for review.
> Both patches have already landed on the respective Git master branches.

Good to know.

Andrey

>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel