[LTP] [PATCH 1/2] madvise09: restore cgroup v1 memory limits with numeric values

[LTP] [PATCH 1/2] madvise09: restore cgroup v1 memory limits with numeric values

[Li Wang]

From: Li Wang <liwang@hygon.cn>

madvise09 resets memory.max and memory.swap.max to "max" before retrying
the child process, so the retried child does not inherit the strict memory
limits from the previous run.

This works for cgroup v2, where "max" is the valid unlimited value.
However, cgroup v1 memory.limit_in_bytes does not accept the string "max".
When the LTP cgroup abstraction maps memory.max to the v1 memory limit
knob, writing "max" can fail with EINVAL:

  tst_test.c:1875: TINFO: Overall timeout per run is 0h 00m 30s
  madvise09.c:154: TBROK: vdprintf(18</sys/fs/cgroup/memory/ltp/test-27493>,
		   	  'memory.limit_in_bytes', '%s'<max>): EINVAL (22)
  tst_test.c:478: TINFO: Child process reported TBROK killing the test
  tst_test.c:1938: TINFO: Killed the leftover descendant processes

Save the original cgroup v1 memory and swap limits during setup and
restore those numeric values on retry. Keep using "max" for cgroup v2.

Fixes: c118be931cd ("madvise09: Reset cgroup limits before retrying test")
Signed-off-by: Li Wang <liwang@hygon.cn>
---
 testcases/kernel/syscalls/madvise/madvise09.c | 29 ++++++++++++++-----
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/testcases/kernel/syscalls/madvise/madvise09.c b/testcases/kernel/syscalls/madvise/madvise09.c
index 2fc1da581..2e792bd11 100644
--- a/testcases/kernel/syscalls/madvise/madvise09.c
+++ b/testcases/kernel/syscalls/madvise/madvise09.c
@@ -54,8 +54,8 @@ static int swap_accounting_enabled;
 #define TOUCHED_PAGE1 0
 #define TOUCHED_PAGE2 10
 
-static long long mem_limit;
-static long long swap_limit;
+static long long mem_limit, ori_mem_limit;
+static long long swap_limit, ori_swap_limit;
 
 static void memory_pressure_child(void)
 {
@@ -151,9 +151,17 @@ static void child(void)
 	 * Otherwise, the retried child inherits the strict MEM_LIMIT from the previous
 	 * run, causing MADV_FREE pages to be dropped immediately before we touch them.
 	 */
-	SAFE_CG_PRINT(tst_cg, "memory.max", "max");
-	if (swap_accounting_enabled)
-		SAFE_CG_PRINT(tst_cg, "memory.swap.max", "max");
+	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 {
+		SAFE_CG_PRINT(tst_cg, "memory.max", "max");
+
+		if (swap_accounting_enabled)
+			SAFE_CG_PRINT(tst_cg, "memory.swap.max", "max");
+	}
 
 	ptr = SAFE_MMAP(NULL, PAGES * page_size, PROT_READ | PROT_WRITE,
 			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
@@ -278,10 +286,17 @@ retry:
 
 static void setup(void)
 {
-	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);
+	} else {
 		tst_res(TINFO, "Swap accounting is disabled");
+	}
 
 	page_size = getpagesize();
 
-- 
2.55.0


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

[LTP] [PATCH 2/2] mailmap: enable liwang@hygon.cn address

[Li Wang]

Signed-off-by: Li Wang <liwang@hygon.cn>
---
 .mailmap | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index d1e4419f0..35f54a937 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1,3 +1,4 @@
+Li Wang <li.wang@linux.dev> <liwang@hygon.cn>
 Li Wang <li.wang@linux.dev> <liwang@redhat.com>
 Li Wang <li.wang@linux.dev> <wangli.ahau@gmail.com>
 Petr Vorel <pvorel@suse.cz> <petr.vorel@gmail.com>
-- 
2.55.0


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

Re: [LTP] [PATCH 2/2] mailmap: enable liwang@hygon.cn address

[Andrea Cervesato via ltp]

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH 2/2] mailmap: enable liwang@hygon.cn address

[Petr Vorel]

Hi Li,

> Signed-off-by: Li Wang <liwang@hygon.cn>
> ---
>  .mailmap | 1 +
>  1 file changed, 1 insertion(+)

> diff --git a/.mailmap b/.mailmap
> index d1e4419f0..35f54a937 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1,3 +1,4 @@
> +Li Wang <li.wang@linux.dev> <liwang@hygon.cn>

