Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The Kconfig option `DRM_TTM_MEMCG` lacks a `default y` directive, which silently disables TTM memcg accounting by default, contradicting the intended behavior described in the commit message. Pre-existing issues: - [High] Explicitly zeroing `page->memcg_data` prior to uncharging skips the memcg uncharge process, causing persistent `obj_cgroup` reference leaks and memcg counter leaks. - [High] DMA-allocated pool pages bypass accounting but are unconditionally moved between active/reclaim states, permanently underflowing `NR_GPU_ACTIVE` and leaking `NR_GPU_RECLAIM` stats. -- commit 29deabc39c8a2c600d3de5d3662fd9003ff7aa7a Author: Dave Airlie <[email protected]> ttm: add support for a module option to disable memcg integration This adds a kconfig and a module option to turn off ttm memcg integration completely. > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -257,6 +257,13 @@ config DRM_TTM_HELPER > help > Helpers for ttm-based gem objects > > +config DRM_TTM_MEMCG > + bool "Enable TTM mem cgroup by default" > + depends on DRM_TTM > + depends on MEMCG [Severity: Medium] Does this Kconfig option need a "default y" directive? The help text and commit message state that the feature is enabled by default, but without it, the configuration will default to disabled in standard builds. > + help > + Enable the memcg integration by default > + [ ... ] > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -321,7 +339,7 @@ static void ttm_pool_type_give(struct ttm_pool_type *pt, > struct page *p) > > INIT_LIST_HEAD(&p->lru); > rcu_read_lock(); > - list_lru_add(&pt->pages, &p->lru, nid, page_memcg_check(p)); > + list_lru_add(&pt->pages, &p->lru, nid, ttm_memcg ? page_memcg_check(p) > : NULL); > rcu_read_unlock(); > > atomic_long_add(num_pages, &allocated_pages[nid]); [Severity: High] This isn't a bug introduced by this patch, but does the call to mem_cgroup_move_gpu_page_reclaim() that immediately follows this code corrupt the memcg stats for DMA pages? When ttm_pool_uses_dma_alloc() is true, ttm_pool_alloc_page() bypasses mem_cgroup_charge_gpu_page(), meaning NR_GPU_ACTIVE is never incremented. However, when the page is added to the pool here in ttm_pool_type_give(), it unconditionally calls mem_cgroup_move_gpu_page_reclaim(..., true), which subtracts from NR_GPU_ACTIVE and adds to NR_GPU_RECLAIM. Finally, when the shrinker frees the page, ttm_pool_free_page() bypasses __free_pages_gpu_account(), so the RECLAIM stat is never decremented. Does this permanently underflow NR_GPU_ACTIVE and leak NR_GPU_RECLAIM? [ ... ] > @@ -844,7 +862,7 @@ static int __ttm_pool_alloc(struct ttm_pool *pool, struct > ttm_tt *tt, > bool allow_pools; > struct page *p; > int r; > - struct obj_cgroup *objcg = memcg_account ? tt->objcg : NULL; > + struct obj_cgroup *objcg = (ttm_memcg && memcg_account) ? tt->objcg : > NULL; > > WARN_ON(!alloc->remaining_pages || ttm_tt_is_populated(tt)); > WARN_ON(alloc->dma_addr && !pool->dev); [Severity: High] This is a pre-existing issue, but does explicitly zeroing page->memcg_data prior to uncharging cause an obj_cgroup leak? During backup restoration in ttm_pool_restore_commit(): copy_highpage(restore->alloced_page + i, p); p->memcg_data = 0; __free_pages_gpu_account(p, 0, false); And similarly during purging in ttm_pool_shrink(): page->private = 0; page->memcg_data = 0; __free_pages_gpu_account(page, order, false); Since __free_pages_gpu_account() relies on mem_cgroup_uncharge_gpu_page() which requires memcg_data to locate the correct obj_cgroup, doesn't clearing it beforehand cause the uncharge to silently fail, leaking the stats and the reference? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
