Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Evicted GPU buffers bypass memory cgroup limits because the memcg accounting flag is cleared during VRAM eviction and pages are never recharged upon re-validation. - [Low] Missing dummy implementations for GPU page memory control functions when CONFIG_MEMCG is disabled causes a linker error. --
commit 14a7ef9775310750fa8f3933b124ce94b537e5c7 Author: Dave Airlie <[email protected]> amdgpu: add support for memory cgroups This adds support for adding a obj cgroup to a buffer object, and passing in the placement flags to make sure it's accounted properly. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 8062b3d611574..1c8123c564ac1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -155,11 +155,14 @@ static void amdgpu_evict_flags(struct ttm_buffer_object > *bo, > amdgpu_bo_placement_from_domain(abo, > AMDGPU_GEM_DOMAIN_GTT | > AMDGPU_GEM_DOMAIN_CPU); > } > + for (int i = 0; i < abo->placement.num_placement; i++) > + abo->placements[i].flags &= ~TTM_PL_FLAG_MEMCG; [Severity: High] Does clearing TTM_PL_FLAG_MEMCG during eviction allow memory cgroup limits to be bypassed? When a user forces eviction (for example, by allocating more VRAM), the flag is cleared in amdgpu_evict_flags() and TTM moves the buffer to system memory. The pages are then allocated via ttm_pool_alloc() with memcg_account set to false, leaving them uncharged. If the user later re-validates the buffer into GTT, ttm_bo_validate() calls ttm_resource_compatible(). This function only checks the memory type and contiguous flags, ignoring TTM_PL_FLAG_MEMCG. This seems to allow the buffer to remain backed by uncharged system pages, potentially letting a containerized workload exhaust host system memory. > break; > case TTM_PL_TT: > case AMDGPU_PL_PREEMPT: > default: > amdgpu_bo_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_CPU); > + abo->placements[0].flags &= ~TTM_PL_FLAG_MEMCG; [Severity: High] Does this also allow bypassing memory cgroup limits for TTM_PL_TT evictions to the CPU domain for the same reason? [ ... ] > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 4c682b91cbbee..625de4a287eb3 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3032,6 +3032,7 @@ __always_inline struct obj_cgroup > *current_obj_cgroup(void) > > return rcu_dereference_check(root_mem_cgroup->nodeinfo[nid]->objcg, 1); > } > +EXPORT_SYMBOL_GPL(current_obj_cgroup); > > struct obj_cgroup *get_obj_cgroup_from_folio(struct folio *folio) > { [Severity: Low] Will this series cause a linker error when CONFIG_DRM_TTM=y and CONFIG_MEMCG=n? It looks like mem_cgroup_charge_gpu_page(), mem_cgroup_uncharge_gpu_page(), and mem_cgroup_move_gpu_page_reclaim() are declared unconditionally in include/linux/memcontrol.h. However, their definitions in mm/memcontrol.c are skipped when CONFIG_MEMCG is disabled. Since ttm_pool_alloc_page() in drivers/gpu/drm/ttm/ttm_pool.c calls these without ifdef guards, this seems to lead to undefined references during linking. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
