Re: [PATCH v2] drm/amdgpu: change vm->task_info handling

2024-01-24 Thread Felix Kuehling 



On 2024-01-24 9:32, Shashank Sharma wrote:


On 19/01/2024 21:23, Felix Kuehling wrote:


On 2024-01-18 14:21, Shashank Sharma wrote:

This patch changes the handling and lifecycle of vm->task_info object.
The major changes are:
- vm->task_info is a dynamically allocated ptr now, and its uasge is
   reference counted.
- introducing two new helper funcs for task_info lifecycle management
 - amdgpu_vm_get_task_info: reference counts up task_info before
   returning this info
 - amdgpu_vm_put_task_info: reference counts down task_info
- last put to task_info() frees task_info from the vm.

This patch also does logistical changes required for existing usage
of vm->task_info.

V2: Do not block all the prints when task_info not found (Felix)

Cc: Christian Koenig 
Cc: Alex Deucher 
Cc: Felix Kuehling 
Signed-off-by: Shashank Sharma 


Nit-picks inline.


[snip]


+/**
+ * amdgpu_vm_put_task_info_pasid - reference down the vm task_info ptr
+ * frees the vm task_info ptr at the last put
+ *
+ * @adev: drm device pointer
+ * @task_info: task_info struct under discussion.
+ * @pasid: pasid of the VM which contains task_info
+ */
+void amdgpu_vm_put_task_info_pasid(struct amdgpu_device *adev,
+   struct amdgpu_task_info *task_info,
+   u32 pasid)
+{
+    int ret;
+
+    ret = kref_put(_info->refcount, amdgpu_vm_destroy_task_info);
+
+    /* Clean up if object was removed in the last put */
+    if (ret == 1) {
+    struct amdgpu_vm *vm;
+
+    vm = amdgpu_vm_get_vm_from_pasid(adev, pasid);
+    if (!vm) {
+    WARN(1, "Invalid PASID %u to put task info\n", pasid);
+    return;
+    }
+
+    vm->task_info = NULL;


This doesn't make sense. If there is a VM pointing to the task_info, 
then the ref count should not go to 0. Therefore this whole "look up 
the VM from PASID and clear vm->task_info" seams bogus.


Actually, (ret == 1) above indicates that cleanup function has been 
called and task_info has been freed, and the vm->task_info is a 
dangling ptr.


The current design is

- first open per process/fd creates vm->task_info

- last close per process/fd frees vm->task_info



I'd expect the last put_task_info call to come from amdgpu_vm_fini. 
At that point setting task_info to NULL is probably unnecessary. But 
if we wanted that, we could set it to NULL in the caller.


Even this can be done, I can call the first get_vm_info() from vm_init 
and last put from vm_fini.


Well, actually it doesn't matter where the last put comes from. The main 
point is, that vm->task_info is a counted reference. As long as that 
reference exists, the refcount should not become 0. If it does, that's a 
bug somewhere. The vm->task_info reference is only dropped in 
amdgpu_vm_fini, after which the whole vm structure is freed. So there 
should never be a need to set vm->task_info to NULL in 
amdgpu_vm_put_task_info_*.


[snip]

+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) {
+    DRM_ERROR("OOM while creating task_info space\n");


Printing OOM error messages is usually redundant. The allocators are 
already very noisy when OOM happens. I think checkpatch.pl also warns 
about that. Maybe it doesn't catch DRM_ERROR but only printk and 
friends.



Agree, I will make this debug instead of error.


Even a debug message is not needed. See https://lkml.org/lkml/2014/6/10/382.

[snip]

  @@ -157,18 +157,26 @@ static int gmc_v10_0_process_interrupt(struct 
amdgpu_device *adev,

  if (!printk_ratelimit())
  return 0;
  -    memset(_info, 0, sizeof(struct amdgpu_task_info));
-    amdgpu_vm_get_task_info(adev, entry->pasid, _info);
+    task_info = amdgpu_vm_get_task_info_pasid(adev, entry->pasid);
+    if (task_info) {
+    dev_err(adev->dev,
+    "[%s] page fault (src_id:%u ring:%u vmid:%u pasid:%u, 
for process %s pid %d thread %s pid %d)\n",

+    entry->vmid_src ? "mmhub" : "gfxhub",
+    entry->src_id, entry->ring_id, entry->vmid,
+    entry->pasid, task_info->process_name, task_info->tgid,
+    task_info->task_name, task_info->pid);
+    amdgpu_vm_put_task_info_pasid(adev, task_info, entry->pasid);
+    } else {
+    dev_err(adev->dev,
+    "[%s] page fault (src_id:%u ring:%u vmid:%u pasid:%u, 
no process info)\n",

+    entry->vmid_src ? "mmhub" : "gfxhub",
+    entry->src_id, entry->ring_id, entry->vmid,
+    entry->pasid);


Can we avoid the duplication here? It's too easy for them to get out 
of sync. I think it's OK to change the message format so that the 
process into is printed on a separate line. E.g.:


That's exactly I thought, but then I was afraid of breaking any 
existing scripts grepping for this exact text. I guess I can do this 
change and see if anyone complaints :).


I don't think there are any ABI guarantees for log

Re: [PATCH v2] drm/amdgpu: change vm->task_info handling

2024-01-24 Thread Shashank Sharma 



On 19/01/2024 21:23, Felix Kuehling wrote:


On 2024-01-18 14:21, Shashank Sharma wrote:

This patch changes the handling and lifecycle of vm->task_info object.
The major changes are:
- vm->task_info is a dynamically allocated ptr now, and its uasge is
   reference counted.
- introducing two new helper funcs for task_info lifecycle management
 - amdgpu_vm_get_task_info: reference counts up task_info before
   returning this info
 - amdgpu_vm_put_task_info: reference counts down task_info
- last put to task_info() frees task_info from the vm.

This patch also does logistical changes required for existing usage
of vm->task_info.

V2: Do not block all the prints when task_info not found (Felix)

Cc: Christian Koenig 
Cc: Alex Deucher 
Cc: Felix Kuehling 
Signed-off-by: Shashank Sharma 


Nit-picks inline.



---
  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |   7 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  18 ++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c   |  12 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 142 +---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  26 +++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   2 +-
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  30 +++--
  drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c  |  31 +++--
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c   |  22 +--
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   |  26 ++--
  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c  |  26 ++--
  drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c    |  26 ++--
  drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c |  20 +--
  13 files changed, 287 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c

index 0e61ebdb3f3e..99c736b6e32c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1775,9 +1775,12 @@ static int amdgpu_debugfs_vm_info_show(struct 
seq_file *m, void *unused)

  list_for_each_entry(file, >filelist, lhead) {
  struct amdgpu_fpriv *fpriv = file->driver_priv;
  struct amdgpu_vm *vm = >vm;
+    struct amdgpu_task_info *ti;
+
+    ti = amdgpu_vm_get_task_info_vm(vm);


Can ti be NULL here? I think it can, so you'd need a NULL check to 
avoid a possible kernel oops.

Agree



+    seq_printf(m, "pid:%d\tProcess:%s --\n", ti->pid, 
ti->process_name);

+    amdgpu_vm_put_task_info_vm(ti, vm);
  -    seq_printf(m, "pid:%d\tProcess:%s --\n",
-    vm->task_info.pid, vm->task_info.process_name);
  r = amdgpu_bo_reserve(vm->root.bo, true);
  if (r)
  break;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c

index 1f357198533f..af23746821b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -35,7 +35,7 @@ static enum drm_gpu_sched_stat 
amdgpu_job_timedout(struct drm_sched_job *s_job)

  {
  struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
  struct amdgpu_job *job = to_amdgpu_job(s_job);
-    struct amdgpu_task_info ti;
+    struct amdgpu_task_info *ti;
  struct amdgpu_device *adev = ring->adev;
  int idx;
  int r;
@@ -48,7 +48,7 @@ static enum drm_gpu_sched_stat 
amdgpu_job_timedout(struct drm_sched_job *s_job)

  return DRM_GPU_SCHED_STAT_ENODEV;
  }
  -    memset(, 0, sizeof(struct amdgpu_task_info));
+
  adev->job_hang = true;
    if (amdgpu_gpu_recovery &&
@@ -58,12 +58,16 @@ static enum drm_gpu_sched_stat 
amdgpu_job_timedout(struct drm_sched_job *s_job)

  goto exit;
  }
  -    amdgpu_vm_get_task_info(ring->adev, job->pasid, );
  DRM_ERROR("ring %s timeout, signaled seq=%u, emitted seq=%u\n",
-  job->base.sched->name, 
atomic_read(>fence_drv.last_seq),

-  ring->fence_drv.sync_seq);
-    DRM_ERROR("Process information: process %s pid %d thread %s pid 
%d\n",

-  ti.process_name, ti.tgid, ti.task_name, ti.pid);
+  job->base.sched->name, 
atomic_read(>fence_drv.last_seq),

+  ring->fence_drv.sync_seq);


Unnecessary (and incorrect) indentation change.


Ah, my bad, looks like copy-paste screwed-up my editor config for 
alignment.

+
+    ti = amdgpu_vm_get_task_info_pasid(ring->adev, job->pasid);
+    if (ti) {
+    DRM_ERROR("Process information: process %s pid %d thread %s 
pid %d\n",

+  ti->process_name, ti->tgid, ti->task_name, ti->pid);
+    amdgpu_vm_put_task_info_pasid(ring->adev, ti, job->pasid);
+    }
    dma_fence_set_error(_job->s_fence->finished, -ETIME);
  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c

index 4baa300121d8..bfd7a6067edd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
@@ -230,8 +230,16 @@ void amdgpu_coredump(struct amdgpu_device *adev, 
bool vram_lost,

   

Re: [PATCH v2] drm/amdgpu: change vm->task_info handling

2024-01-19 Thread Felix Kuehling 



On 2024-01-18 14:21, Shashank Sharma wrote:

This patch changes the handling and lifecycle of vm->task_info object.
The major changes are:
- vm->task_info is a dynamically allocated ptr now, and its uasge is
   reference counted.
- introducing two new helper funcs for task_info lifecycle management
 - amdgpu_vm_get_task_info: reference counts up task_info before
   returning this info
 - amdgpu_vm_put_task_info: reference counts down task_info
- last put to task_info() frees task_info from the vm.

This patch also does logistical changes required for existing usage
of vm->task_info.

V2: Do not block all the prints when task_info not found (Felix)

Cc: Christian Koenig 
Cc: Alex Deucher 
Cc: Felix Kuehling 
Signed-off-by: Shashank Sharma 


Nit-picks inline.



---
  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |   7 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  18 ++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c   |  12 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 142 +---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  26 +++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   2 +-
  drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  30 +++--
  drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c  |  31 +++--
  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c   |  22 +--
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   |  26 ++--
  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c  |  26 ++--
  drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c|  26 ++--
  drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c |  20 +--
  13 files changed, 287 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 0e61ebdb3f3e..99c736b6e32c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1775,9 +1775,12 @@ static int amdgpu_debugfs_vm_info_show(struct seq_file 
*m, void *unused)
list_for_each_entry(file, >filelist, lhead) {
struct amdgpu_fpriv *fpriv = file->driver_priv;
struct amdgpu_vm *vm = >vm;
+   struct amdgpu_task_info *ti;
+
+   ti = amdgpu_vm_get_task_info_vm(vm);


Can ti be NULL here? I think it can, so you'd need a NULL check to avoid 
a possible kernel oops.




+   seq_printf(m, "pid:%d\tProcess:%s --\n", ti->pid, 
ti->process_name);
+   amdgpu_vm_put_task_info_vm(ti, vm);
  
-		seq_printf(m, "pid:%d\tProcess:%s --\n",

-   vm->task_info.pid, vm->task_info.process_name);
r = amdgpu_bo_reserve(vm->root.bo, true);
if (r)
break;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 1f357198533f..af23746821b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -35,7 +35,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
drm_sched_job *s_job)
  {
struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
struct amdgpu_job *job = to_amdgpu_job(s_job);
-   struct amdgpu_task_info ti;
+   struct amdgpu_task_info *ti;
struct amdgpu_device *adev = ring->adev;
int idx;
int r;
@@ -48,7 +48,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
drm_sched_job *s_job)
return DRM_GPU_SCHED_STAT_ENODEV;
}
  
-	memset(, 0, sizeof(struct amdgpu_task_info));

+
adev->job_hang = true;
  
  	if (amdgpu_gpu_recovery &&

@@ -58,12 +58,16 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
drm_sched_job *s_job)
goto exit;
}
  
