[PATCH] drm/sched: Avoid killing entity last used by parent on child SIGKILL

[PATCH] drm/sched: Avoid killing entity last used by parent on child SIGKILL

[David Rosca]

drm_sched_entity_flush should only kill the entity if the current
process is the last user of the entity. The last_user is only set
when adding new job, so entities that had no jobs submitted to them
have NULL last_user and would always be killed.
Another issue is setting last_user to NULL from drm_sched_entity_flush,
which causes subsequent calls to kill the entity.

Signed-off-by: David Rosca <david.rosca@amd.com>
Fixes: 51564e9f06f0 ("drm/amdgpu: Avoid extra evict-restore process.")
---
 drivers/gpu/drm/scheduler/sched_entity.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 8867b95ab089..a325e4a59990 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -70,6 +70,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
+	entity->last_user = current->group_leader;
 	/*
 	 * It's perfectly valid to initialize an entity without having a valid
 	 * scheduler attached. It's just not valid to use the scheduler before it
@@ -278,7 +279,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
 long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
 {
 	struct drm_gpu_scheduler *sched;
-	struct task_struct *last_user;
 	long ret = timeout;
 
 	if (!entity->rq)
@@ -301,8 +301,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
 	}
 
 	/* For killed process disable any more IBs enqueue right now */
