[PATCH 0/3] Remove process exit error message

[PATCH 0/3] Remove process exit error message

[Philip Yang]

This series fix the dmesg error message "still active bo inside vm" and
2 potential races when process exit and vm cleanup.

Philip Yang (3):
  drm/amdgpu: seq64 memory unmap uses uninterruptible lock
  drm/amdgpu: amdgpu_vm_fini hold vm lock to access vm->va
  drm/amdkfd: destroy_pdds release pdd->drm_file at end

 drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    |  8 ++++----
 drivers/gpu/drm/amd/amdkfd/kfd_process.c  | 10 +++++++---
 3 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.49.0

[PATCH 1/3] drm/amdgpu: seq64 memory unmap uses uninterruptible lock

[Philip Yang]

To unmap and free seq64 memory when drm node close to free vm, if there
is signal accepted, then taking vm lock failed and leaking seq64 va
mapping, and then dmesg has error log "still active bo inside vm".

Change to use uninterruptible lock fix the mapping leaking and no dmesg
error log.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
index 3939761be31c..d45ebfb642ca 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
@@ -139,7 +139,7 @@ void amdgpu_seq64_unmap(struct amdgpu_device *adev, struct amdgpu_fpriv *fpriv)
 
 	vm = &fpriv->vm;
 
-	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
+	drm_exec_init(&exec, 0, 0);
 	drm_exec_until_all_locked(&exec) {
 		r = amdgpu_vm_lock_pd(vm, &exec, 0);
 		if (likely(!r))
-- 
2.49.0

[PATCH 2/3] drm/amdgpu: amdgpu_vm_fini hold vm lock to access vm->va

[Philip Yang]

Move vm root bo unreserve after vm->va mapping free because we should
hold vm lock to access vm->va.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3911c78f8282..fb5baa6ec32d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2740,10 +2740,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	}
 
 	amdgpu_vm_pt_free_root(adev, vm);
-	amdgpu_bo_unreserve(root);
-	amdgpu_bo_unref(&root);
-	WARN_ON(vm->root.bo);
-
 	amdgpu_vm_fini_entities(vm);
 
 	if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
@@ -2758,6 +2754,10 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		kfree(mapping);
 	}
 
+	amdgpu_bo_unreserve(root);
+	amdgpu_bo_unref(&root);
+	WARN_ON(vm->root.bo);
+
 	dma_fence_put(vm->last_update);
 
 	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
-- 
2.49.0

[PATCH 3/3] drm/amdkfd: destroy_pdds release pdd->drm_file at end

[Philip Yang]

Release pdd->drm_file may free the vm if this is the last reference,
move it to the last step after memory is unmapped.

Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index e868cc8da46f..b009c852180d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1063,9 +1063,6 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
 		kfd_process_device_destroy_cwsr_dgpu(pdd);
 		kfd_process_device_destroy_ib_mem(pdd);
 
-		if (pdd->drm_file)
-			fput(pdd->drm_file);
-
 		if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
 			free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
 				get_order(KFD_CWSR_TBA_TMA_SIZE));
@@ -1088,6 +1085,13 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
 			pdd->runtime_inuse = false;
 		}
 
+		/*
+		 * This may release the vm if application already close the drm node,
+		 * do it as last step after memory unmapped.
+		 */
+		if (pdd->drm_file)
+			fput(pdd->drm_file);
+
 		kfree(pdd);
 		p->pdds[i] = NULL;
 	}
-- 
2.49.0

Re: [PATCH 2/3] drm/amdgpu: amdgpu_vm_fini hold vm lock to access vm->va

[Christian König]

On 5/14/25 19:10, Philip Yang wrote:
> Move vm root bo unreserve after vm->va mapping free because we should
> hold vm lock to access vm->va.

That should be unnecessary since we are about to destroy the VM.

If anybody is concurrently using it at that point we are completely busted anyway.

Regards,
Christian.