-	amdgpu_vm_get_task_info(ring->adev, job->pasid, );

DRM_ERROR("ring %s timeout, signaled seq=%u, emitted seq=%u\n",
- job->base.sched->name, atomic_read(>fence_drv.last_seq),
- ring->fence_drv.sync_seq);
-   DRM_ERROR("Process information: process %s pid %d thread %s pid %d\n",
- ti.process_name, ti.tgid, ti.task_name, ti.pid);
+ job->base.sched->name, 
atomic_read(>fence_drv.last_seq),
+ ring->fence_drv.sync_seq);


Unnecessary (and incorrect) indentation change.



+
+   ti = amdgpu_vm_get_task_info_pasid(ring->adev, job->pasid);
+   if (ti) {
+   DRM_ERROR("Process information: process %s pid %d thread %s pid 
%d\n",
+ ti->process_name, ti->tgid, ti->task_name, ti->pid);
+   amdgpu_vm_put_task_info_pasid(ring->adev, ti, job->pasid);
+   }
  
  	dma_fence_set_error(_job->s_fence->finished, -ETIME);
  
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c

index 4baa300121d8..bfd7a6067edd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
@@ -230,8 +230,16 @@ void amdgpu_coredump(struct

[PATCH v2] drm/amdgpu: change vm->task_info handling

2024-01-18 Thread Shashank Sharma 
This patch changes the handling and lifecycle of vm->task_info object.
The major changes are:
- vm->task_info is a dynamically allocated ptr now, and its uasge is
  reference counted.
- introducing two new helper funcs for task_info lifecycle management
- amdgpu_vm_get_task_info: reference counts up task_info before
  returning this info
- amdgpu_vm_put_task_info: reference counts down task_info
- last put to task_info() frees task_info from the vm.

This patch also does logistical changes required for existing usage
of vm->task_info.

V2: Do not block all the prints when task_info not found (Felix)

Cc: Christian Koenig 
Cc: Alex Deucher 
Cc: Felix Kuehling 
Signed-off-by: Shashank Sharma 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |   7 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  18 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c   |  12 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 142 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  26 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  30 +++--
 drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c  |  31 +++--
 drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c   |  22 +--
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   |  26 ++--
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c  |  26 ++--
 drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c|  26 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c |  20 +--
 13 files changed, 287 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 0e61ebdb3f3e..99c736b6e32c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1775,9 +1775,12 @@ static int amdgpu_debugfs_vm_info_show(struct seq_file 
*m, void *unused)
list_for_each_entry(file, >filelist, lhead) {
struct amdgpu_fpriv *fpriv = file->driver_priv;
struct amdgpu_vm *vm = >vm;
+   struct amdgpu_task_info *ti;
+
+   ti = amdgpu_vm_get_task_info_vm(vm);
+   seq_printf(m, "pid:%d\tProcess:%s --\n", ti->pid, 
ti->process_name);
+   amdgpu_vm_put_task_info_vm(ti, vm);
 
-   seq_printf(m, "pid:%d\tProcess:%s --\n",
-   vm->task_info.pid, vm->task_info.process_name);
r = amdgpu_bo_reserve(vm->root.bo, true);
if (r)
break;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 1f357198533f..af23746821b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -35,7 +35,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
drm_sched_job *s_job)
 {
struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
struct amdgpu_job *job = to_amdgpu_job(s_job);
-   struct amdgpu_task_info ti;
+   struct amdgpu_task_info *ti;
struct amdgpu_device *adev = ring->adev;
int idx;
int r;
@@ -48,7 +48,7 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
drm_sched_job *s_job)
return DRM_GPU_SCHED_STAT_ENODEV;
}
 
