Re: [PATCH 09/10] ttm: add support for a module option to disable memcg integration

[sashiko-bot@kernel.org]

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 <airlied@redhat.com>

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/20260706024122.853329-1-airlied@gmail.com?part=9