> 
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3911c78f8282..fb5baa6ec32d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2740,10 +2740,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>  	}
>  
>  	amdgpu_vm_pt_free_root(adev, vm);
> -	amdgpu_bo_unreserve(root);
> -	amdgpu_bo_unref(&root);
> -	WARN_ON(vm->root.bo);
> -
>  	amdgpu_vm_fini_entities(vm);
>  
>  	if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
> @@ -2758,6 +2754,10 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>  		kfree(mapping);
>  	}
>  
> +	amdgpu_bo_unreserve(root);
> +	amdgpu_bo_unref(&root);
> +	WARN_ON(vm->root.bo);
> +
>  	dma_fence_put(vm->last_update);
>  
>  	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {

Re: [PATCH 3/3] drm/amdkfd: destroy_pdds release pdd->drm_file at end

[Chen, Xiaogang]

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

Does this patch fix a bug or just make code look more reasonable? 
kfd_process_destroy_pdds releases pdd related buffers, not related to 
operations on vm. So vm tear down dose not affect this function.

Regards

Xiaogang

On 5/14/2025 12:10 PM, Philip Yang wrote:
> Release pdd->drm_file may free the vm if this is the last reference,
> move it to the last step after memory is unmapped.
>
> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index e868cc8da46f..b009c852180d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -1063,9 +1063,6 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>   		kfd_process_device_destroy_cwsr_dgpu(pdd);
>   		kfd_process_device_destroy_ib_mem(pdd);
>   
> -		if (pdd->drm_file)
> -			fput(pdd->drm_file);
> -
>   		if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
>   			free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
>   				get_order(KFD_CWSR_TBA_TMA_SIZE));
> @@ -1088,6 +1085,13 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>   			pdd->runtime_inuse = false;
>   		}
>   
> +		/*
> +		 * This may release the vm if application already close the drm node,
> +		 * do it as last step after memory unmapped.
> +		 */
> +		if (pdd->drm_file)
> +			fput(pdd->drm_file);
> +
>   		kfree(pdd);
>   		p->pdds[i] = NULL;
>   	}

[-- Attachment #2: Type: text/html, Size: 2110 bytes --]

Re: [PATCH 2/3] drm/amdgpu: amdgpu_vm_fini hold vm lock to access vm->va

[Chen, Xiaogang]

On 5/14/2025 12:10 PM, Philip Yang wrote:
> Move vm root bo unreserve after vm->va mapping free because we should
> hold vm lock to access vm->va.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3911c78f8282..fb5baa6ec32d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2740,10 +2740,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	}
>   
>   	amdgpu_vm_pt_free_root(adev, vm);
> -	amdgpu_bo_unreserve(root);
> -	amdgpu_bo_unref(&root);
> -	WARN_ON(vm->root.bo);
> -
>   	amdgpu_vm_fini_entities(vm);
>   
>   	if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
> @@ -2758,6 +2754,10 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		kfree(mapping);
>   	}
>   
> +	amdgpu_bo_unreserve(root);
> +	amdgpu_bo_unref(&root);
> +	WARN_ON(vm->root.bo);
> +

if the places of unresrved/unref vm root here do matter putting them at 
end of this function may look more reasonable.

Regards

Xiaogang


>   	dma_fence_put(vm->last_update);
>   
>   	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {

Re: [PATCH 2/3] drm/amdgpu: amdgpu_vm_fini hold vm lock to access vm->va

[Philip Yang]

On 2025-05-15 10:40, Chen, Xiaogang wrote:
>
> On 5/14/2025 12:10 PM, Philip Yang wrote:
>> Move vm root bo unreserve after vm->va mapping free because we should
>> hold vm lock to access vm->va.
>>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3911c78f8282..fb5baa6ec32d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2740,10 +2740,6 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>       }
>>         amdgpu_vm_pt_free_root(adev, vm);
>> -    amdgpu_bo_unreserve(root);
>> -    amdgpu_bo_unref(&root);
>> -    WARN_ON(vm->root.bo);
>> -
>>       amdgpu_vm_fini_entities(vm);
>>         if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
>> @@ -2758,6 +2754,10 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>           kfree(mapping);
>>       }
>>   +    amdgpu_bo_unreserve(root);
>> +    amdgpu_bo_unref(&root);
>> +    WARN_ON(vm->root.bo);
>> +
>
> if the places of unresrved/unref vm root here do matter putting them 
> at end of this function may look more reasonable.

No, this change doesn't fix any issue, as Christian stated, we should 
not have concurrent using of vm inside this function, hold root bo lock 
is only required by assert in amdgpu_vm_pt_free_root. I will drop this 
patch.

Regards,

Philip