-	last_user = cmpxchg(&entity->last_user, current->group_leader, NULL);
-	if ((!last_user || last_user == current->group_leader) &&
+	if (entity->last_user == current->group_leader &&
 	    (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))
 		drm_sched_entity_kill(entity);
 
-- 
2.43.0

Re: [PATCH] drm/sched: Avoid killing entity last used by parent on child SIGKILL

[Christian König]

On 02.10.25 17:05, David Rosca wrote:
> drm_sched_entity_flush should only kill the entity if the current
> process is the last user of the entity. The last_user is only set
> when adding new job, so entities that had no jobs submitted to them
> have NULL last_user and would always be killed.
> Another issue is setting last_user to NULL from drm_sched_entity_flush,
> which causes subsequent calls to kill the entity.
> 
> Signed-off-by: David Rosca <david.rosca@amd.com>
> Fixes: 51564e9f06f0 ("drm/amdgpu: Avoid extra evict-restore process.")

Good catch, but in general please CC the relevant maintainers and mailing lists for scheduler patches.

> ---
>  drivers/gpu/drm/scheduler/sched_entity.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 8867b95ab089..a325e4a59990 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -70,6 +70,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>  	entity->guilty = guilty;
>  	entity->num_sched_list = num_sched_list;
>  	entity->priority = priority;
> +	entity->last_user = current->group_leader;
>  	/*
>  	 * It's perfectly valid to initialize an entity without having a valid
>  	 * scheduler attached. It's just not valid to use the scheduler before it
> @@ -278,7 +279,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>  long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>  {
>  	struct drm_gpu_scheduler *sched;
> -	struct task_struct *last_user;
>  	long ret = timeout;
>  
>  	if (!entity->rq)
> @@ -301,8 +301,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>  	}
>  
>  	/* For killed process disable any more IBs enqueue right now */
> -	last_user = cmpxchg(&entity->last_user, current->group_leader, NULL);
> -	if ((!last_user || last_user == current->group_leader) &&
> +	if (entity->last_user == current->group_leader &&

You still need the cmpxchg() here or otherwise drm_sched_entity_kill() would run multiple times.

Regards,
Christian.

>  	    (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))
>  		drm_sched_entity_kill(entity);
>  

Re: [PATCH] drm/sched: Avoid killing entity last used by parent on child SIGKILL

[Christian König]

FYI

On 09.10.25 09:01, Christian König wrote:
> On 02.10.25 17:05, David Rosca wrote:
>> drm_sched_entity_flush should only kill the entity if the current
>> process is the last user of the entity. The last_user is only set
>> when adding new job, so entities that had no jobs submitted to them
>> have NULL last_user and would always be killed.
>> Another issue is setting last_user to NULL from drm_sched_entity_flush,
>> which causes subsequent calls to kill the entity.
>>
>> Signed-off-by: David Rosca <david.rosca@amd.com>
>> Fixes: 51564e9f06f0 ("drm/amdgpu: Avoid extra evict-restore process.")
> 
> Good catch, but in general please CC the relevant maintainers and mailing lists for scheduler patches.
> 
>> ---
>>  drivers/gpu/drm/scheduler/sched_entity.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 8867b95ab089..a325e4a59990 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -70,6 +70,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>  	entity->guilty = guilty;
>>  	entity->num_sched_list = num_sched_list;
>>  	entity->priority = priority;
>> +	entity->last_user = current->group_leader;
>>  	/*
>>  	 * It's perfectly valid to initialize an entity without having a valid
>>  	 * scheduler attached. It's just not valid to use the scheduler before it
>> @@ -278,7 +279,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>  long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>  {
>>  	struct drm_gpu_scheduler *sched;
>> -	struct task_struct *last_user;
>>  	long ret = timeout;
>>  
>>  	if (!entity->rq)
>> @@ -301,8 +301,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>  	}
>>  
>>  	/* For killed process disable any more IBs enqueue right now */
>> -	last_user = cmpxchg(&entity->last_user, current->group_leader, NULL);
>> -	if ((!last_user || last_user == current->group_leader) &&
>> +	if (entity->last_user == current->group_leader &&
> 
> You still need the cmpxchg() here or otherwise drm_sched_entity_kill() would run multiple times.
> 
> Regards,
> Christian.
> 
>>  	    (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))
>>  		drm_sched_entity_kill(entity);
>>  
> 

Re: [PATCH] drm/sched: Avoid killing entity last used by parent on child SIGKILL

[Christian König]

David any objections that I take this patch and make the necessary modifications?

People are pinging me about the problem.

Regards,
Christian.

On 09.10.25 17:04, Christian König wrote:
> FYI
> 
> On 09.10.25 09:01, Christian König wrote:
>> On 02.10.25 17:05, David Rosca wrote:
>>> drm_sched_entity_flush should only kill the entity if the current
>>> process is the last user of the entity. The last_user is only set
>>> when adding new job, so entities that had no jobs submitted to them
>>> have NULL last_user and would always be killed.
>>> Another issue is setting last_user to NULL from drm_sched_entity_flush,
>>> which causes subsequent calls to kill the entity.
>>>
>>> Signed-off-by: David Rosca <david.rosca@amd.com>
>>> Fixes: 51564e9f06f0 ("drm/amdgpu: Avoid extra evict-restore process.")
>>
>> Good catch, but in general please CC the relevant maintainers and mailing lists for scheduler patches.
>>
>>> ---
>>>  drivers/gpu/drm/scheduler/sched_entity.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 8867b95ab089..a325e4a59990 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -70,6 +70,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>>  	entity->guilty = guilty;
>>>  	entity->num_sched_list = num_sched_list;
>>>  	entity->priority = priority;
>>> +	entity->last_user = current->group_leader;
>>>  	/*
>>>  	 * It's perfectly valid to initialize an entity without having a valid
>>>  	 * scheduler attached. It's just not valid to use the scheduler before it
>>> @@ -278,7 +279,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>>  long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>>  {
>>>  	struct drm_gpu_scheduler *sched;
>>> -	struct task_struct *last_user;
>>>  	long ret = timeout;
>>>  
>>>  	if (!entity->rq)
>>> @@ -301,8 +301,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>>  	}
>>>  
>>>  	/* For killed process disable any more IBs enqueue right now */
>>> -	last_user = cmpxchg(&entity->last_user, current->group_leader, NULL);
>>> -	if ((!last_user || last_user == current->group_leader) &&
>>> +	if (entity->last_user == current->group_leader &&
>>
>> You still need the cmpxchg() here or otherwise drm_sched_entity_kill() would run multiple times.
>>
>> Regards,
>> Christian.
>>
>>>  	    (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))
>>>  		drm_sched_entity_kill(entity);
>>>  
>>
> 

Re: [PATCH] drm/sched: Avoid killing entity last used by parent on child SIGKILL

[David Rosca]

On 10. 10. 25 14:28, Christian König wrote:
> David any objections that I take this patch and make the necessary modifications?

Sure, please go ahead.

Thanks,
David

>
> People are pinging me about the problem.
>
> Regards,
> Christian.
>
> On 09.10.25 17:04, Christian König wrote:
>> FYI
>>
>> On 09.10.25 09:01, Christian König wrote:
>>> On 02.10.25 17:05, David Rosca wrote:
>>>> drm_sched_entity_flush should only kill the entity if the current
>>>> process is the last user of the entity. The last_user is only set
>>>> when adding new job, so entities that had no jobs submitted to them
>>>> have NULL last_user and would always be killed.
>>>> Another issue is setting last_user to NULL from drm_sched_entity_flush,
>>>> which causes subsequent calls to kill the entity.
>>>>
>>>> Signed-off-by: David Rosca <david.rosca@amd.com>
>>>> Fixes: 51564e9f06f0 ("drm/amdgpu: Avoid extra evict-restore process.")
>>> Good catch, but in general please CC the relevant maintainers and mailing lists for scheduler patches.
>>>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_entity.c | 5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>>>> index 8867b95ab089..a325e4a59990 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>> @@ -70,6 +70,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>>>   	entity->guilty = guilty;
>>>>   	entity->num_sched_list = num_sched_list;
>>>>   	entity->priority = priority;
>>>> +	entity->last_user = current->group_leader;
>>>>   	/*
>>>>   	 * It's perfectly valid to initialize an entity without having a valid
>>>>   	 * scheduler attached. It's just not valid to use the scheduler before it
>>>> @@ -278,7 +279,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>>>   long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>>>   {
>>>>   	struct drm_gpu_scheduler *sched;
>>>> -	struct task_struct *last_user;
>>>>   	long ret = timeout;
>>>>   
>>>>   	if (!entity->rq)
>>>> @@ -301,8 +301,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>>>   	}
>>>>   
>>>>   	/* For killed process disable any more IBs enqueue right now */
>>>> -	last_user = cmpxchg(&entity->last_user, current->group_leader, NULL);
>>>> -	if ((!last_user || last_user == current->group_leader) &&
>>>> +	if (entity->last_user == current->group_leader &&
>>> You still need the cmpxchg() here or otherwise drm_sched_entity_kill() would run multiple times.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>   	    (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))
>>>>   		drm_sched_entity_kill(entity);
>>>>