Re: [PATCH 2/2] drm/amdkfd: track unified memory reservation with xnack off

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2022-05-19 um 10:01 schrieb philip yang:
> On 2022-05-18 17:40, Felix Kuehling wrote:
> > On 2022-05-18 14:36, philip yang wrote:
> > > On 2022-05-17 19:11, Alex Sierra wrote:
> > > > [WHY]
> > > > Unified memory with xnack off should be tracked, as userptr mappings
> > > > and legacy allocations do. To avoid oversuscribe system memory when
> > > > xnack off.
> > > > [How]
> > > > Exposing functions reserve_mem_limit and unreserve_mem_limit to SVM
> > > > API and call them on every prange creation and free.
> > > >
> > > > Signed-off-by: Alex Sierra<alex.sierra@xxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  4 +++
> > > >   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 24 +++++++------
> > > >   drivers/gpu/drm/amd/amdkfd/kfd_svm.c          | 34 
> > > > ++++++++++++++-----
> > > >   3 files changed, 43 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > > > index f8b9f27adcf5..f55f34af6480 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > > > @@ -301,6 +301,10 @@ bool amdgpu_amdkfd_bo_mapped_to_dev(struct 
> > > > amdgpu_device *adev, struct kgd_mem *
> > > >   void amdgpu_amdkfd_block_mmu_notifications(void *p);
> > > >   int amdgpu_amdkfd_criu_resume(void *p);
> > > >   bool amdgpu_amdkfd_ras_query_utcl2_poison_status(struct 
> > > > amdgpu_device *adev);
> > > > +int amdgpu_amdkfd_reserve_mem_limit(struct amdgpu_device *adev,
> > > > +        uint64_t size, u32 alloc_flag);
> > > > +void amdgpu_amdkfd_unreserve_mem_limit(struct amdgpu_device *adev,
> > > > +        uint64_t size, u32 alloc_flag);
> > > >     #if IS_ENABLED(CONFIG_HSA_AMD)
> > > >   void amdgpu_amdkfd_gpuvm_init_mem_limits(void);
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > > index e985cf9c7ec0..b2236159ff39 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > > @@ -122,7 +122,7 @@ void amdgpu_amdkfd_reserve_system_mem(uint64_t 
> > > > size)
> > > >    *
> > > >    * Return: returns -ENOMEM in case of error, ZERO otherwise
> > > >    */
> > > > -static int amdgpu_amdkfd_reserve_mem_limit(struct amdgpu_device 
> > > > *adev,
> > > > +int amdgpu_amdkfd_reserve_mem_limit(struct amdgpu_device *adev,
> > > >           uint64_t size, u32 alloc_flag)
> > > >   {
> > > >       uint64_t reserved_for_pt =
> > > > @@ -157,8 +157,8 @@ static int 
> > > > amdgpu_amdkfd_reserve_mem_limit(struct amdgpu_device *adev,
> > > >            kfd_mem_limit.max_system_mem_limit && 
> > > > !no_system_mem_limit) ||
> > > >           (kfd_mem_limit.ttm_mem_used + ttm_mem_needed >
> > > >            kfd_mem_limit.max_ttm_mem_limit) ||
> > > > -        (adev->kfd.vram_used + vram_needed >
> > > > -         adev->gmc.real_vram_size - reserved_for_pt)) {
> > > > +        (adev && (adev->kfd.vram_used + vram_needed >
> > > > +         adev->gmc.real_vram_size - reserved_for_pt))) {
> > > >           ret = -ENOMEM;
> > > >           goto release;
> > > >       }
> > > > @@ -166,7 +166,8 @@ static int 
> > > > amdgpu_amdkfd_reserve_mem_limit(struct amdgpu_device *adev,
> > > >       /* Update memory accounting by decreasing available system
> > > >        * memory, TTM memory and GPU memory as computed above
> > > >        */
> > > > -    adev->kfd.vram_used += vram_needed;
> > > > +    if (adev)
> > > > +        adev->kfd.vram_used += vram_needed;
> > > >       kfd_mem_limit.system_mem_used += system_mem_needed;
> > > >       kfd_mem_limit.ttm_mem_used += ttm_mem_needed;
> > > >   @@ -175,7 +176,7 @@ static int 
> > > > amdgpu_amdkfd_reserve_mem_limit(struct amdgpu_device *adev,
> > > >       return ret;
> > > >   }
> > > >   -static void unreserve_mem_limit(struct amdgpu_device *adev,
> > > > +void amdgpu_amdkfd_unreserve_mem_limit(struct amdgpu_device *adev,
> > > >           uint64_t size, u32 alloc_flag)
> > > >   {
> > > >       spin_lock(&kfd_mem_limit.mem_limit_lock);
> > > > @@ -184,7 +185,8 @@ static void unreserve_mem_limit(struct 
> > > > amdgpu_device *adev,
> > > >           kfd_mem_limit.system_mem_used -= size;
> > > >           kfd_mem_limit.ttm_mem_used -= size;
> > > >       } else if (alloc_flag & KFD_IOC_ALLOC_MEM_FLAGS_VRAM) {
> > > > -        adev->kfd.vram_used -= size;
> > > > +        if (adev)
> > > > +            adev->kfd.vram_used -= size;
> > > >       } else if (alloc_flag & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) {
> > > >           kfd_mem_limit.system_mem_used -= size;
> > > >       } else if (!(alloc_flag &
> > > > @@ -193,9 +195,9 @@ static void unreserve_mem_limit(struct 
> > > > amdgpu_device *adev,
> > > >           pr_err("%s: Invalid BO type %#x\n", __func__, alloc_flag);
> > > >           goto release;
> > > >       }
> > > > -
> > > > -    WARN_ONCE(adev->kfd.vram_used < 0,
> > > > -          "KFD VRAM memory accounting unbalanced");
> > > > +    if (adev)
> > > > +        WARN_ONCE(adev->kfd.vram_used < 0,
> > > > +            "KFD VRAM memory accounting unbalanced");
> > > >       WARN_ONCE(kfd_mem_limit.ttm_mem_used < 0,
> > > >             "KFD TTM memory accounting unbalanced");
> > > >       WARN_ONCE(kfd_mem_limit.system_mem_used < 0,
> > > > @@ -211,7 +213,7 @@ void amdgpu_amdkfd_release_notify(struct 
> > > > amdgpu_bo *bo)
> > > >       u32 alloc_flags = bo->kfd_bo->alloc_flags;
> > > >       u64 size = amdgpu_bo_size(bo);
> > > >   -    unreserve_mem_limit(adev, size, alloc_flags);
> > > > +    amdgpu_amdkfd_unreserve_mem_limit(adev, size, alloc_flags);
> > > >         kfree(bo->kfd_bo);
> > > >   }
> > > > @@ -1601,7 +1603,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
> > > >       /* Don't unreserve system mem limit twice */
> > > >       goto err_reserve_limit;
> > > >   err_bo_create:
> > > > -    unreserve_mem_limit(adev, size, flags);
> > > > +    amdgpu_amdkfd_unreserve_mem_limit(adev, size, flags);
> > > >   err_reserve_limit:
> > > >       mutex_destroy(&(*mem)->lock);
> > > >       if (gobj)
> > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
> > > > b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > > > index 5ed8d9b549a4..e7e9b388fea4 100644
> > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > > > @@ -261,11 +261,21 @@ void svm_range_free_dma_mappings(struct 
> > > > svm_range *prange)
> > > >     static void svm_range_free(struct svm_range *prange)
> > > >   {
> > > > +    uint64_t size = (prange->last - prange->start + 1) << PAGE_SHIFT;
> > > > +    struct kfd_process *p;
> > > > +
> > > >       pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx]\n", 
> > > > prange->svms, prange,
> > > >            prange->start, prange->last);
> > > >         svm_range_vram_node_free(prange);
> > > >       svm_range_free_dma_mappings(prange);
> > > > +
> > > > +    p = container_of(prange->svms, struct kfd_process, svms);
> > > > +    if (!p->xnack_enabled) {
> > > > +        pr_debug("unreserve mem limit: %lld\n", size);
> > > > +        amdgpu_amdkfd_unreserve_mem_limit(NULL, size,
> > > > +                    KFD_IOC_ALLOC_MEM_FLAGS_USERPTR);
> > > > +    }
> > > >       mutex_destroy(&prange->lock);
> > > >       mutex_destroy(&prange->migrate_mutex);
> > > >       kfree(prange);
> > > > @@ -284,7 +294,7 @@ svm_range_set_default_attributes(int32_t 
> > > > *location, int32_t *prefetch_loc,
> > > >     static struct
> > > >   svm_range *svm_range_new(struct svm_range_list *svms, uint64_t 
> > > > start,
> > > > -             uint64_t last)
> > > > +             uint64_t last, bool is_new_alloc)
> > > >   {
> > > >       uint64_t size = last - start + 1;
> > > >       struct svm_range *prange;
> > > > @@ -293,6 +303,15 @@ svm_range *svm_range_new(struct svm_range_list 
> > > > *svms, uint64_t start,
> > > >       prange = kzalloc(sizeof(*prange), GFP_KERNEL);
> > > >       if (!prange)
> > > >           return NULL;
> > > > +
> > > > +    p = container_of(svms, struct kfd_process, svms);
> > > > +    if (!p->xnack_enabled && is_new_alloc &&
> > > > +        amdgpu_amdkfd_reserve_mem_limit(NULL, size << PAGE_SHIFT,
> > > > +                        KFD_IOC_ALLOC_MEM_FLAGS_USERPTR)) {
> > > The range will create svm_bo to migrate to VRAM, so count acc_size 
> > > is correct.
> >
> > I'm not sure how to understand this comment. The thing is, the VRAM 
> > BO can be evicted without losing functionality. So I don't think we 
> > need to track potential VRAM usage of SVM ranges. We only need to 
> > account for system memory usage. That includes the system memory 
> > being mapped, and maybe the prange structures and dma_addr arrays 
> > used for the mapping. However, like I said, the old acc_size isn't 
> > really suitable for that because it doesn't account for mappings on 
> > multiple GPUs.
> >
> > The dma address arrays take 1/512 of the memory size, per GPU. Even 
> > on an 8GPU system, that's only 1/64 of the memory size. So I think 
> > the 15/16 system memory limit still leaves enough room for those data 
> > structures. If that gets too tight, we may just decide to use a lower 
> > system memory limit, or change the system memory limit based on the 
> > number of GPUs in the system. That would be easier than accurately 
> > tracking the data structure sizes for each allocation and potentially 
> > each mapping on a multi-GPU system.
>
> Yes, based on this calculation and acc_size is not accurate now for 
> mGPUs with IOMMU support and svm range, we can remove acc_size for 
> svm_bo, only count svm range as userptr system memory.
>
> Regards,
>
> Philip
>
> > Regards,
> >   Felix
> >
> >
> > > > +        pr_err("Failed to allocate usrptr memory\n");

I just noticed this error message. It's misleading. The failure here is 
not an allocation failure. Also the term "userptr" is misleading because 
it points to the legacy userptr BO mechanism. I'm not sure we should 
print an error message here at all. Every time we print kernel error 
messages because of user mode problems it confuses people.

Anyway, if we have to have a message, it should be something like "SVM 
mapping failed, exceeds resident system memory limit". I would at least 
downgrade it to a warning or info.

Regards,
  Felix


> > > > +        kfree(prange);
> > > > +        return NULL;
> > > > +    }
> > > >       prange->npages = size;
> > > >       prange->svms = svms;
> > > >       prange->start = start;
> > > > @@ -307,7 +326,6 @@ svm_range *svm_range_new(struct svm_range_list 
> > > > *svms, uint64_t start,
> > > >       mutex_init(&prange->migrate_mutex);
> > > >       mutex_init(&prange->lock);
> > > >   -    p = container_of(svms, struct kfd_process, svms);
> > > >       if (p->xnack_enabled)
> > > >           bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
> > > >                   MAX_GPU_INSTANCE);
> > > > @@ -1000,9 +1018,9 @@ svm_range_split(struct svm_range *prange, 
> > > > uint64_t start, uint64_t last,
> > > >         svms = prange->svms;
> > > >       if (old_start == start)
> > > > -        *new = svm_range_new(svms, last + 1, old_last);
> > > > +        *new = svm_range_new(svms, last + 1, old_last, false);
> > > >       else
> > > > -        *new = svm_range_new(svms, old_start, start - 1);
> > > > +        *new = svm_range_new(svms, old_start, start - 1, false);
> > > >       if (!*new)
> > > >           return -ENOMEM;
> > > >   @@ -1825,7 +1843,7 @@ static struct svm_range 
> > > > *svm_range_clone(struct svm_range *old)
> > > >   {
> > > >       struct svm_range *new;
> > > >   -    new = svm_range_new(old->svms, old->start, old->last);
> > > > +    new = svm_range_new(old->svms, old->start, old->last, true);
> > >
> > > clone prange is not new memory allocation, use false.
> > >
> > > Regards,
> > >
> > > Philip
> > >
> > > >       if (!new)
> > > >           return NULL;
> > > >   @@ -1951,7 +1969,7 @@ svm_range_add(struct kfd_process *p, 
> > > > uint64_t start, uint64_t size,
> > > >             /* insert a new node if needed */
> > > >           if (node->start > start) {
> > > > -            prange = svm_range_new(svms, start, node->start - 1);
> > > > +            prange = svm_range_new(svms, start, node->start - 1, 
> > > > true);
> > > >               if (!prange) {
> > > >                   r = -ENOMEM;
> > > >                   goto out;
> > > > @@ -1967,7 +1985,7 @@ svm_range_add(struct kfd_process *p, uint64_t 
> > > > start, uint64_t size,
> > > >         /* add a final range at the end if needed */
> > > >       if (start <= last) {
> > > > -        prange = svm_range_new(svms, start, last);
> > > > +        prange = svm_range_new(svms, start, last, true);
> > > >           if (!prange) {
> > > >               r = -ENOMEM;
> > > >               goto out;
> > > > @@ -2585,7 +2603,7 @@ svm_range 
> > > > *svm_range_create_unregistered_range(struct amdgpu_device *adev,
> > > >           last = addr;
> > > >       }
> > > >   -    prange = svm_range_new(&p->svms, start, last);
> > > > +    prange = svm_range_new(&p->svms, start, last, true);
> > > >       if (!prange) {
> > > >           pr_debug("Failed to create prange in address [0x%llx]\n", 
> > > > addr);
> > > >           return NULL;