>
> Regards
>
> Xiaogang
>
>
>>       dma_fence_put(vm->last_update);
>>         for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {

Re: [PATCH 3/3] drm/amdkfd: destroy_pdds release pdd->drm_file at end

[Philip Yang]

On 2025-05-15 10:29, Chen, Xiaogang wrote:
>
> Does this patch fix a bug or just make code look more reasonable? 
> kfd_process_destroy_pdds releases pdd related buffers, not related to 
> operations on vm. So vm tear down dose not affect this function.
>
This change doesn't fix anything currently, as fput(pdd->drm_file) to 
free vm is right between free vm mapping qpd->cwsr_mem, qpd->ib_mem and 
free kernel bo qpd->proc_doorbells, pdd->proc_ctx_bo, to make it clear 
for future change.

Regards,

Philip

> Regards
>
> Xiaogang
>
> On 5/14/2025 12:10 PM, Philip Yang wrote:
>> Release pdd->drm_file may free the vm if this is the last reference,
>> move it to the last step after memory is unmapped.
>>
>> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index e868cc8da46f..b009c852180d 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -1063,9 +1063,6 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>>   		kfd_process_device_destroy_cwsr_dgpu(pdd);
>>   		kfd_process_device_destroy_ib_mem(pdd);
>>   
>> -		if (pdd->drm_file)
>> -			fput(pdd->drm_file);
>> -
>>   		if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
>>   			free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
>>   				get_order(KFD_CWSR_TBA_TMA_SIZE));
>> @@ -1088,6 +1085,13 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>>   			pdd->runtime_inuse = false;
>>   		}
>>   
>> +		/*
>> +		 * This may release the vm if application already close the drm node,
>> +		 * do it as last step after memory unmapped.
>> +		 */
>> +		if (pdd->drm_file)
>> +			fput(pdd->drm_file);
>> +
>>   		kfree(pdd);
>>   		p->pdds[i] = NULL;
>>   	}

Re: [PATCH 3/3] drm/amdkfd: destroy_pdds release pdd->drm_file at end

[Chen, Xiaogang]

On 5/15/2025 3:45 PM, Philip Yang wrote:
>
> On 2025-05-15 10:29, Chen, Xiaogang wrote:
>>
>> Does this patch fix a bug or just make code look more reasonable? 
>> kfd_process_destroy_pdds releases pdd related buffers, not related to 
>> operations on vm. So vm tear down dose not affect this function.
>>
> This change doesn't fix anything currently, as fput(pdd->drm_file) to 
> free vm is right between free vm mapping qpd->cwsr_mem, qpd->ib_mem 
> and free kernel bo qpd->proc_doorbells, pdd->proc_ctx_bo, to make it 
> clear for future change.

Then the current place to do fput(pdd->drm_file) make more sense: unmap 
vm mapping of qpd->cwsr_mem, qpd->ib_mem is the last place where kfd 
process release procedure needs vm alive. After that the kfd process 
release does not need vm alive. It then releases remaining buffers. So 
release drm_file as soon as we do not need hold it.

Regards

Xiaogang

> Regards,
>
> Philip
>
>> Regards
>>
>> Xiaogang
>>
>> On 5/14/2025 12:10 PM, Philip Yang wrote:
>>> Release pdd->drm_file may free the vm if this is the last reference,
>>> move it to the last step after memory is unmapped.
>>>
>>> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> index e868cc8da46f..b009c852180d 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> @@ -1063,9 +1063,6 @@ static void kfd_process_destroy_pdds(struct 
>>> kfd_process *p)
>>>           kfd_process_device_destroy_cwsr_dgpu(pdd);
>>>           kfd_process_device_destroy_ib_mem(pdd);
>>>   -        if (pdd->drm_file)
>>> -            fput(pdd->drm_file);
>>> -
>>>           if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
>>>               free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
>>>                   get_order(KFD_CWSR_TBA_TMA_SIZE));
>>> @@ -1088,6 +1085,13 @@ static void kfd_process_destroy_pdds(struct 
>>> kfd_process *p)
>>>               pdd->runtime_inuse = false;
>>>           }
>>>   +        /*
>>> +         * This may release the vm if application already close the 
>>> drm node,
>>> +         * do it as last step after memory unmapped.
>>> +         */
>>> +        if (pdd->drm_file)
>>> +            fput(pdd->drm_file);
>>> +
>>>           kfree(pdd);
>>>           p->pdds[i] = NULL;
>>>       }

