From ee08f537a7552e92d92da55e5e528af0f157050c Mon Sep 17 00:00:00 2001 From: Harish Kasiviswanathan Date: Mon, 14 Sep 2015 12:18:48 -0400 Subject: [PATCH] Topology, memory allocation, cleanup issue for gGPU Patch submitted by Besar Wicaksono 1. Bug on detecting local memory size interpreted as 32 bit value instead of 64. The bug causes thunk to go into an infinite loop trying to reserve virtual address range for dgpu system memory. 2. SIMD count in the node property is 0. Runtime use this attribute to find a gpu device. Regarding other attributes of intel+tonga topology, Harish started a discussion on August iirc, could you please share an update ? This would help me progress with more tests such as scratch memory, which require the scratch aperture information in order to construct a buffer srd in gpuvm space. 3. Bug on releasing memory via fmm_release, where no actual release is being done. The vm_object can't be found because the memory size does not match due to the allocation padded the size with 32KB. 4. Pointer arithmetic on vm_area allocation/release. The value of vm_area_t::end seems to be interpreted inconsistently whether it is (start + size -1) or (start + size). One example of potential issue I see is the logic could report larger size of the hole in the vm area list. 5. Resource cleanup on multiple library load/unload within a single process. - Any memory allocation on subsequent library load will result an error "va above limit". To my understanding this is due to the reserved memory for the system memory not being released on unload. - The static variable events_page needs to be invalidated appropriately on library unload so the next load could reinitialize it. 6. Could you please update if AQL queue is ready to test with the stg kfd/kmt ? 7. The system memory allocation with size larger than 32KB seems to be padded by an extra 32KB. I was wondering if we could remove this overhead. Change-Id: I039988d36637525089c7569dc3b77e58750e2121 --- src/events.c | 7 +++++++ src/fmm.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---- src/fmm.h | 2 ++ src/openclose.c | 4 ++++ src/topology.c | 10 ++++++--- 5 files changed, 71 insertions(+), 7 deletions(-) diff --git a/src/events.c b/src/events.c index 7cedacb459..747534fa78 100644 --- a/src/events.c +++ b/src/events.c @@ -42,6 +42,13 @@ static bool IsSystemEventType(HSA_EVENTTYPE type) return (type != HSA_EVENTTYPE_SIGNAL && type != HSA_EVENTTYPE_DEBUG_EVENT); } +void CleanupEvent(void) +{ + // Release is handled during aperture cleanup. + free_exec_aligned_memory_gpu(events_page, KFD_SIGNAL_EVENT_LIMIT * 8); + events_page = NULL; +} + HSAKMT_STATUS HSAKMTAPI hsaKmtCreateEvent( diff --git a/src/fmm.c b/src/fmm.c index df864779ea..cfd24649e4 100644 --- a/src/fmm.c +++ b/src/fmm.c @@ -98,6 +98,7 @@ typedef struct { static gpu_mem_t gpu_mem[] = INIT_GPUs_MEM; static HSAKMT_STATUS dgpu_mem_init(uint8_t node_id, void **base, void **limit); +static HSAKMT_STATUS dgpu_mem_release(void); static int set_dgpu_aperture(uint32_t node_id, uint64_t base, uint64_t limit); static void __fmm_release(uint32_t gpu_id, void *address, uint64_t MemorySizeInBytes, manageble_aperture_t *aperture); @@ -309,7 +310,7 @@ static void *aperture_allocate_area(manageble_aperture_t *app, break; /* address space "hole" */ - if ((VOID_PTRS_SUB(next->start, cur->end) >= + if (((VOID_PTRS_SUB(next->start, cur->end) - 1) >= MemorySizeInBytes)) break; @@ -317,7 +318,7 @@ static void *aperture_allocate_area(manageble_aperture_t *app, }; /* If the new range is inside the reserved aperture */ - if (VOID_PTRS_SUB(app->limit, cur->end) + 1 >= + if (VOID_PTRS_SUB(app->limit, cur->end) >= MemorySizeInBytes) { /* * cur points to the last inspected element: the tail @@ -571,6 +572,7 @@ static void* __fmm_allocate_device(uint32_t gpu_id, uint64_t MemorySizeInBytes, #define GPUVM_APP_OFFSET 0x10000 void *fmm_allocate_device(uint32_t gpu_id, uint64_t MemorySizeInBytes) { + MemorySizeInBytes += 0x8000 - (MemorySizeInBytes % 0x8000); manageble_aperture_t *aperture; int32_t gpu_mem_id; @@ -616,6 +618,7 @@ static void* fmm_allocate_host_gpu(uint32_t gpu_id, manageble_aperture_t *aperture; int32_t gpu_mem_id; uint64_t mmap_offset; + static size_t total_mem = 0; /* Retrieve gpu_mem id according to gpu_id */ gpu_mem_id = gpu_mem_find_by_gpu_id(gpu_id); @@ -637,6 +640,9 @@ static void* fmm_allocate_host_gpu(uint32_t gpu_id, return NULL; } + total_mem += MemorySizeInBytes; + printf("total_mem: %lu\n", total_mem); + return ret; } @@ -756,6 +762,7 @@ void fmm_release(void *address, uint64_t MemorySizeInBytes) if (address >= gpu_mem[i].dgpu_aperture.base && address <= gpu_mem[i].dgpu_aperture.limit) { found = true; + MemorySizeInBytes += 0x8000 - (MemorySizeInBytes % 0x8000); __fmm_release(gpu_mem[i].gpu_id, address, MemorySizeInBytes, &gpu_mem[i].dgpu_aperture); fmm_print(gpu_mem[i].gpu_id); @@ -817,6 +824,32 @@ HSAKMT_STATUS fmm_init_process_apertures(void) return HSAKMT_STATUS_SUCCESS; } +HSAKMT_STATUS fmm_cleanup_process_apertures(void) +{ + uint32_t i; + struct kfd_ioctl_unmap_memory_from_gpu_args unmap_args; + struct kfd_ioctl_free_memory_of_gpu_args free_args; + + for (i = 0 ; i < NUM_OF_SUPPORTED_GPUS; i++) { + vm_object_t *cur = gpu_mem[i].dgpu_aperture.vm_objects; + pthread_mutex_lock(&gpu_mem[i].dgpu_aperture.fmm_mutex); + while (cur) { + unmap_args.handle = cur->handle; + kmtIoctl(kfd_fd, AMDKFD_IOC_UNMAP_MEMORY_FROM_GPU, &unmap_args); + free_args.handle = cur->handle; + kmtIoctl(kfd_fd, AMDKFD_IOC_FREE_MEMORY_OF_GPU, &free_args); + aperture_release_area(&gpu_mem[i].dgpu_aperture, cur->start, cur->size); + vm_object_t* temp = cur; + cur = temp->next; + vm_remove_object(&gpu_mem[i].dgpu_aperture, temp); + }; + pthread_mutex_unlock(&gpu_mem[i].dgpu_aperture.fmm_mutex); + } + + dgpu_mem_release(); + return HSAKMT_STATUS_SUCCESS; +} + HSAuint64 fmm_get_aperture_limit(aperture_type_e aperture_type, HSAuint32 gpu_id) { int32_t slot = gpu_mem_find_by_gpu_id(gpu_id); @@ -1060,7 +1093,7 @@ static HSAKMT_STATUS dgpu_mem_init(uint8_t node_id, void **base, void **limit) bool found; HSAKMT_STATUS ret; void *addr, *ret_addr; - uint32_t max_len; + HSAuint64 max_len; long long unsigned int temp; uint32_t gpu_id; HsaNodeProperties props; @@ -1077,7 +1110,7 @@ static HSAKMT_STATUS dgpu_mem_init(uint8_t node_id, void **base, void **limit) if (ret != HSAKMT_STATUS_SUCCESS) return ret; - max_len = (uint32_t)props.LocalMemSize; + max_len = props.LocalMemSize; found = false; for (addr = (void *)PAGE_SIZE, ret_addr = NULL; @@ -1110,6 +1143,20 @@ static HSAKMT_STATUS dgpu_mem_init(uint8_t node_id, void **base, void **limit) return HSAKMT_STATUS_ERROR; } +static HSAKMT_STATUS dgpu_mem_release(void) +{ + size_t max_len; + if (is_dgpu_mem_init) { + max_len = (size_t)dgpu_shared_aperture_limit - + (size_t)dgpu_shared_aperture_base; + munmap(dgpu_shared_aperture_base, max_len); + is_dgpu_mem_init = false; + dgpu_shared_aperture_base = NULL; + dgpu_shared_aperture_limit = NULL; + } + return HSAKMT_STATUS_SUCCESS; +} + bool fmm_get_handle(void *address, uint64_t *handle) { int32_t i; diff --git a/src/fmm.h b/src/fmm.h index c2ff77e074..30af235170 100644 --- a/src/fmm.h +++ b/src/fmm.h @@ -43,7 +43,9 @@ typedef struct { void* start_address; } aperture_properties_t; +void CleanupEvent(void); HSAKMT_STATUS fmm_init_process_apertures(void); +HSAKMT_STATUS fmm_cleanup_process_apertures(void); /* * Memory interface */ diff --git a/src/openclose.c b/src/openclose.c index d5b91e23dd..47aef0df7a 100644 --- a/src/openclose.c +++ b/src/openclose.c @@ -30,6 +30,7 @@ #include #include #include +#include #include "fmm.h" static const char kfd_device_name[] = "/dev/kfd"; @@ -90,6 +91,9 @@ hsaKmtCloseKFD(void) { if (--kfd_open_count == 0) { + printf("cleanup aperture\n"); + CleanupEvent(); + fmm_cleanup_process_apertures(); close(kfd_fd); if (amd_hsa_thunk_lock_fd > 0) { diff --git a/src/topology.c b/src/topology.c index 608efa2d71..f367428dd0 100644 --- a/src/topology.c +++ b/src/topology.c @@ -319,8 +319,12 @@ topology_sysfs_get_node_props(uint32_t node_id, HsaNodeProperties *props, uint32 while(sscanf(p+=prog, "%s %llu\n%n", prop_name, &prop_val, &prog) == 2) { if (strcmp(prop_name,"cpu_cores_count") == 0) props->NumCPUCores = (uint32_t)prop_val; - else if (strcmp(prop_name,"simd_count") == 0) - props->NumFComputeCores = (uint32_t)prop_val; + else if (strcmp(prop_name,"simd_count") == 0) { + if ((uint32_t)prop_val == 0) + props->NumFComputeCores = (uint32_t)1; + else + props->NumFComputeCores = (uint32_t)prop_val; + } else if (strcmp(prop_name,"mem_banks_count") == 0) props->NumMemoryBanks = (uint32_t)prop_val; else if (strcmp(prop_name,"caches_count") == 0) @@ -364,7 +368,7 @@ topology_sysfs_get_node_props(uint32_t node_id, HsaNodeProperties *props, uint32 else if (strcmp(prop_name,"max_engine_clk_ccompute") == 0) props->MaxEngineClockMhzCCompute = (uint32_t)prop_val; else if (strcmp(prop_name,"local_mem_size") == 0) - props->LocalMemSize = (uint32_t)prop_val; + props->LocalMemSize = (HSAuint64)prop_val; }