Re: [PATCH 01/10] memcg: add support for GPU page counters. (v4)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mem_cgroup_move_gpu_page_reclaim silently fails to move pages to or from the root cgroup, bypassing memory limits and leaking charges.
- [High] Tagging GPU pages with MEMCG_DATA_KMEM without corresponding kmem counter charges causes global kmem underflows on standard page frees.
- [Medium] Cross-cgroup page moves in mem_cgroup_move_gpu_page_reclaim completely ignore the `to_reclaim` parameter and corrupt stats.
- [Low] Missing static inline stubs for GPU memory tracking APIs when CONFIG_MEMCG is disabled.
--

commit 497e4e8b1bbad8552dbc2f2daeb121cd40f196af
Author: Dave Airlie <airlied@redhat.com>

memcg: add support for GPU page counters. (v4)

This introduces new statistics and memcontrol APIs for dealing with GPU system
memory allocations. It allows charging, uncharging, and moving pages between
object cgroups and reclaim states.

> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index e1f46a0016fcf..3a968c6be0c84 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1583,6 +1583,17 @@ static inline void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
>  #endif	/* CONFIG_CGROUP_WRITEBACK */
>  
>  struct sock;
> +bool mem_cgroup_charge_gpu_page(struct obj_cgroup *objcg, struct page *page,
> +			   unsigned int order,
> +			   gfp_t gfp_mask, bool reclaim);
> +void mem_cgroup_uncharge_gpu_page(struct page *page,
> +				  unsigned int order,
> +				  bool reclaim);
> +bool mem_cgroup_move_gpu_page_reclaim(struct obj_cgroup *objcg,
> +				      struct page *page,
> +				      unsigned int order,
> +				      bool to_reclaim);
> +

[Severity: Low]
Are there missing static inline stubs for these APIs when CONFIG_MEMCG is
disabled? Will this cause undefined reference link errors for drivers using
these functions on non-memcg builds, since they are declared globally but
only implemented in mm/memcontrol.c?

>  #ifdef CONFIG_MEMCG
>  extern struct static_key_false memcg_sockets_enabled_key;
>  #define mem_cgroup_sockets_enabled static_branch_unlikely(&memcg_sockets_enabled_key)

[ ... ]

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6dc4888a90f3f..4c682b91cbbee 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c

[ ... ]

> @@ -5508,6 +5512,106 @@ void mem_cgroup_flush_workqueue(void)
>  	flush_workqueue(memcg_wq);
>  }
>  
> +/**
> + * mem_cgroup_charge_gpu_page - charge a page to GPU memory tracking
> + * @objcg: objcg to charge, NULL charges root memcg

[ ... ]

> +bool mem_cgroup_charge_gpu_page(struct obj_cgroup *objcg, struct page *page,
> +				unsigned int order, gfp_t gfp_mask, bool reclaim)
> +{
> +	unsigned int nr_pages = 1 << order;
> +	struct mem_cgroup *memcg = NULL;
> +	struct lruvec *lruvec;
> +	int ret;
> +
> +	if (objcg) {
> +		memcg = get_mem_cgroup_from_objcg(objcg);
> +
> +		ret = try_charge_memcg(memcg, gfp_mask, nr_pages);
> +		if (ret) {
> +			mem_cgroup_put(memcg);
> +			return false;
> +		}
> +
> +		obj_cgroup_get(objcg);
> +		page_set_objcg(page, objcg);

[Severity: High]
Since page_set_objcg() embeds the MEMCG_DATA_KMEM flag into the page, if this
GPU page is later freed directly by the kernel rather than through
mem_cgroup_uncharge_gpu_page(), will the VM route the uncharge to
__memcg_kmem_uncharge_page()?

Could this improperly decrement the global kmem counters (which were never
incremented during the GPU page charging) and bypass the decrement of the
NR_GPU_ACTIVE or NR_GPU_RECLAIM stats?

> +	}
> +
> +	lruvec = mem_cgroup_lruvec(memcg, page_pgdat(page));
> +	mod_lruvec_state(lruvec, reclaim ? NR_GPU_RECLAIM : NR_GPU_ACTIVE, nr_pages);
> +
> +	mem_cgroup_put(memcg);
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(mem_cgroup_charge_gpu_page);

[ ... ]

> +/**
> + * mem_cgroup_move_gpu_reclaim - move pages from gpu to gpu reclaim and back
> + * @new_objcg: objcg to move page to, NULL if just stats update.
> + * @nr_pages: number of pages to move
> + * @to_reclaim: true moves pages into reclaim, false moves them back
> + */
> +bool mem_cgroup_move_gpu_page_reclaim(struct obj_cgroup *new_objcg,
> +				      struct page *page,
> +				      unsigned int order,
> +				      bool to_reclaim)
> +{
> +	struct obj_cgroup *objcg = page_objcg(page);
> +
> +	if (!objcg || !new_objcg || objcg == new_objcg) {

[Severity: High]
Does this conditional unintentionally merge cross-cgroup moves involving the
root cgroup (where objcg or new_objcg is NULL) into the fast-path for
in-place state transitions?

If a page is moved to or from the root cgroup, it looks like this will
silently fail to transfer ownership to the new child cgroup, bypassing memory
limits or leaving charges stuck in the original cgroup indefinitely.

> +		struct mem_cgroup *memcg = objcg ? get_mem_cgroup_from_objcg(objcg) : NULL;
> +		struct lruvec *lruvec;
> +		unsigned long flags;
> +		int nr_pages = 1 << order;
> +
> +		lruvec = mem_cgroup_lruvec(memcg, page_pgdat(page));
> +		local_irq_save(flags);
> +		mod_lruvec_state(lruvec, to_reclaim ? NR_GPU_RECLAIM : NR_GPU_ACTIVE, nr_pages);
> +		mod_lruvec_state(lruvec, to_reclaim ? NR_GPU_ACTIVE : NR_GPU_RECLAIM, -nr_pages);
> +		local_irq_restore(flags);
> +		mem_cgroup_put(memcg);
> +		return true;
> +	} else {
> +		mem_cgroup_uncharge_gpu_page(page, order, true);
> +		return mem_cgroup_charge_gpu_page(new_objcg, page, order, 0, false);

[Severity: Medium]
Does this block ignore the to_reclaim boolean parameter by hardcoding true
for the uncharge and false for the charge?

It looks like this will corrupt the stats by always decrementing the RECLAIM
counter of the old cgroup and always incrementing the ACTIVE counter of the
new cgroup, regardless of the caller's requested state.

> +	}
> +}
> +EXPORT_SYMBOL_GPL(mem_cgroup_move_gpu_page_reclaim);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706024122.853329-1-airlied@gmail.com?part=1