Re: [PATCH 3/3] drm/amdkfd: destroy_pdds release pdd->drm_file at end

[Philip Yang]

On 2025-05-15 17:31, Chen, Xiaogang wrote:
>
> On 5/15/2025 3:45 PM, Philip Yang wrote:
>>
>> On 2025-05-15 10:29, Chen, Xiaogang wrote:
>>>
>>> Does this patch fix a bug or just make code look more reasonable? 
>>> kfd_process_destroy_pdds releases pdd related buffers, not related 
>>> to operations on vm. So vm tear down dose not affect this function.
>>>
>> This change doesn't fix anything currently, as fput(pdd->drm_file) to 
>> free vm is right between free vm mapping qpd->cwsr_mem, qpd->ib_mem 
>> and free kernel bo qpd->proc_doorbells, pdd->proc_ctx_bo, to make it 
>> clear for future change.
>
> Then the current place to do fput(pdd->drm_file) make more sense: 
> unmap vm mapping of qpd->cwsr_mem, qpd->ib_mem is the last place where 
> kfd process release procedure needs vm alive. After that the kfd 
> process release does not need vm alive. It then releases remaining 
> buffers. So release drm_file as soon as we do not need hold it.

The issue was  vm_fini shows error message "still active bo inside vm" 
(1/1000) chance, took a while to trace down the leaking vm mapping, the 
issue is seq64 memory mapping leaking and fixed by the first patch. KFD 
pdd cleanup path, free vm is in the middle of free pdd memory, this is 
one of the suspicious vm_fini race. We may add new pdd memory mapping to 
vm in future, to prevent the potential vm_fini race, this patch move 
free vm to after all pdd memory is freed and add comment.

Regards,

Philip

>
> Regards
>
> Xiaogang
>
>> Regards,
>>
>> Philip
>>
>>> Regards
>>>
>>> Xiaogang
>>>
>>> On 5/14/2025 12:10 PM, Philip Yang wrote:
>>>> Release pdd->drm_file may free the vm if this is the last reference,
>>>> move it to the last step after memory is unmapped.
>>>>
>>>> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> index e868cc8da46f..b009c852180d 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> @@ -1063,9 +1063,6 @@ static void kfd_process_destroy_pdds(struct 
>>>> kfd_process *p)
>>>>           kfd_process_device_destroy_cwsr_dgpu(pdd);
>>>>           kfd_process_device_destroy_ib_mem(pdd);
>>>>   -        if (pdd->drm_file)
>>>> -            fput(pdd->drm_file);
>>>> -
>>>>           if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
>>>>               free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
>>>>                   get_order(KFD_CWSR_TBA_TMA_SIZE));
>>>> @@ -1088,6 +1085,13 @@ static void kfd_process_destroy_pdds(struct 
>>>> kfd_process *p)
>>>>               pdd->runtime_inuse = false;
>>>>           }
>>>>   +        /*
>>>> +         * This may release the vm if application already close 
>>>> the drm node,
>>>> +         * do it as last step after memory unmapped.
>>>> +         */
>>>> +        if (pdd->drm_file)
>>>> +            fput(pdd->drm_file);
>>>> +
>>>>           kfree(pdd);
>>>>           p->pdds[i] = NULL;
>>>>       }

Re: [PATCH 3/3] drm/amdkfd: destroy_pdds release pdd->drm_file at end

[Chen, Xiaogang]

On 5/16/2025 8:07 AM, Philip Yang wrote:
>
> On 2025-05-15 17:31, Chen, Xiaogang wrote:
>>
>> On 5/15/2025 3:45 PM, Philip Yang wrote:
>>>
>>> On 2025-05-15 10:29, Chen, Xiaogang wrote:
>>>>
>>>> Does this patch fix a bug or just make code look more reasonable? 
>>>> kfd_process_destroy_pdds releases pdd related buffers, not related 
>>>> to operations on vm. So vm tear down dose not affect this function.
>>>>
>>> This change doesn't fix anything currently, as fput(pdd->drm_file) 
>>> to free vm is right between free vm mapping qpd->cwsr_mem, 
>>> qpd->ib_mem and free kernel bo qpd->proc_doorbells, 
>>> pdd->proc_ctx_bo, to make it clear for future change.
>>
>> Then the current place to do fput(pdd->drm_file) make more sense: 
>> unmap vm mapping of qpd->cwsr_mem, qpd->ib_mem is the last place 
>> where kfd process release procedure needs vm alive. After that the 
>> kfd process release does not need vm alive. It then releases 
>> remaining buffers. So release drm_file as soon as we do not need hold 
>> it.
>
> The issue was  vm_fini shows error message "still active bo inside vm" 
> (1/1000) chance, took a while to trace down the leaking vm mapping, 
> the issue is seq64 memory mapping leaking and fixed by the first 
> patch. KFD pdd cleanup path, free vm is in the middle of free pdd 
> memory, this is one of the suspicious vm_fini race. We may add new pdd 
> memory mapping to vm in future, to prevent the potential vm_fini race, 
> this patch move free vm to after all pdd memory is freed and add comment.

