[PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> Sent: Tuesday, November 12, 2024 5:54
> Am 10.11.24 um 16:41 schrieb Yunxiang Li:
> > @@ -310,6 +311,92 @@ static void amdgpu_vm_bo_reset_state_machine(struct
> amdgpu_vm *vm)
> >     spin_unlock(&vm->status_lock);
> >   }
> >
> > +/**
> > + * amdgpu_vm_update_shared - helper to update shared memory stat
> > + * @base: base structure for tracking BO usage in a VM
> > + * @sign: if we should add (+1) or subtract (-1) from the shared stat
> > + *
> > + * Takes the vm status_lock and updates the shared memory stat. If
> > +the basic
> > + * stat changed (e.g. buffer was moved) amdgpu_vm_update_stats need
> > +to be called
> > + * as well.
> > + */
> > +static void amdgpu_vm_update_shared(struct amdgpu_vm_bo_base *base,
> > +int sign) {
> > +   struct amdgpu_vm *vm = base->vm;
> > +   struct amdgpu_bo *bo = base->bo;
> > +   struct ttm_resource *res;
> > +   int64_t size;
> > +   uint32_t type;
> > +
> > +   if (!vm || !bo)
> > +           return;
> > +
> > +   size = sign * amdgpu_bo_size(bo);
> > +   res = bo->tbo.resource;
> > +   type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);
>
> Again, it's a clear NAK from my side to do stuff like that.
>
> When there isn't any backing store the BO should *not* be accounted to anything.

I don't have a preference either way, but I think it should be a separate discussion to properly define what drm-total- means.

> > +   type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);
> > +   shared = drm_gem_object_is_shared_for_memory_stats(&bo->tbo.base);
> > +
> > +   if (type >= __AMDGPU_PL_LAST)
> > +           return;
> > +
> > +   spin_lock(&vm->status_lock);
> > +
> > +   if (shared)
> > +           vm->stats[type].drm.shared += size;
> > +   else
> > +           vm->stats[type].drm.private += size;
> > +   if (res)
> > +           vm->stats[type].drm.resident += size;
> > +   if (bo->flags & AMDGPU_GEM_CREATE_DISCARDABLE)
> > +           vm->stats[type].drm.purgeable += size;
> > +
> > +   if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) {
> > +           vm->stats[TTM_PL_VRAM].requested += size;
> > +           if (type != TTM_PL_VRAM)
> > +                   vm->stats[TTM_PL_VRAM].evicted += size;
>
> Again that is incorrect. BOs can be created with VRAM|GTT as their placement.
>
> If such a BO is placed into GTT that doesn't mean it is evicted.

In that case, do we count BO with VRAM|GTT in both VRAM and GTT's .requested field? and if they are not in either, they go in both .evicted field?

> > @@ -2612,7 +2707,6 @@ 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);
> >     amdgpu_vm_set_pasid(adev, vm, 0);
> >     dma_fence_wait(vm->last_unlocked, false);
> >     dma_fence_put(vm->last_unlocked);
> > @@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
> struct amdgpu_vm *vm)
> >             }
> >     }
> >
> > +   if (!amdgpu_vm_stats_is_zero(vm)) {
> > +           struct amdgpu_task_info *ti = vm->task_info;
> > +
> > +           dev_warn(adev->dev,
> > +                    "VM memory stats for proc %s(%d) task %s(%d) is non-zero
> when fini\n",
> > +                    ti->process_name, ti->pid, ti->task_name, ti->tgid);
> > +   }
> > +
> > +   amdgpu_vm_put_task_info(vm->task_info);
>
> Please don't move the call to amdgpu_vm_put_task_info().

Is keeping the task_info alive a hazard here? I could copy out the info, it just seemed a bit wasteful.

Regards,
Teddy