[PATCH v3] drm/amdkfd: Use partial migrations in GPU page faults
Chen, Xiaogang
xiaogang.chen at amd.com
Fri Sep 29 19:44:13 UTC 2023
On 9/29/2023 1:41 PM, Felix Kuehling wrote:
>
> On 2023-09-29 14:25, Chen, Xiaogang wrote:
>>
>> On 9/28/2023 4:26 PM, Felix Kuehling wrote:
>>> On 2023-09-20 13:32, Xiaogang.Chen wrote:
>>>> From: Xiaogang Chen <xiaogang.chen at amd.com>
>>>>
>>>> This patch implements partial migration in gpu page fault according
>>>> to migration
>>>> granularity(default 2MB) and not split svm range in cpu page fault
>>>> handling.
>>>> A svm range may include pages from both system ram and vram of one
>>>> gpu now.
>>>> These chagnes are expected to improve migration performance and
>>>> reduce mmu
>>>> callback and TLB flush workloads.
>>>>
>>>> Signed-off-by: xiaogang chen<xiaogang.chen at amd.com>
>>>
>>> Some more nit-picks inline.
> [snip]
>>> @@ -202,12 +200,24 @@ svm_range_dma_map_dev(struct amdgpu_device
>>> *adev, struct svm_range *prange,
>>> static int
>>> svm_range_dma_map(struct svm_range *prange, unsigned long *bitmap,
>>> unsigned long offset, unsigned long npages,
>>> - unsigned long *hmm_pfns)
>>> + unsigned long *hmm_pfns, uint64_t *vram_pages)
>>> {
>>> struct kfd_process *p;
>>> uint32_t gpuidx;
>>> + struct page **page;
>>> int r;
>>> + page = kvcalloc(npages, sizeof(*page), GFP_KERNEL);
>>>
>>> Is there a reason this needs to be 0-initialized? The loop below
>>> initializes all elements.
>> Will use kvmalloc_array instead. This array is not needed to init
>> during alloc as it will be set afterword.
>>>
>>> I'm also not happy about having to allocate this page array here. It
>>> may be justified if the repeated calls to hmm_pfn_to_page were
>>> expensive compared to the memory allocation and initialization. I'm
>>> not convinced that's the case, though. With
>>> CONFIG_SPARSEMEM_VMEMMAP, hmm_pfn_to_page basically boils down to
>>> __pfn_to_page, which is a macro that does just this:
>>>
>>> #define __pfn_to_page(pfn) (vmemmap + (pfn))
>>>
>>> See
>>> https://elixir.bootlin.com/linux/v6.5.5/source/include/asm-generic/memory_model.h#L37.
>>>
>> We get page* array here, then we do not need get page at
>> svm_range_dma_map_dev for each device since pages are same for all
>> gpu devices. So we save calling hmm_pfn_to_page though calling
>> hmm_pfn_to_page cost is low. Here we also want get vram page number
>> during svm_range_dma_map.
>>
>> I think you do not want alloc a page array each time we do
>> svm_range_dma_map? then can we put this struct page **page at
>> svm_range, and alloc it each time create new prange(and clone/split
>> prange)?
>
> Allocating and freeing memory in every dmamap call adds a point of
> failure and non-deterministic overhead for the memory allocation. It's
> non-deterministic because it depends on current memory pressure
> situation and if the array is large, it also needs to update the
> kernel page table and potentially flush CPU TLBs.
>
> Allocating the page pointer array permanently in the prange structure
> would be even worse, because that increases our system memory
> overhead. For each GB of virtual address space we manage, we waste 2MB
> of system memory for the page arrays. To make matters worse, those
> page arrays are allocated in kernel mode where they are not swappable.
> It's like taking physical DIMM sticks out of your system. We worked
> hard to eliminate redundant address and page array allocations from
> the driver a few years ago.
>
> I'd prefer a small amount of redundant address calculations over the
> memory or non-deterministic allocation overhead.
>
Then I think we can cal vram_pages atsvm_range_validate_and_map as the
last version:
+for (int i = 0; i < npages; i++) {
+if (is_zone_device_page(hmm_pfn_to_page(hmm_range->hmm_pfns[i])))
vram_pages++;
+}
That will not alloc page* array, but have additional calling
hmm_pfn_to_page.
Or add a parameter at svm_range_dma_map_dev to indicate whether cal
vram_pages, and only one svm_range_dma_map_dev call will cal vram_pages.
Regards
Xiaogang
> Regards,
> Felix
>
>
>>
>>>
>>>> + if (!page)
>>>> + return -ENOMEM;
>>>> +
>>>> + *vram_pages = 0;
>>>> + for (int i = 0; i < npages; i++) {
>>>> + page[i] = hmm_pfn_to_page(hmm_pfns[i]);
>>>> + if (is_zone_device_page(page[i]))
>>>> + (*vram_pages)++;
>>>> + }
>>>> +
>>>> p = container_of(prange->svms, struct kfd_process, svms);
>>>> for_each_set_bit(gpuidx, bitmap, MAX_GPU_INSTANCE) {
>>>> @@ -221,11 +231,12 @@ svm_range_dma_map(struct svm_range *prange,
>>>> unsigned long *bitmap,
>>>> }
>>>> r = svm_range_dma_map_dev(pdd->dev->adev, prange,
>>>> offset, npages,
>>>> - hmm_pfns, gpuidx);
>>>> + hmm_pfns, gpuidx, page);
>>>> if (r)
>>>> break;
>>>> }
>>>> + kvfree(page);
>>>> return r;
>>>> }
>>>> @@ -347,6 +358,7 @@ svm_range *svm_range_new(struct
>>>> svm_range_list *svms, uint64_t start,
>>>> INIT_LIST_HEAD(&prange->child_list);
>>>> atomic_set(&prange->invalid, 0);
>>>> prange->validate_timestamp = 0;
>>>> + prange->vram_pages = 0;
>>>> mutex_init(&prange->migrate_mutex);
>>>> mutex_init(&prange->lock);
>>>> @@ -393,6 +405,8 @@ static void svm_range_bo_release(struct kref
>>>> *kref)
>>>> prange->start, prange->last);
>>>> mutex_lock(&prange->lock);
>>>> prange->svm_bo = NULL;
>>>> + /* prange does not hold vram page now */
>>>> + prange->actual_loc = 0;
>>>
>>> If you need this here, something probably went wrong elsewhere.
>>> Before we release the BO, we should have migrated everything to
>>> system memory, and actual_loc should already be 0. If anything, I'd
>>> add a WARN_ON(prange->actual_loc) here.
>>>
>> Will replace by WARN_ON as mentioned before.
>>>
>>>> mutex_unlock(&prange->lock);
>>>> spin_lock(&svm_bo->list_lock);
>>>> @@ -966,6 +980,11 @@ svm_range_split_nodes(struct svm_range *new,
>>>> struct svm_range *old,
>>>> new->svm_bo = svm_range_bo_ref(old->svm_bo);
>>>> new->ttm_res = old->ttm_res;
>>>> + /* set new's vram_pages as old range's now, the acurate
>>>> vram_pages
>>>> + * will be updated during mapping
>>>> + */
>>>> + new->vram_pages = min(old->vram_pages, new->npages);
>>>> +
>>>> spin_lock(&new->svm_bo->list_lock);
>>>> list_add(&new->svm_bo_list, &new->svm_bo->range_list);
>>>> spin_unlock(&new->svm_bo->list_lock);
>>>> @@ -1610,6 +1629,7 @@ static int svm_range_validate_and_map(struct
>>>> mm_struct *mm,
>>>> struct svm_validate_context *ctx;
>>>> unsigned long start, end, addr;
>>>> struct kfd_process *p;
>>>> + uint64_t vram_pages;
>>>> void *owner;
>>>> int32_t idx;
>>>> int r = 0;
>>>> @@ -1678,11 +1698,13 @@ static int
>>>> svm_range_validate_and_map(struct mm_struct *mm,
>>>> }
>>>> }
>>>> + vram_pages = 0;
>>>> start = prange->start << PAGE_SHIFT;
>>>> end = (prange->last + 1) << PAGE_SHIFT;
>>>> for (addr = start; addr < end && !r; ) {
>>>> struct hmm_range *hmm_range;
>>>> struct vm_area_struct *vma;
>>>> + uint64_t vram_pages_vma;
>>>> unsigned long next;
>>>> unsigned long offset;
>>>> unsigned long npages;
>>>> @@ -1711,12 +1733,14 @@ static int
>>>> svm_range_validate_and_map(struct mm_struct *mm,
>>>> offset = (addr - start) >> PAGE_SHIFT;
>>>> r = svm_range_dma_map(prange, ctx->bitmap, offset, npages,
>>>> - hmm_range->hmm_pfns);
>>>> + hmm_range->hmm_pfns, &vram_pages_vma);
>>>> if (r) {
>>>> pr_debug("failed %d to dma map range\n", r);
>>>> goto unreserve_out;
>>>> }
>>>> + vram_pages += vram_pages_vma;
>>>> +
>>>> svm_range_lock(prange);
>>>> if (amdgpu_hmm_range_get_pages_done(hmm_range)) {
>>>> pr_debug("hmm update the range, need validate again\n");
>>>> @@ -1738,8 +1762,20 @@ static int svm_range_validate_and_map(struct
>>>> mm_struct *mm,
>>>> addr = next;
>>>> }
>>>> - if (addr == end)
>>>> + if (addr == end) {
>>>> prange->mapped_to_gpu = true;
>>>> + prange->vram_pages = vram_pages;
>>>> +
>>>> + /* if pragne does not include any vram page and it
>>>
>>> Typo: prange
>>>
>> ok.
>>>
>>>> + * has not released svm_bo drop its svm_bo reference
>>>> + */
>>>> + if (!vram_pages && prange->ttm_res) {
>>>> + svm_range_vram_node_free(prange);
>>>> + mutex_lock(&prange->lock);
>>>
>>> Is this lock really needed.
>>>
>> same as before, will remove mutex_lock/unlock(&prange->lock) as it
>> seems overprotected.
>>>
>>>> + prange->actual_loc = 0;
>>>> + mutex_unlock(&prange->lock);
>>>> + }
>>>> + }
>>>> unreserve_out:
>>>> svm_range_unreserve_bos(ctx);
>>>> @@ -1996,6 +2032,7 @@ static struct svm_range
>>>> *svm_range_clone(struct svm_range *old)
>>>> new->actual_loc = old->actual_loc;
>>>> new->granularity = old->granularity;
>>>> new->mapped_to_gpu = old->mapped_to_gpu;
>>>> + new->vram_pages = old->vram_pages;
>>>> bitmap_copy(new->bitmap_access, old->bitmap_access,
>>>> MAX_GPU_INSTANCE);
>>>> bitmap_copy(new->bitmap_aip, old->bitmap_aip, MAX_GPU_INSTANCE);
>>>> @@ -2903,6 +2940,7 @@ svm_range_restore_pages(struct
>>>> amdgpu_device *adev, unsigned int pasid,
>>>> uint32_t vmid, uint32_t node_id,
>>>> uint64_t addr, bool write_fault)
>>>> {
>>>> + unsigned long start, last, size;
>>>> struct mm_struct *mm = NULL;
>>>> struct svm_range_list *svms;
>>>> struct svm_range *prange;
>>>> @@ -3038,32 +3076,38 @@ svm_range_restore_pages(struct
>>>> amdgpu_device *adev, unsigned int pasid,
>>>> kfd_smi_event_page_fault_start(node, p->lead_thread->pid, addr,
>>>> write_fault, timestamp);
>>>> - if (prange->actual_loc != best_loc) {
>>>> + if (prange->actual_loc != 0 || best_loc != 0) {
>>>> migration = true;
>>>> + /* Align migration range start and size to granularity
>>>> size */
>>>> + size = 1UL << prange->granularity;
>>>> + start = ALIGN_DOWN(addr, size);
>>>> + last = ALIGN(addr + 1, size) - 1;
>>>> +
>>>> + start = (start >= prange->start) ? start : prange->start;
>>>> + last = (last <= prange->last) ? last : prange->last;
>>>
>>> Similar to the simplification I recommended in svm_migrate_to_ram,
>>> this could be:
>>>
>>> start = max(ALIGN_DOWN(addr, size), prange->start);
>>> last = min(ALIGN(addr + 1, size) - 1, prange->last);
>>>
>> ok, overlooked last time.
>>
>> Regards
>>
>> Xiaogang
>>
>>> Regards,
>>> Felix
>>>
>>>
>>>> +
>>>> if (best_loc) {
>>>> - r = svm_migrate_to_vram(prange, best_loc, mm,
>>>> - KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU);
>>>> + r = svm_migrate_to_vram(prange, best_loc, start, last,
>>>> + mm, KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU);
>>>> if (r) {
>>>> pr_debug("svm_migrate_to_vram failed (%d) at
>>>> %llx, falling back to system memory\n",
>>>> r, addr);
>>>> /* Fallback to system memory if migration to
>>>> * VRAM failed
>>>> */
>>>> - if (prange->actual_loc)
>>>> - r = svm_migrate_vram_to_ram(prange, mm,
>>>> - KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU,
>>>> - NULL);
>>>> + if (prange->actual_loc && prange->actual_loc !=
>>>> best_loc)
>>>> + r = svm_migrate_vram_to_ram(prange, mm, start,
>>>> last,
>>>> + KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU, NULL);
>>>> else
>>>> r = 0;
>>>> }
>>>> } else {
>>>> - r = svm_migrate_vram_to_ram(prange, mm,
>>>> - KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU,
>>>> - NULL);
>>>> + r = svm_migrate_vram_to_ram(prange, mm, start, last,
>>>> + KFD_MIGRATE_TRIGGER_PAGEFAULT_GPU, NULL);
>>>> }
>>>> if (r) {
>>>> pr_debug("failed %d to migrate svms %p [0x%lx 0x%lx]\n",
>>>> - r, svms, prange->start, prange->last);
>>>> + r, svms, start, last);
>>>> goto out_unlock_range;
>>>> }
>>>> }
>>>> @@ -3417,18 +3461,24 @@ svm_range_trigger_migration(struct
>>>> mm_struct *mm, struct svm_range *prange,
>>>> *migrated = false;
>>>> best_loc = svm_range_best_prefetch_location(prange);
>>>> - if (best_loc == KFD_IOCTL_SVM_LOCATION_UNDEFINED ||
>>>> - best_loc == prange->actual_loc)
>>>> + /* when best_loc is a gpu node and same as prange->actual_loc
>>>> + * we still need do migration as prange->actual_loc !=0 does
>>>> + * not mean all pages in prange are vram. hmm migrate will pick
>>>> + * up right pages during migration.
>>>> + */
>>>> + if ((best_loc == KFD_IOCTL_SVM_LOCATION_UNDEFINED) ||
>>>> + (best_loc == 0 && prange->actual_loc == 0))
>>>> return 0;
>>>> if (!best_loc) {
>>>> - r = svm_migrate_vram_to_ram(prange, mm,
>>>> + r = svm_migrate_vram_to_ram(prange, mm, prange->start,
>>>> prange->last,
>>>> KFD_MIGRATE_TRIGGER_PREFETCH, NULL);
>>>> *migrated = !r;
>>>> return r;
>>>> }
>>>> - r = svm_migrate_to_vram(prange, best_loc, mm,
>>>> KFD_MIGRATE_TRIGGER_PREFETCH);
>>>> + r = svm_migrate_to_vram(prange, best_loc, prange->start,
>>>> prange->last,
>>>> + mm, KFD_MIGRATE_TRIGGER_PREFETCH);
>>>> *migrated = !r;
>>>> return r;
>>>> @@ -3483,7 +3533,11 @@ static void
>>>> svm_range_evict_svm_bo_worker(struct work_struct *work)
>>>> mutex_lock(&prange->migrate_mutex);
>>>> do {
>>>> + /* migrate all vram pages in this prange to sys ram
>>>> + * after that prange->actual_loc should be zero
>>>> + */
>>>> r = svm_migrate_vram_to_ram(prange, mm,
>>>> + prange->start, prange->last,
>>>> KFD_MIGRATE_TRIGGER_TTM_EVICTION, NULL);
>>>> } while (!r && prange->actual_loc && --retries);
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
>>>> index 5fd958a97a28..8574cc5eeabd 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
>>>> @@ -78,6 +78,7 @@ struct svm_work_list_item {
>>>> * @update_list:link list node used to add to update_list
>>>> * @mapping: bo_va mapping structure to create and update GPU
>>>> page table
>>>> * @npages: number of pages
>>>> + * @vram_pages: vram pages number in this svm_range
>>>> * @dma_addr: dma mapping address on each GPU for system memory
>>>> physical page
>>>> * @ttm_res: vram ttm resource map
>>>> * @offset: range start offset within mm_nodes
>>>> @@ -88,7 +89,9 @@ struct svm_work_list_item {
>>>> * @flags: flags defined as KFD_IOCTL_SVM_FLAG_*
>>>> * @perferred_loc: perferred location, 0 for CPU, or GPU id
>>>> * @perfetch_loc: last prefetch location, 0 for CPU, or GPU id
>>>> - * @actual_loc: the actual location, 0 for CPU, or GPU id
>>>> + * @actual_loc: this svm_range location. 0: all pages are from sys
>>>> ram;
>>>> + * GPU id: this svm_range may include vram pages from
>>>> GPU with
>>>> + * id actual_loc.
>>>> * @granularity:migration granularity, log2 num pages
>>>> * @invalid: not 0 means cpu page table is invalidated
>>>> * @validate_timestamp: system timestamp when range is validated
>>>> @@ -112,6 +115,7 @@ struct svm_range {
>>>> struct list_head list;
>>>> struct list_head update_list;
>>>> uint64_t npages;
>>>> + uint64_t vram_pages;
>>>> dma_addr_t *dma_addr[MAX_GPU_INSTANCE];
>>>> struct ttm_resource *ttm_res;
>>>> uint64_t offset;
More information about the amd-gfx
mailing list