I see the reason of [PATCH 1/3]. This patch is delay kfd's pdd drm_file 
release a bit. kfd should release drm_file as soon as it does not need 
vm. The issue you saw is there is buffer mapping still alive when driver 
decides to tear down vm.  Is the mapping from from kfd process? if not, 
change timing somehow at kfd process release is not right place.

Regards

Xiaogang

>
> Regards,
>
> Philip
>
>>
>> Regards
>>
>> Xiaogang
>>
>>> Regards,
>>>
>>> Philip
>>>
>>>> Regards
>>>>
>>>> Xiaogang
>>>>
>>>> On 5/14/2025 12:10 PM, Philip Yang wrote:
>>>>> Release pdd->drm_file may free the vm if this is the last reference,
>>>>> move it to the last step after memory is unmapped.
>>>>>
>>>>> Signed-off-by: Philip Yang<Philip.Yang@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 +++++++---
>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> index e868cc8da46f..b009c852180d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> @@ -1063,9 +1063,6 @@ static void kfd_process_destroy_pdds(struct 
>>>>> kfd_process *p)
>>>>>           kfd_process_device_destroy_cwsr_dgpu(pdd);
>>>>>           kfd_process_device_destroy_ib_mem(pdd);
>>>>>   -        if (pdd->drm_file)
>>>>> -            fput(pdd->drm_file);
>>>>> -
>>>>>           if (pdd->qpd.cwsr_kaddr && !pdd->qpd.cwsr_base)
>>>>>               free_pages((unsigned long)pdd->qpd.cwsr_kaddr,
>>>>>                   get_order(KFD_CWSR_TBA_TMA_SIZE));
>>>>> @@ -1088,6 +1085,13 @@ static void kfd_process_destroy_pdds(struct 
>>>>> kfd_process *p)
>>>>>               pdd->runtime_inuse = false;
>>>>>           }
>>>>>   +        /*
>>>>> +         * This may release the vm if application already close 
>>>>> the drm node,
>>>>> +         * do it as last step after memory unmapped.
>>>>> +         */
>>>>> +        if (pdd->drm_file)
>>>>> +            fput(pdd->drm_file);
>>>>> +
>>>>>           kfree(pdd);
>>>>>           p->pdds[i] = NULL;
>>>>>       }

Re: [PATCH 1/3] drm/amdgpu: seq64 memory unmap uses uninterruptible lock

[Christian König]

On 5/14/25 19:10, Philip Yang wrote:
> To unmap and free seq64 memory when drm node close to free vm, if there
> is signal accepted, then taking vm lock failed and leaking seq64 va
> mapping, and then dmesg has error log "still active bo inside vm".
> 
> Change to use uninterruptible lock fix the mapping leaking and no dmesg
> error log.
> 
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>

I'm not 100% sure but I think I've seen the same patch from Arun before. So don't be surprised if that is already fixed on amd-stangin-drm-next.

Anyway feel free to add Reviewed-by: Christian König <christian.koenig@amd.com> and push to amd-staging-drm-next since that change is certainly correct.

Regards,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> index 3939761be31c..d45ebfb642ca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
> @@ -139,7 +139,7 @@ void amdgpu_seq64_unmap(struct amdgpu_device *adev, struct amdgpu_fpriv *fpriv)
>  
>  	vm = &fpriv->vm;
>  
> -	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
> +	drm_exec_init(&exec, 0, 0);
>  	drm_exec_until_all_locked(&exec) {
>  		r = amdgpu_vm_lock_pd(vm, &exec, 0);
>  		if (likely(!r))