On 2024-10-09 18:16, Chen, Xiaogang wrote:
On 10/9/2024 4:45 PM, Felix Kuehling wrote:
On 2024-10-09 17:02, Chen, Xiaogang wrote:
On 10/9/2024 3:38 PM, Felix Kuehling wrote:
On 2024-10-09 14:08, Xiaogang.Chen wrote:
From: Xiaogang Chen <xiaogang.chen@xxxxxxx>
kfd process kref count(process->ref) is initialized to 1 by
kref_init. After
it is created not need to increaes its kref. Instad add kfd
process kref at kfd
process mmu notifier allocation since we decrease the ref at
free_notifier of
mmu_notifier_ops, so pair them.
That's not correct. kfd_create_process returns a struct kfd_process
* reference. That gets stored by the caller in filep->private_data.
That requires incrementing the reference count. You can have
multiple references to the same struct kfd_process if user mode
opens /dev/kfd multiple times. The reference is released in
kfd_release. Your change breaks that use case.
ok, if user mode open and close /dev/kfd multiple times(current
Thunk only allows user process open the kfd node once) the change
will break this use case.
The reference released in kfd_process_free_notifier is only one per
process and it's the reference created by kref_init.
I think we can increase kref if find_process return true(the user
process already created kfd process). If find_process return false
do not increase kref since kref_init has been set to 1.
Or change find_process(thread, false) to find_process(thread, true)
that will increase kref if it finds kfd process has been created.
The idea is to pair kref update between alloc_notifier and
free_notifier of mmu_notifier_ops for same process(mm). That would
seem natural.
What's the problem you're trying to solve? Is it just a cosmetic
issue? The MMU notifier is registered in create_process (not
kfd_create_process). If you add a kref_get in
kfd_process_alloc_notifier you need to kfd_unref_process somewhere in
create_process. I don't think it's worth the trouble and only risks
introducing more reference counting bugs.
It is for making code cleaner or natural to read. mmu_notifier_get is
the last call at create_process. If mmu_notifier_get fail the process
is freed: kfree(process). If create_process success
kfd_create_process return that process anyway(after create_process
kfd_create_process creates sys entries that not affect return created
kfd process). The finally result is same that kref is 2: one for kfd
process creation, one for mmu notifier allocation.
Currently, when you call kfd_create_process for the first time, it
returns with kref=2. One reference for the MMU notifier, and one for
file->private_data.
Subsequent invocations of kfd_create_process when the process already
exists should increment the kref by one to track the additional
reference put into the new file->private_data.
If you can come up with a patch that preserves this logic _and makes the
code simpler and more readable_, I will consider approving it. Also keep
in mind that your patch would need to be ported to the DKMS branch,
where there are two different code paths to support older kernels that
don't have mmu_notifier_get/put.
Regards,
Felix
Regards
Xiaogang
Regards,
Felix
Regards
Xiaogang
Regards,
Felix
Signed-off-by: Xiaogang Chen <Xiaogang.Chen@xxxxxxx>
---
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index d07acf1b2f93..7c5471d7d743 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -899,8 +899,6 @@ struct kfd_process *kfd_create_process(struct
task_struct *thread)
init_waitqueue_head(&process->wait_irq_drain);
}
out:
- if (!IS_ERR(process))
- kref_get(&process->ref);
mutex_unlock(&kfd_processes_mutex);
mmput(thread->mm);
@@ -1191,7 +1189,11 @@ static struct mmu_notifier
*kfd_process_alloc_notifier(struct mm_struct *mm)
srcu_read_unlock(&kfd_processes_srcu, idx);
- return p ? &p->mmu_notifier : ERR_PTR(-ESRCH);
+ if (p) {
+ kref_get(&p->ref);
+ return &p->mmu_notifier;
+ }
+ return ERR_PTR(-ESRCH);
}
static void kfd_process_free_notifier(struct mmu_notifier *mn)
