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 <[email protected]> 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/[email protected]?part=1