Acked-by: Petr Vorel <pvorel@suse.cz>

Congratulation to a new job :).

Kind regards,
Petr

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

Re: [LTP] [PATCH 2/2] mailmap: enable liwang@hygon.cn address

[Li Wang]

Petr Vorel wrote:

> Hi Li,
> 
> > Signed-off-by: Li Wang <liwang@hygon.cn>
> > ---
> >  .mailmap | 1 +
> >  1 file changed, 1 insertion(+)
> 
> > diff --git a/.mailmap b/.mailmap
> > index d1e4419f0..35f54a937 100644
> > --- a/.mailmap
> > +++ b/.mailmap
> > @@ -1,3 +1,4 @@
> > +Li Wang <li.wang@linux.dev> <liwang@hygon.cn>
> 
> Acked-by: Petr Vorel <pvorel@suse.cz>
> 
> Congratulation to a new job :).

Thank you Petr, Andrea.

Yes, I have started a new job and will still be working on
Linux quality related area, with a continued focus on LTP :).

Just sent out v2 but forgot to include both of your Acked-by tags;
I will add them when merging.

-- 
Regards,
Li Wang

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

Re: [LTP] [PATCH 2/2] mailmap: enable liwang@hygon.cn address

[Andrea Cervesato via ltp]

> Yes, I have started a new job and will still be working on
> Linux quality related area, with a continued focus on LTP :).
> 

That sounds fantastic! Thanks for continuing LTP maintenance.
It's really appreciate :-)

And congratulations for your new career!

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

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

[linuxtestproject.agent]

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 <li.wang@linux.dev> <liwang@hygon.cn>

Patch 1/2 is already signed off with liwang@hygon.cn, 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

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

[Li Wang]

On Thu, Jul 30, 2026 at 08:29:01AM +0000, linuxtestproject.agent@gmail.com 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

Re: [LTP] [PATCH 1/2] madvise09: restore cgroup v1 memory limits with numeric values

[Andrea Cervesato via ltp]

Hi Li,

>  static void memory_pressure_child(void)
>  {
> @@ -151,9 +151,17 @@ static void child(void)
>  	 * Otherwise, the retried child inherits the strict MEM_LIMIT from the previous
>  	 * run, causing MADV_FREE pages to be dropped immediately before we touch them.
>  	 */
> -	SAFE_CG_PRINT(tst_cg, "memory.max", "max");
> -	if (swap_accounting_enabled)
> -		SAFE_CG_PRINT(tst_cg, "memory.swap.max", "max");
> +	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 {
> +		SAFE_CG_PRINT(tst_cg, "memory.max", "max");
> +
> +		if (swap_accounting_enabled)
> +			SAFE_CG_PRINT(tst_cg, "memory.swap.max", "max");
> +	}
>  
>  	ptr = SAFE_MMAP(NULL, PAGES * page_size, PROT_READ | PROT_WRITE,
>  			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> @@ -278,10 +286,17 @@ retry:
>  
>  static void setup(void)
>  {
> -	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);
> +	} else {
>  		tst_res(TINFO, "Swap accounting is disabled");
> +	}


I see a pattern inside text function and setup(), maybe we can create
a function that reset memory.max and memory.swap.max in one place.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH 1/2] madvise09: restore cgroup v1 memory limits with numeric values

[Li Wang]

Andrea Cervesato wrote:

> > +	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 {
> > +		SAFE_CG_PRINT(tst_cg, "memory.max", "max");
> > +
> > +		if (swap_accounting_enabled)
> > +			SAFE_CG_PRINT(tst_cg, "memory.swap.max", "max");
> > +	}

> >  static void setup(void)
> >  {
> > -	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);
> > +	} else {
> >  		tst_res(TINFO, "Swap accounting is disabled");
> > +	}
> 
> 
> I see a pattern inside text function and setup(), maybe we can create
> a function that reset memory.max and memory.swap.max in one place.

Not exactly, the logic in setup (save) and test (restore) are
not identical, one reads the other writes. 

Or, did I miss anything there?

-- 
Regards,
Li Wang

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

Re: [LTP] [PATCH 1/2] madvise09: restore cgroup v1 memory limits with numeric values

[Andrea Cervesato via ltp]

Hi Li,

> Not exactly, the logic in setup (save) and test (restore) are
> not identical, one reads the other writes. 
> 
> Or, did I miss anything there?

Uops :-) feel free to add:

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>

and merge then.

Thanks,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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