Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Uncoordinated double registration of ZONE_DEVICE pages for VRAM causes PFN collisions, breaks KFD SVM migration on XGMI, and wastes system memory. - [High] Ignoring the return value of `amdgpu_svm_migration_init()` masks critical initialization failures and causes a permanent, compounding memory leak of ZONE_DEVICE `struct page` arrays. --
commit d498d3d35f13a60420ca6ba9733ca4560370b681 Author: Junhua Shen <[email protected]> drm/amdgpu: hook up ZONE_DEVICE registration in device init and reset This patch calls amdgpu_svm_migration_init() in the device initialization and XGMI reset-restore paths to register the GPU VRAM as a ZONE_DEVICE region. This activates the drm_pagemap migration infrastructure before KFD initialization. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 6608780ffef2f..60a8ba1b3095b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c [ ... ] > @@ -4102,6 +4103,7 @@ int amdgpu_device_init(struct amdgpu_device *adev, > > /* Don't init kfd if whole hive need to be reset during init */ > if (adev->init_lvl->level != AMDGPU_INIT_LEVEL_MINIMAL_XGMI) { > + amdgpu_svm_migration_init(adev); > kgd2kfd_init_zone_device(adev); [Severity: Critical] Does this sequence result in an uncoordinated double registration of ZONE_DEVICE pages for VRAM? It appears both amdgpu_svm_migration_init() and kgd2kfd_init_zone_device() independently attempt to allocate and register a dev_pagemap. On XGMI coherent systems, if both attempt to map the exact same physical address range (adev->gmc.aper_base), could the second call fail due to conflicting mappings and silently break KFD SVM migration? On non-XGMI configurations, would this successfully allocate two separate, disjoint fake physical memory blocks for the same underlying VRAM, redundantly allocating duplicate vmemmap struct page arrays and wasting system memory? [Severity: High] Should the return value of amdgpu_svm_migration_init() be checked here? Looking at amdgpu_svm_migration_init() in amdgpu_migrate.c, if drm_pagemap_init() fails, it returns -EINVAL but does not unwind the prior devm_memremap_pages() allocations. If the error is ignored here and probe continues successfully, does this permanently leak the struct page arrays? > kfd_update_svm_support_properties(adev); > } [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c > index da77e0ef129ba..c2ea540e392b4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c [ ... ] > @@ -87,6 +88,7 @@ static int amdgpu_reset_xgmi_reset_on_init_restore_hwctxt( > return r; > list_for_each_entry(tmp_adev, reset_device_list, reset_list) { > if (!tmp_adev->kfd.init_complete) { > + amdgpu_svm_migration_init(tmp_adev); > kgd2kfd_init_zone_device(tmp_adev); [Severity: High] Could the missing return value check here cause a compounding memory leak? Because the struct page arrays are not freed on failure, and adev->apagemap is not flagged as initialized, repeated XGMI resets might re-execute this initialization function. Would this allocate new memory regions and struct page arrays each time a reset occurs? > amdgpu_amdkfd_device_init(tmp_adev); > amdgpu_amdkfd_drm_client_create(tmp_adev); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
