Re: [LTP] madvise09: restore cgroup v1 memory limits with numeric values

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.

>  	/*
>  	 * 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?

> -	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?

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

--- [PATCH 2/2] ---

> +Li Wang <[email protected]> <[email protected]>

Patch 1/2 is already signed off with [email protected], so would it make
sense to order this one first in the series?

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

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