Re: madvise09: restore cgroup v1 memory limits with numeric values

On Thu, Jul 30, 2026 at 08:29:01AM +0000, [email protected] wrote:
> Hi Li,
> 
> On Thu, 30 Jul 2026, Li Wang wrote:
> > madvise09: restore cgroup v1 memory limits with numeric values
> 
> --- [PATCH 1/2] ---
> 
> > +	if (TST_CG_VER_IS_V1(tst_cg, "memory")) {
> > +		SAFE_CG_PRINTF(tst_cg, "memory.max", "%lld", ori_mem_limit);
> > +
> > +		if (swap_accounting_enabled)
> > +			SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%lld", ori_swap_limit);
> > +	} else {
> 
> Doesn't this hit EINVAL again on v1, just with a different value?
> 
> On the retry path the cgroup still carries the limits written by the
> previous child at the end of child():
> 
> 	memory.max      = mem_limit  = 16 * PAGES * page_size  (8MB @4K)
> 	memory.swap.max = swap_limit = 2 * mem_limit           (16MB @4K)
> 
> On v1 these alias to memory.limit_in_bytes and
> memory.memsw.limit_in_bytes, and the kernel enforces
> memory.max <= memsw.max in mem_cgroup_resize_max()
> (mm/memcontrol-v1.c):
> 
> 	limits_invariant = memsw ? max >= READ_ONCE(memcg->memory.max) :
> 				   max <= memcg->memsw.max;
> 	if (!limits_invariant) {
> 		mutex_unlock(&memcg_max_mutex);
> 		ret = -EINVAL;
> 		break;
> 	}
> 
> ori_mem_limit is the v1 default (PAGE_COUNTER_MAX, ~8EB), which is well
> above the 16MB memsw limit still installed. So writing memory.max first
> should fail with EINVAL and SAFE_CG_PRINTF aborts with TBROK.
> 
> When raising the limits the writes have to go memsw first:
> 
> 	if (swap_accounting_enabled)
> 		SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%lld", ori_swap_limit);
> 
> 	SAFE_CG_PRINTF(tst_cg, "memory.max", "%lld", ori_mem_limit);
> 
> The lowering sequence further down stays as it is, since lowering
> memory.max first keeps the invariant.

Yes, good point. On cgroup v1 the retry path is raising both limits back
to their original values, and raising memory.limit_in_bytes before
memory.memsw.limit_in_bytes can violate the v1 invariant
memory.limit_in_bytes <= memory.memsw.limit_in_bytes while the old memsw
limit is still installed.

I will fix this in V2.

> 
> >  	/*
> >  	 * Reset cgroup memory limits to default ("max") in case this is a retry run.
> 
> The comment above the hunk still says the limits are reset to "max",
> which is now only true for the v2 branch. Could it be reworded to cover
> both cases?

Sure, this can be rewriten.

> 
> > -	if (SAFE_CG_HAS(tst_cg, "memory.swap.max"))
> > +	if (TST_CG_VER_IS_V1(tst_cg, "memory"))
> > +		SAFE_CG_SCANF(tst_cg, "memory.max", "%lld", &ori_mem_limit);
> > +
> > +	if (SAFE_CG_HAS(tst_cg, "memory.swap.max")) {
> >  		swap_accounting_enabled = 1;
> > -	else
> > +
> > +		if (TST_CG_VER_IS_V1(tst_cg, "memory"))
> > +			SAFE_CG_SCANF(tst_cg, "memory.swap.max", "%lld", &ori_swap_limit);
> 
> TST_CG_VER_IS_V1() expands to a tst_cg_ver() call and is evaluated three
> times across setup() and child(). Would storing it once in a static
> (e.g. "static int cg_v1;") be simpler, given child() needs it anyway?

Hmm, this is a little bit nitpicking.

> Minor: LTP usually spells such saved values "orig_*", so orig_mem_limit
> and orig_swap_limit would match the rest of the tree.

Sure, orig_* looks better.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp