From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khorenko Date: Fri, 21 Aug 2026 16:03:29 +0200 Subject: Re: [Devel] [PATCH vz10 02/14] mm: fix VZ build errors with CONFIG_MEMCG=n In-Reply-To: <20260625220832.2201873-2-eva.kurchatova@virtuozzo.com> References: <20260625220832.2201873-1-eva.kurchatova@virtuozzo.com> <20260625220832.2201873-2-eva.kurchatova@virtuozzo.com> Message-ID: <16955f7d-3e39-4a99-845d-4f64f1e6a8d4@virtuozzo.com> List-Id: On 6/26/26 00:08, Eva Kurchatova wrote: > Guard VZ-specific code that uses memory cgroup internals > (memory_cgrp_id, struct mem_cgroup, root_mem_cgroup) with > CONFIG_MEMCG. > > Without CONFIG_MEMCG: > - oom_berserker() and oom_kill_memcg_member() dereference struct > mem_cgroup fields that do not exist. > - si_meminfo_ve(), fill_meminfo_ve(), and fill_vmstat_ve() use > memory_cgrp_id and mem_cgroup_from_css() which are undeclared. > - proc_oom_score() VE-specific path references memory_cgrp_id. > > Wrap affected functions and their call sites with CONFIG_MEMCG (or > CONFIG_VE && CONFIG_MEMCG where appropriate) and provide stubs where > needed. Also guard VZ-specific nid field accesses in memcontrol.c > with CONFIG_MEMCG_V1 and move the numa_migrate cftypes entry inside > the existing CONFIG_NUMA block. > > Signed-off-by: Eva Kurchatova > > https://virtuozzo.atlassian.net/browse/VSTOR-134732 > Feature: fix kunit > --- > fs/proc/base.c | 4 ++++ > fs/proc/meminfo.c | 4 ++++ > mm/memcontrol.c | 9 +++++++-- > mm/oom_kill.c | 8 ++++++++ > mm/show_mem.c | 2 ++ > mm/vmstat.c | 7 +++++-- > 6 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 8174b2a8a5dc..db7d304f1905 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -692,6 +692,7 @@ static int proc_oom_score(struct seq_file *m, struct pid_namespace *ns, > unsigned long points = 0; > long badness; > > +#ifdef CONFIG_MEMCG > scoped_guard (rcu) { > struct cgroup_subsys_state *css = task_css(task, memory_cgrp_id); > > @@ -702,6 +703,9 @@ static int proc_oom_score(struct seq_file *m, struct pid_namespace *ns, > memcg = mem_cgroup_from_css(css); > totalpages = mem_cgroup_get_max(memcg); > } > +#else > + scoped_guard (rcu) { > +#endif > } Empty block with scoped_guard ? > > badness = oom_badness(task, totalpages, NULL); > diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c > index ac3c88e68728..4db69eed0e21 100644 > --- a/fs/proc/meminfo.c > +++ b/fs/proc/meminfo.c > @@ -73,6 +73,7 @@ static int meminfo_proc_show_mi(struct seq_file *m, struct meminfo *mi) > return 0; > } > > +#if defined(CONFIG_VE) && defined(CONFIG_MEMCG) > static void fill_meminfo_ve(struct meminfo *mi, struct ve_struct *ve) > { > struct cgroup_subsys_state *css; > @@ -84,6 +85,7 @@ static void fill_meminfo_ve(struct meminfo *mi, struct ve_struct *ve) > css_put(css); > > } > +#endif > > static int meminfo_proc_show_ve(struct seq_file *m, void *v, > struct ve_struct *ve) > @@ -104,11 +106,13 @@ static int meminfo_proc_show_ve(struct seq_file *m, void *v, > mi.si = &i; > mi.ve = ve; > > +#if defined(CONFIG_VE) && defined(CONFIG_MEMCG) > if (!ve_is_super(ve) && ve->meminfo_val == VE_MEMINFO_DEFAULT) { > fill_meminfo_ve(&mi, ve); > > return meminfo_proc_show_mi(m, &mi); > } > +#endif > > committed = vm_memory_committed(); > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 6301319529ee..c27ee90e599c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -714,8 +714,11 @@ percpu_stats_memcg(struct mem_cgroup *memcg, struct mem_cgroup_per_node **pn) > } while (memcg->percpu_stats_disabled); > > if (pn) { > +#ifdef CONFIG_MEMCG_V1 > unsigned int nid = (*pn)->nid; > - > +#else > + unsigned int nid = lruvec_pgdat(&(*pn)->lruvec)->node_id; > +#endif Three objections to deriving the node id from the lruvec instead of keeping the field: 1) It reads a lazily initialized field. lruvec_pgdat(&(*pn)->lruvec) reads pn->lruvec.pgdat, but lruvec_init() memsets the whole structure and never sets pgdat - it is filled in on the first lookup, by mem_cgroup_lruvec(): if (unlikely(lruvec->pgdat != pgdat)) lruvec->pgdat = pgdat; For the only caller that passes a non-NULL pn today - __mod_memcg_lruvec_state(), which always gets its lruvec from mem_cgroup_lruvec() or folio_lruvec() - the pointer is already set by then, so as it stands this will not oops. But it makes a correctness-critical read depend on initialization performed elsewhere: add a second caller whose lruvec comes from another source and you get NULL->node_id, with nothing in the compiler or in the code to warn about it. 2) It costs more on a hot path. __mod_memcg_lruvec_state() is one of the hottest paths in memcg accounting, and this turns a single unsigned short field read into two pointer dereferences (pn->lruvec.pgdat, then ->node_id) for no gain. 3) It makes the two configurations behave differently by construction. With v1 the node id comes from a field, without v1 it is derived from the pgdat. In statistics accounting code that is the last thing you want: the configurations diverge by design rather than by accident. So I went the other way instead - declare nid in the CONFIG_MEMCG_V1=n branch of struct mem_cgroup_per_node as well: #ifdef CONFIG_MEMCG_V1 ... RH_KABI_FILL_HOLE(unsigned short nid) #else + /* + * Not a v1 field: percpu_stats_memcg() needs the node id to find + * the matching per-node structure of the parent memcg. + */ + unsigned short nid; CACHELINE_PADDING(_pad1_); #endif It goes before CACHELINE_PADDING(_pad1_) on purpose: nid is written once at allocation and only read afterwards, so it belongs on the read-mostly side of the false-sharing boundary that the padding exists to maintain. With this, both the reader in percpu_stats_memcg() and the writer in alloc_mem_cgroup_per_node_info() stay unconditional and no .c file has to change. The v1 branch is untouched, so the layout of the shipped kernel - and its kABI checksum - does not change either: RH_KABI_FILL_HOLE() expands to nothing under __GENKSYMS__, and genksyms only ever sees the configuration it is run with, which has MEMCG_V1=y. Note that MEMCG=y with MEMCG_V1=n is not an exotic combination, by the way: MEMCG_V1 is default n upstream, so any plain modern config built from this tree hits it. > *pn = memcg->nodeinfo[nid]; > } > return memcg; > @@ -4179,7 +4182,9 @@ static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > > lruvec_init(&pn->lruvec); > pn->memcg = memcg; > +#ifdef CONFIG_MEMCG_V1 > pn->nid = node; > +#endif > > memcg->nodeinfo[node] = pn; > return true; > @@ -5258,12 +5263,12 @@ static struct cftype memory_files[] = { > .name = "numa_stat", > .seq_show = memory_numa_stat_show, > }, > -#endif > { > .name = "numa_migrate", > .flags = CFTYPE_NOT_ON_ROOT, > .write = memcg_numa_migrate_write, > }, > +#endif > { > .name = "oom.group", > .flags = CFTYPE_NOT_ON_ROOT | CFTYPE_NS_DELEGATABLE, > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 1fd5f99ce3b9..b26a15072d06 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -1034,6 +1034,7 @@ static void __oom_kill_process(struct task_struct *victim, const char *message) > * Kill provided task unless it's secured by setting > * oom_score_adj to OOM_SCORE_ADJ_MIN. > */ > +#ifdef CONFIG_MEMCG > static int oom_kill_memcg_member(struct task_struct *task, void *message) > { > if (task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN && > @@ -1043,10 +1044,12 @@ static int oom_kill_memcg_member(struct task_struct *task, void *message) > } > return 0; > } > +#endif > > /* > * Kill more processes if oom happens too often in this context. > */ > +#ifdef CONFIG_MEMCG > static void oom_berserker(struct oom_control *oc) > { > static DEFINE_RATELIMIT_STATE(berserker_rs, > @@ -1150,6 +1153,9 @@ static void oom_berserker(struct oom_control *oc) > > pr_err("OOM killer in rage %d: %d tasks killed\n", rage, killed); > } > +#else > +static inline void oom_berserker(struct oom_control *oc) { } > +#endif /* CONFIG_MEMCG */ > > atomic_t global_oom = ATOMIC_INIT(0); > > @@ -1196,6 +1202,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) > /* > * If necessary, kill all tasks in the selected memory cgroup. > */ > +#ifdef CONFIG_MEMCG > if (oom_group) { > memcg_memory_event(oom_group, MEMCG_OOM_GROUP_KILL); > mem_cgroup_print_oom_group(oom_group); > @@ -1203,6 +1210,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) > (void *)message); > mem_cgroup_put(oom_group); > } > +#endif > oom_berserker(oc); > } > > diff --git a/mm/show_mem.c b/mm/show_mem.c > index 3ab11c945bf4..4c879177a531 100644 > --- a/mm/show_mem.c > +++ b/mm/show_mem.c > @@ -91,6 +91,7 @@ void si_meminfo(struct sysinfo *val) > > EXPORT_SYMBOL(si_meminfo); > > +#if defined(CONFIG_VE) && defined(CONFIG_MEMCG) > void si_meminfo_ve(struct sysinfo *si, struct ve_struct *ve) > { > unsigned long memtotal, memused, swaptotal, swapused; > @@ -138,6 +139,7 @@ void si_meminfo_ve(struct sysinfo *si, struct ve_struct *ve) > /* bufferram, totalhigh and freehigh left 0 */ > } > EXPORT_SYMBOL(si_meminfo_ve); > +#endif > > #ifdef CONFIG_NUMA > void si_meminfo_node(struct sysinfo *val, int nid) > diff --git a/mm/vmstat.c b/mm/vmstat.c > index 47cb6bf4ecec..de4f8b4e51b6 100644 > --- a/mm/vmstat.c > +++ b/mm/vmstat.c > @@ -1855,6 +1855,7 @@ static const struct seq_operations zoneinfo_op = { > (IS_ENABLED(CONFIG_VM_EVENT_COUNTERS) ? \ > NR_VM_EVENT_ITEMS : 0)) > > +#if defined(CONFIG_VE) && defined(CONFIG_MEMCG) > static void fill_vmstat_ve(unsigned long *stat, struct ve_struct *ve) > { > struct cgroup_subsys_state *css; > @@ -1863,10 +1864,10 @@ static void fill_vmstat_ve(unsigned long *stat, struct ve_struct *ve) > mem_cgroup_fill_vmstat(mem_cgroup_from_css(css), stat); > css_put(css); > } > +#endif > > static void *vmstat_start(struct seq_file *m, loff_t *pos) > { > - struct ve_struct *ve; > unsigned long *v; > int i; > > @@ -1880,12 +1881,14 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos) > if (!v) > return ERR_PTR(-ENOMEM); > > - ve = get_exec_env(); > +#if defined(CONFIG_VE) && defined(CONFIG_MEMCG) > + struct ve_struct *ve = get_exec_env(); > if (!ve_is_super(ve)) { > memset(v, 0, NR_VMSTAT_ITEMS * sizeof(unsigned long)); > fill_vmstat_ve(v, ve); > return (unsigned long *)m->private + *pos; > } > +#endif > > for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) > v[i] = global_zone_page_state(i);