-   memset(, 0, sizeof(struct amdgpu_task_info));
+
adev->job_hang = true;
 
if (amdgpu_gpu_recovery &&
@@ -58,12 +58,16 @@ static enum drm_gpu_sched_stat amdgpu_job_timedout(struct 
drm_sched_job *s_job)
goto exit;
}
 
-   amdgpu_vm_get_task_info(ring->adev, job->pasid, );
DRM_ERROR("ring %s timeout, signaled seq=%u, emitted seq=%u\n",
- job->base.sched->name, atomic_read(>fence_drv.last_seq),
- ring->fence_drv.sync_seq);
-   DRM_ERROR("Process information: process %s pid %d thread %s pid %d\n",
- ti.process_name, ti.tgid, ti.task_name, ti.pid);
+ job->base.sched->name, 
atomic_read(>fence_drv.last_seq),
+ ring->fence_drv.sync_seq);
+
+   ti = amdgpu_vm_get_task_info_pasid(ring->adev, job->pasid);
+   if (ti) {
+   DRM_ERROR("Process information: process %s pid %d thread %s pid 
%d\n",
+ ti->process_name, ti->tgid, ti->task_name, ti->pid);
+   amdgpu_vm_put_task_info_pasid(ring->adev, ti, job->pasid);
+   }
 
dma_fence_set_error(_job->s_fence->finished, -ETIME);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
index 4baa300121d8..bfd7a6067edd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
@@ -230,8 +230,16 @@ void amdgpu_coredump(struct amdgpu_device *adev, bool 
vram_lost,
 
coredump->reset_vram_lost = vram_lost;
 
-   if (reset_context->job && reset_context->job->vm)
-   coredump->reset_task_info = reset_context->job->vm->task_info;
+