| Message ID | 20240920090920.1342694-5-pierre-eric.pelloux-prayer@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM_SET_NAME ioctl | expand |
On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote: > This will allow to use flexible array to store the process name and > other information. > > This also means that process name will be determined once and for all, > instead of at each submit. But the pid and others can still change? By design? > Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index e20d19ae01b2..690676cab022 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -2331,7 +2331,7 @@ amdgpu_vm_get_task_info_vm(struct amdgpu_vm *vm) > { > struct amdgpu_task_info *ti = NULL; > > - if (vm) { > + if (vm && vm->task_info) { > ti = vm->task_info; > kref_get(&vm->task_info->refcount); > } > @@ -2372,8 +2372,12 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) > */ > void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) > { > - if (!vm->task_info) > - return; > + if (!vm->task_info) { > + if (amdgpu_vm_create_task_info(vm)) > + return; > + > + get_task_comm(vm->task_info->process_name, current->group_leader); > + } > > if (vm->task_info->pid == current->pid) This ends up relying on vm->task_info->pid being zero due kzalloc right? > return; > @@ -2385,7 +2389,6 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) > return; > > vm->task_info->tgid = current->group_leader->pid; > - get_task_comm(vm->task_info->process_name, current->group_leader); > } I wonder how many of the task_info fields you want to set once instead of per submission. Like a fully one shot like the below be what you want? diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index a060c28f0877..da492223a8b5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2349,16 +2349,6 @@ amdgpu_vm_get_task_info_pasid(struct amdgpu_device *adev, u32 pasid) amdgpu_vm_get_vm_from_pasid(adev, pasid)); } -static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) -{ - vm->task_info = kzalloc(sizeof(struct amdgpu_task_info), GFP_KERNEL); - if (!vm->task_info) - return -ENOMEM; - - kref_init(&vm->task_info->refcount); - return 0; -} - /** * amdgpu_vm_set_task_info - Sets VMs task info. * @@ -2366,20 +2356,28 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) */ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) { - if (!vm->task_info) - return; + struct amdgpu_task_info *task_info = vm->task_info; + + if (!task_info) { + task_info = kzalloc(sizeof(struct amdgpu_task_info), + GFP_KERNEL); + if (!task_info) + return; - if (vm->task_info->pid == current->pid) + kref_init(&task_info->refcount); + } else { return; + } - vm->task_info->pid = current->pid; - get_task_comm(vm->task_info->task_name, current); + task_info->pid = current->pid; + get_task_comm(task_info->task_name, current); - if (current->group_leader->mm != current->mm) - return; + if (current->group_leader->mm == current->mm) { + task_info->tgid = current->group_leader->pid; + get_task_comm(task_info->process_name, current->group_leader); + } - vm->task_info->tgid = current->group_leader->pid; - get_task_comm(vm->task_info->process_name, current->group_leader); + vm->task_info = task_info; } /** End result is code like this: void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) { struct amdgpu_task_info *task_info = vm->task_info; if (!task_info) { task_info = kzalloc(sizeof(struct amdgpu_task_info), GFP_KERNEL); if (!task_info) return; kref_init(&task_info->refcount); } else { return; } task_info->pid = current->pid; get_task_comm(task_info->task_name, current); if (current->group_leader->mm == current->mm) { task_info->tgid = current->group_leader->pid; get_task_comm(task_info->process_name, current->group_leader); } vm->task_info = task_info; } ? > > /** > @@ -2482,7 +2485,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, > if (r) > goto error_free_root; > > - r = amdgpu_vm_create_task_info(vm); > if (r) > DRM_DEBUG("Failed to create task info for VM\n"); Two more lines to delete here. Regards, Tvrtko > > @@ -2608,7 +2610,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) > > root = amdgpu_bo_ref(vm->root.bo); > amdgpu_bo_reserve(root, true); > - amdgpu_vm_put_task_info(vm->task_info); > + if (vm->task_info) > + amdgpu_vm_put_task_info(vm->task_info); > amdgpu_vm_set_pasid(adev, vm, 0); > dma_fence_wait(vm->last_unlocked, false); > dma_fence_put(vm->last_unlocked);
Le 23/09/2024 à 12:58, Tvrtko Ursulin a écrit : > > On 20/09/2024 10:06, Pierre-Eric Pelloux-Prayer wrote: >> This will allow to use flexible array to store the process name and >> other information. >> >> This also means that process name will be determined once and for all, >> instead of at each submit. > > But the pid and others can still change? By design? pid and task_name can change, yes. tgid could be set once and for all I think. > >> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> index e20d19ae01b2..690676cab022 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >> @@ -2331,7 +2331,7 @@ amdgpu_vm_get_task_info_vm(struct amdgpu_vm *vm) >> { >> struct amdgpu_task_info *ti = NULL; >> - if (vm) { >> + if (vm && vm->task_info) { >> ti = vm->task_info; >> kref_get(&vm->task_info->refcount); >> } >> @@ -2372,8 +2372,12 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) >> */ >> void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) >> { >> - if (!vm->task_info) >> - return; >> + if (!vm->task_info) { >> + if (amdgpu_vm_create_task_info(vm)) >> + return; >> + >> + get_task_comm(vm->task_info->process_name, current->group_leader); >> + } >> if (vm->task_info->pid == current->pid) > > This ends up relying on vm->task_info->pid being zero due kzalloc right? Yes. > >> return; >> @@ -2385,7 +2389,6 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) >> return; >> vm->task_info->tgid = current->group_leader->pid; >> - get_task_comm(vm->task_info->process_name, current->group_leader); >> } > > I wonder how many of the task_info fields you want to set once instead of per submission. Like a > fully one shot like the below be what you want? As written above, process name, drm client name and pid (tgid) can be set once. Task name + tid are updated on submit. I've updated slightly this part, so v4 should hopefully be clearer. > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index a060c28f0877..da492223a8b5 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -2349,16 +2349,6 @@ amdgpu_vm_get_task_info_pasid(struct amdgpu_device *adev, u32 pasid) > amdgpu_vm_get_vm_from_pasid(adev, pasid)); > } > > -static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) > -{ > - vm->task_info = kzalloc(sizeof(struct amdgpu_task_info), GFP_KERNEL); > - if (!vm->task_info) > - return -ENOMEM; > - > - kref_init(&vm->task_info->refcount); > - return 0; > -} > - > /** > * amdgpu_vm_set_task_info - Sets VMs task info. > * > @@ -2366,20 +2356,28 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) > */ > void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) > { > - if (!vm->task_info) > - return; > + struct amdgpu_task_info *task_info = vm->task_info; > + > + if (!task_info) { > + task_info = kzalloc(sizeof(struct amdgpu_task_info), > + GFP_KERNEL); > + if (!task_info) > + return; > > - if (vm->task_info->pid == current->pid) > + kref_init(&task_info->refcount); > + } else { > return; > + } > > - vm->task_info->pid = current->pid; > - get_task_comm(vm->task_info->task_name, current); > + task_info->pid = current->pid; > + get_task_comm(task_info->task_name, current); > > - if (current->group_leader->mm != current->mm) > - return; > + if (current->group_leader->mm == current->mm) { > + task_info->tgid = current->group_leader->pid; > + get_task_comm(task_info->process_name, current->group_leader); > + } > > - vm->task_info->tgid = current->group_leader->pid; > - get_task_comm(vm->task_info->process_name, current->group_leader); > + vm->task_info = task_info; > } > > /** > > End result is code like this: > > void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) > { > struct amdgpu_task_info *task_info = vm->task_info; > > if (!task_info) { > task_info = kzalloc(sizeof(struct amdgpu_task_info), > GFP_KERNEL); > if (!task_info) > return; > > kref_init(&task_info->refcount); > } else { > return; > } > > task_info->pid = current->pid; > get_task_comm(task_info->task_name, current); > > if (current->group_leader->mm == current->mm) { > task_info->tgid = current->group_leader->pid; > get_task_comm(task_info->process_name, current->group_leader); > } > > vm->task_info = task_info; > } > > ? > >> /** >> @@ -2482,7 +2485,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, >> if (r) >> goto error_free_root; >> - r = amdgpu_vm_create_task_info(vm); >> if (r) >> DRM_DEBUG("Failed to create task info for VM\n"); > > Two more lines to delete here. Done, thanks. Pierre-Eric > > Regards, > > Tvrtko > >> @@ -2608,7 +2610,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) >> root = amdgpu_bo_ref(vm->root.bo); >> amdgpu_bo_reserve(root, true); >> - amdgpu_vm_put_task_info(vm->task_info); >> + if (vm->task_info) >> + amdgpu_vm_put_task_info(vm->task_info); >> amdgpu_vm_set_pasid(adev, vm, 0); >> dma_fence_wait(vm->last_unlocked, false); >> dma_fence_put(vm->last_unlocked);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index e20d19ae01b2..690676cab022 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2331,7 +2331,7 @@ amdgpu_vm_get_task_info_vm(struct amdgpu_vm *vm) { struct amdgpu_task_info *ti = NULL; - if (vm) { + if (vm && vm->task_info) { ti = vm->task_info; kref_get(&vm->task_info->refcount); } @@ -2372,8 +2372,12 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm) */ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) { - if (!vm->task_info) - return; + if (!vm->task_info) { + if (amdgpu_vm_create_task_info(vm)) + return; + + get_task_comm(vm->task_info->process_name, current->group_leader); + } if (vm->task_info->pid == current->pid) return; @@ -2385,7 +2389,6 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm) return; vm->task_info->tgid = current->group_leader->pid; - get_task_comm(vm->task_info->process_name, current->group_leader); } /** @@ -2482,7 +2485,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (r) goto error_free_root; - r = amdgpu_vm_create_task_info(vm); if (r) DRM_DEBUG("Failed to create task info for VM\n"); @@ -2608,7 +2610,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) root = amdgpu_bo_ref(vm->root.bo); amdgpu_bo_reserve(root, true); - amdgpu_vm_put_task_info(vm->task_info); + if (vm->task_info) + amdgpu_vm_put_task_info(vm->task_info); amdgpu_vm_set_pasid(adev, vm, 0); dma_fence_wait(vm->last_unlocked, false); dma_fence_put(vm->last_unlocked);
This will allow to use flexible array to store the process name and other information. This also means that process name will be determined once and for all, instead of at each submit. Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)