[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[zhaogongyi]

Hi Richard,

That's right. I have a Misunderstanding of cgroup.procs. Actrually, we can write pid or tgid to cgroup.procs instead of pgid.

I'll update the code as:

	SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpid());

BTW I find that the testcase run passed on my local system, it seems that tst_* will not trig memory allocation:

tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
memcg_kmem_limit_in_bytes.c:58: TPASS: fork fail as expected
memcg_kmem_limit_in_bytes.c:73: TPASS: kmem alloc is controled by memory.kmem.limit_in_bytes

Summary:
passed   2
failed   0
broken   0
skipped  0
warnings 0


Thanks very much!

Best Regards,
Gongyi

> 
> Hello,
> 
> zhaogongyi <zhaogongyi@huawei.com> writes:
> 
> > Hi Richard,
> >
> > Calling setpgid in this testcase is aim to make the task group be
> controlled by memory.kmem.limit_in_bytes since we have set the pgid to
> cgroup.procs.
> >
> 
> Please see cgroup_procs_write_start in cgroup/cgroup.c. It calls
> find_task_by_vpid which interprets pid as PIDTYPE_PID not PIDTYPE_PGID.
> 
> --
> Thank you,
> Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Richard Palethorpe]

Hello,

zhaogongyi <zhaogongyi@huawei.com> writes:

> Hi Richard,
>
> That's right. I have a Misunderstanding of cgroup.procs. Actrually, we can write pid or tgid to cgroup.procs instead of pgid.
>
> I'll update the code as:
>
> 	SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpid());
>
> BTW I find that the testcase run passed on my local system, it seems
> that tst_* will not trig memory allocation:

After a short search I found do_tty_write can return ENOMEM due to a
call to kmalloc with GFP_KERNEL.

As I said previously stderr can point to many things with different
write implementations. Some of these are likely to trigger memory
allocation. Please see my previous reply and suggestion.

Also please note the warning message in the kernel log when using
memory.kmem.limit_in_bytes.

>
> tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> memcg_kmem_limit_in_bytes.c:58: TPASS: fork fail as expected
> memcg_kmem_limit_in_bytes.c:73: TPASS: kmem alloc is controled by
> memory.kmem.limit_in_bytes
>
> Summary:
> passed   2
> failed   0
> broken   0
> skipped  0
> warnings 0
>
>
> Thanks very much!
>
> Best Regards,
> Gongyi
>
>> 
>> Hello,
>> 
>> zhaogongyi <zhaogongyi@huawei.com> writes:
>> 
>> > Hi Richard,
>> >
>> > Calling setpgid in this testcase is aim to make the task group be
>> controlled by memory.kmem.limit_in_bytes since we have set the pgid to
>> cgroup.procs.
>> >
>> 
>> Please see cgroup_procs_write_start in cgroup/cgroup.c. It calls
>> find_task_by_vpid which interprets pid as PIDTYPE_PID not PIDTYPE_PGID.
>> 
>> --
>> Thank you,
>> Richard.


-- 
Thank you,
Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Cyril Hrubis]

Hi!
> BTW I find that the testcase run passed on my local system, it seems
> that tst_* will not trig memory allocation:

This is not, by any means, a proof that the test is correct.

"Passed on my machine" is nothing but poor excuse.

I can easily write utterly broken code that will work fine on a single
machine and fail on anything else. Correctly written test depends on
valid assumptions we can make about the operation system the test is
supposed to run on.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[zhaogongyi]

Hi Richard, Cyril,

Thanks for your review!

1) I can't agree with you more that correctly written test depends on valid assumptions we can make about the operation system the test is supposed to run on

2)when we use tst_res/tst_brk to print information to STDIO, it seems have not trig kmem allocation, I have checked the kernel log,
there is only one malloc failed log in dmesg info. And we can see the STDIO info is printed as expected. 

3)when we use tst_res/tst_brk to print information to other file, it might trig memory alloction. So, we need remove the calling of tst_res/tst_brk for safety.

I have resubmit the patch according to your review, please see: https://patchwork.ozlabs.org/project/ltp/patch/20210415032911.7542-1-zhaogongyi@huawei.com/


Thanks very much!

Best Regards,
Gongyi

> 
> Hello,
> 
> zhaogongyi <zhaogongyi@huawei.com> writes:
> 
> > Hi Richard,
> >
> > That's right. I have a Misunderstanding of cgroup.procs. Actrually, we can
> write pid or tgid to cgroup.procs instead of pgid.
> >
> > I'll update the code as:
> >
> > 	SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpid());
> >
> > BTW I find that the testcase run passed on my local system, it seems
> > that tst_* will not trig memory allocation:
> 
> After a short search I found do_tty_write can return ENOMEM due to a
> call to kmalloc with GFP_KERNEL.
> 
> As I said previously stderr can point to many things with different write
> implementations. Some of these are likely to trigger memory allocation.
> Please see my previous reply and suggestion.
> 
> Also please note the warning message in the kernel log when using
> memory.kmem.limit_in_bytes.
> 
> >
> > tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> > memcg_kmem_limit_in_bytes.c:58: TPASS: fork fail as expected
> > memcg_kmem_limit_in_bytes.c:73: TPASS: kmem alloc is controled by
> > memory.kmem.limit_in_bytes
> >
> > Summary:
> > passed   2
> > failed   0
> > broken   0
> > skipped  0
> > warnings 0
> >
> >
> > Thanks very much!
> >
> > Best Regards,
> > Gongyi
> >
> >>
> >> Hello,
> >>
> >> zhaogongyi <zhaogongyi@huawei.com> writes:
> >>
> >> > Hi Richard,
> >> >
> >> > Calling setpgid in this testcase is aim to make the task group be
> >> controlled by memory.kmem.limit_in_bytes since we have set the pgid
> >> to cgroup.procs.
> >> >
> >>
> >> Please see cgroup_procs_write_start in cgroup/cgroup.c. It calls
> >> find_task_by_vpid which interprets pid as PIDTYPE_PID not
> PIDTYPE_PGID.
> >>
> >> --
> >> Thank you,
> >> Richard.
> 
> 
> --
> Thank you,
> Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Richard Palethorpe]

Hello,

zhaogongyi <zhaogongyi@huawei.com> writes:

> Hi Richard,
>
> Calling setpgid in this testcase is aim to make the task group be controlled by memory.kmem.limit_in_bytes since we have set the pgid to cgroup.procs.
>

Please see cgroup_procs_write_start in cgroup/cgroup.c. It calls
find_task_by_vpid which interprets pid as PIDTYPE_PID not PIDTYPE_PGID.

-- 
Thank you,
Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[zhaogongyi]

Hi Richard,

Calling setpgid in this testcase is aim to make the task group be controlled by memory.kmem.limit_in_bytes since we have set the pgid to cgroup.procs.

In this testcase, memory cgroup is just effective for the task group of cgroup.procs. it is no affection for the parent process as expect.

So if fork success, we can use tst_* and would not be failed, otherwise, we can use tst_* since it is in parent process.

I don't know if I express it clearly.


Thanks very much!

Best Regards,
Gongyi

> 
> Hello,
> 
> zhaogongyi <zhaogongyi@huawei.com> writes:
> 
> > Hi Richard,
> >
> > Thanks so much for your review!
> >
> >> >> > +	pid = SAFE_FORK();
> >> >> > +	if (!pid) {
> >> >> > +		SAFE_SETPGID(0, 0);
> >> >> > +
> >> >> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
> >> >> > +
> >> >> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d",
> getpgid(getpid()));
> >> >>
> >> >> After setting this we should avoid doing any work in this process
> >> >> except calling fork. AFAIK there is no guarantee that some other
> >> >> syscall or tasklet etc. Won't try to allocate memory and also fail.
> >> >>
> >> >
> >> > It seems that we would not be afraid since there is no asynchronous
> >> > operation in the test process.
> >>
> >> IIRC one of the problems with tasklets is they can run in any
> >> process. Also, in theory stdout can point to anything. So write() might
> allocate memory.
> >>
> >> Why not do something like:
> >>
> >> SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid())); errno = 0;
> >> fork(); exit(errno);
> >>
> >> Then check that errno == ENOMEM, in the parent process?
> >>
> >>
> >>> > +		TEST(fork());
> >>> > +		if (TST_RET == -1) {
> >>> > +			if (TST_ERR == ENOMEM)
> >>> > +				tst_res(TPASS, "fork fail as expected");
> >>> > +			else
> >>> > +				tst_brk(TFAIL | TTERRNO,
> >>> > +					"fork fail as unexpected");
> >
> > As far as my personal understanding, after setpgid, the controlled
> > process is the sub process whose process id is pid, excluded from the
> parent process, and the functions tst_res or tst_brk Is executed in its
> parent process.
> 
> setpgid(0, 0) sets the process group id (pgid) to be the same as the (pid),
> which creates a new process group. This probably has the effect that the
> calling process will not recieve signals from the terminal. Also it should
> receive SIGTTOU when calling tst_res and tst_brk, if stdout/stderr point to
> a terminal with the right configuration.
> 
> AFAICT process groups are for shells and affect where signals are sent.
> Nothing else.
> 
> >
> > In this case, is it no affection to this test?
> >
> >
> > Best Regards,
> > Gongyi
> 
> 
> --
> Thank you,
> Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Richard Palethorpe]

Hello,

zhaogongyi <zhaogongyi@huawei.com> writes:

> Hi Richard,
>
> Thanks so much for your review!
>
>> >> > +	pid = SAFE_FORK();
>> >> > +	if (!pid) {
>> >> > +		SAFE_SETPGID(0, 0);
>> >> > +
>> >> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
>> >> > +
>> >> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
>> >>
>> >> After setting this we should avoid doing any work in this process
>> >> except calling fork. AFAIK there is no guarantee that some other
>> >> syscall or tasklet etc. Won't try to allocate memory and also fail.
>> >>
>> >
>> > It seems that we would not be afraid since there is no asynchronous
>> > operation in the test process.
>> 
>> IIRC one of the problems with tasklets is they can run in any process. Also,
>> in theory stdout can point to anything. So write() might allocate memory.
>> 
>> Why not do something like:
>> 
>> SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid())); errno = 0;
>> fork(); exit(errno);
>> 
>> Then check that errno == ENOMEM, in the parent process?
>>
>>
>>> > +		TEST(fork());
>>> > +		if (TST_RET == -1) {
>>> > +			if (TST_ERR == ENOMEM)
>>> > +				tst_res(TPASS, "fork fail as expected");
>>> > +			else
>>> > +				tst_brk(TFAIL | TTERRNO,
>>> > +					"fork fail as unexpected");
>
> As far as my personal understanding, after setpgid, the controlled process is the sub process whose process id is pid, excluded from the parent process, and the functions tst_res or tst_brk
> Is executed in its parent process.

setpgid(0, 0) sets the process group id (pgid) to be the same as the
(pid), which creates a new process group. This probably has the effect
that the calling process will not recieve signals from the
terminal. Also it should receive SIGTTOU when calling tst_res and
tst_brk, if stdout/stderr point to a terminal with the right
configuration.

AFAICT process groups are for shells and affect where signals are
sent. Nothing else.

>
> In this case, is it no affection to this test?
>
>
> Best Regards,
> Gongyi


-- 
Thank you,
Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[zhaogongyi]

Hi Richard,

Thanks so much for your review!

> >> > +	pid = SAFE_FORK();
> >> > +	if (!pid) {
> >> > +		SAFE_SETPGID(0, 0);
> >> > +
> >> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
> >> > +
> >> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
> >>
> >> After setting this we should avoid doing any work in this process
> >> except calling fork. AFAIK there is no guarantee that some other
> >> syscall or tasklet etc. Won't try to allocate memory and also fail.
> >>
> >
> > It seems that we would not be afraid since there is no asynchronous
> > operation in the test process.
> 
> IIRC one of the problems with tasklets is they can run in any process. Also,
> in theory stdout can point to anything. So write() might allocate memory.
> 
> Why not do something like:
> 
> SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid())); errno = 0;
> fork(); exit(errno);
> 
> Then check that errno == ENOMEM, in the parent process?
>
>
>> > +		TEST(fork());
>> > +		if (TST_RET == -1) {
>> > +			if (TST_ERR == ENOMEM)
>> > +				tst_res(TPASS, "fork fail as expected");
>> > +			else
>> > +				tst_brk(TFAIL | TTERRNO,
>> > +					"fork fail as unexpected");

As far as my personal understanding, after setpgid, the controlled process is the sub process whose process id is pid, excluded from the parent process, and the functions tst_res or tst_brk
Is executed in its parent process.

In this case, is it no affection to this test?


Best Regards,
Gongyi

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Richard Palethorpe]

Hello,

zhaogongyi <zhaogongyi@huawei.com> writes:

> Hi Richard,
>
> Yes, I will pay attention on your patchs about cgroup API.
>
>> > +static void run(void)
>> > +{
>> > +	pid_t pid;
>> > +	int st;
>> > +
>> > +	pid = SAFE_FORK();
>> > +	if (!pid) {
>> > +		SAFE_SETPGID(0, 0);
>> > +
>> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
>> > +
>> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
>> 
>> After setting this we should avoid doing any work in this process except
>> calling fork. AFAIK there is no guarantee that some other syscall or tasklet
>> etc. Won't try to allocate memory and also fail.
>>
>
> It seems that we would not be afraid since there is no asynchronous
> operation in the test process.

IIRC one of the problems with tasklets is they can run in any
process. Also, in theory stdout can point to anything. So write() might
allocate memory.

Why not do something like:

SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
errno = 0;
fork();
exit(errno);

Then check that errno == ENOMEM, in the parent process?

>
>
>> > +		TEST(fork());
>> > +		if (TST_RET == -1) {
>> > +			if (TST_ERR == ENOMEM)
>> > +				tst_res(TPASS, "fork fail as expected");
>> > +			else
>> > +				tst_brk(TFAIL | TTERRNO,
>> > +					"fork fail as unexpected");
>> 
>> This should be "fork failed with unexpected error".
>
> It's right. Thanks for your review.

Thanks :-)

>
> Best Regards,
> Gongyi
>
>
>> 
>> Hello,
>> 
>> Zhao Gongyi <zhaogongyi@huawei.com> writes:
>> 
>> > Add memory cgroup testcase for checking that kmem overflow is
>> > controlled by kmem.limit_in_bytes.
>> 
>> I am currently working on a new LTP CGroups API, please see:
>> https://patchwork.ozlabs.org/project/ltp/list/?series=238773
>> 
>> If the new API is merged quickly, then this test will need to be converted
>> to it. Also please see comments below.
>> 
>> >
>> > Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
>> > ---
>> > v1->v2:
>> > 	1) add testcase to runtest/controllers
>> > 	2) add testcase to testcases/kernel/controllers/memcg/.gitignore
>> > ---
>> >  runtest/controllers                           |  1 +
>> >  testcases/kernel/controllers/memcg/.gitignore |  1 +
>> >  .../functional/memcg_kmem_limit_in_bytes.c    | 87
>> +++++++++++++++++++
>> >  3 files changed, 89 insertions(+)
>> >  create mode 100644
>> >
>> testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_b
>> yte
>> > s.c
>> >
>> > diff --git a/runtest/controllers b/runtest/controllers index
>> > e3d0243f1..f13a112c7 100644
>> > --- a/runtest/controllers
>> > +++ b/runtest/controllers
>> > @@ -15,6 +15,7 @@ memcg_use_hierarchy
>> 	memcg_use_hierarchy_test.sh
>> >  memcg_usage_in_bytes	memcg_usage_in_bytes_test.sh
>> >  memcg_stress		memcg_stress_test.sh
>> >  memcg_control		memcg_control_test.sh
>> > +memcg_kmem_limit_in_bytes memcg_kmem_limit_in_bytes
>> >
>> >  cgroup_fj_function_debug cgroup_fj_function.sh debug
>> > cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset diff --git
>> > a/testcases/kernel/controllers/memcg/.gitignore
>> > b/testcases/kernel/controllers/memcg/.gitignore
>> > index c0b6d0714..dce8412de 100644
>> > --- a/testcases/kernel/controllers/memcg/.gitignore
>> > +++ b/testcases/kernel/controllers/memcg/.gitignore
>> > @@ -1,5 +1,6 @@
>> >  /control/mem_process
>> >  /functional/memcg_process
>> > +/functional/memcg_kmem_limit_in_bytes
>> >  /regression/memcg_test_1
>> >  /regression/memcg_test_2
>> >  /regression/memcg_test_3
>> > diff --git
>> >
>> a/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in
>> _by
>> > tes.c
>> >
>> b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in
>> _by
>> > tes.c
>> > new file mode 100644
>> > index 000000000..61f3f6fd5
>> > --- /dev/null
>> > +++
>> b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_i
>> > +++ n_bytes.c
>> > @@ -0,0 +1,87 @@
>> > +// SPDX-License-Identifier: GPL-2.0-or-later
>> > +/*
>> > + * Copyright (c) 2021 HUAWEI LIMITED
>> > + * Author: Zhao Gongyi <zhaogongyi@huawei.com>  */
>> > +
>> > +/*\
>> > + * [Description]
>> > + * Check that kmem overflow is controlled by kmem.limit_in_bytes.
>> > + *
>> > + * [Algorithm]
>> > + * 1) mount memory cgroup
>> > + * 2) set 0 to memory.kmem.limit_in_bytes
>> > + * 3) set test process group to cgroup.procs
>> > + * 4) fork in test process group to trig kmem overflow  */
>> > +
>> > +#include <sys/wait.h>
>> > +#include "tst_test.h"
>> > +
>> > +#define MNT_POINT "memcg"
>> > +#define TESTDIR "memcg/ltpmemcg"
>> > +#define CGROUP_PROCS "memcg/ltpmemcg/cgroup.procs"
>> > +#define KMEM_LIMIT_IN_BYTES
>> "memcg/ltpmemcg/memory.kmem.limit_in_bytes"
>> > +
>> > +static void cleanup(void)
>> > +{
>> > +	SAFE_RMDIR(TESTDIR);
>> > +	SAFE_UMOUNT(MNT_POINT);
>> > +	SAFE_RMDIR(MNT_POINT);
>> > +}
>> > +
>> > +static void setup(void)
>> > +{
>> > +	SAFE_MKDIR(MNT_POINT, 0755);
>> > +	SAFE_MOUNT("memcg", MNT_POINT, "cgroup", 0, "memory");
>> > +	SAFE_MKDIR(TESTDIR, 0755);
>> > +}
>> > +
>> > +static void run(void)
>> > +{
>> > +	pid_t pid;
>> > +	int st;
>> > +
>> > +	pid = SAFE_FORK();
>> > +	if (!pid) {
>> > +		SAFE_SETPGID(0, 0);
>> > +
>> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
>> > +
>> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
>> 
>> After setting this we should avoid doing any work in this process except
>> calling fork. AFAIK there is no guarantee that some other syscall or tasklet
>> etc. Won't try to allocate memory and also fail.
>> 
>> > +
>> > +		TEST(fork());
>> > +		if (TST_RET == -1) {
>> > +			if (TST_ERR == ENOMEM)
>> > +				tst_res(TPASS, "fork fail as expected");
>> > +			else
>> > +				tst_brk(TFAIL | TTERRNO,
>> > +					"fork fail as unexpected");
>> 
>> This should be "fork failed with unexpected error".
>> 
>> > +		} else if (TST_RET == 0) {
>> > +			tst_brk(TFAIL, "fork success as unexpected");
>> > +		} else {
>> > +			SAFE_WAIT(NULL);
>> > +			tst_brk(TFAIL, "fork success as unexpected");
>> > +		}
>> > +	} else {
>> > +		SAFE_WAIT(&st);
>> > +		if (WIFEXITED(st))
>> > +			if (!WEXITSTATUS(st)) {
>> > +				tst_res(TPASS, "kmem alloc is controled "
>> > +					"by memory.kmem.limit_in_bytes");
>> > +				return;
>> > +			}
>> > +
>> > +		tst_res(TFAIL,"kmem alloc is not controled "
>> > +			"by memory.kmem.limit_in_bytes");
>> > +	}
>> > +}
>> > +
>> > +static struct tst_test test = {
>> > +	.needs_root = 1,
>> > +	.needs_tmpdir = 1,
>> > +	.forks_child = 1,
>> > +	.setup = setup,
>> > +	.test_all = run,
>> > +	.cleanup = cleanup,
>> > +};
>> > --
>> > 2.17.1
>> 
>> 
>> --
>> Thank you,
>> Richard.


-- 
Thank you,
Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[zhaogongyi]

Hi Richard,

Yes, I will pay attention on your patchs about cgroup API.

> > +static void run(void)
> > +{
> > +	pid_t pid;
> > +	int st;
> > +
> > +	pid = SAFE_FORK();
> > +	if (!pid) {
> > +		SAFE_SETPGID(0, 0);
> > +
> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
> > +
> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
> 
> After setting this we should avoid doing any work in this process except
> calling fork. AFAIK there is no guarantee that some other syscall or tasklet
> etc. Won't try to allocate memory and also fail.
>

It seems that we would not be afraid since there is no asynchronous operation in the test process.


> > +		TEST(fork());
> > +		if (TST_RET == -1) {
> > +			if (TST_ERR == ENOMEM)
> > +				tst_res(TPASS, "fork fail as expected");
> > +			else
> > +				tst_brk(TFAIL | TTERRNO,
> > +					"fork fail as unexpected");
> 
> This should be "fork failed with unexpected error".

It's right. Thanks for your review.

Best Regards,
Gongyi


> 
> Hello,
> 
> Zhao Gongyi <zhaogongyi@huawei.com> writes:
> 
> > Add memory cgroup testcase for checking that kmem overflow is
> > controlled by kmem.limit_in_bytes.
> 
> I am currently working on a new LTP CGroups API, please see:
> https://patchwork.ozlabs.org/project/ltp/list/?series=238773
> 
> If the new API is merged quickly, then this test will need to be converted
> to it. Also please see comments below.
> 
> >
> > Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> > ---
> > v1->v2:
> > 	1) add testcase to runtest/controllers
> > 	2) add testcase to testcases/kernel/controllers/memcg/.gitignore
> > ---
> >  runtest/controllers                           |  1 +
> >  testcases/kernel/controllers/memcg/.gitignore |  1 +
> >  .../functional/memcg_kmem_limit_in_bytes.c    | 87
> +++++++++++++++++++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644
> >
> testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_b
> yte
> > s.c
> >
> > diff --git a/runtest/controllers b/runtest/controllers index
> > e3d0243f1..f13a112c7 100644
> > --- a/runtest/controllers
> > +++ b/runtest/controllers
> > @@ -15,6 +15,7 @@ memcg_use_hierarchy
> 	memcg_use_hierarchy_test.sh
> >  memcg_usage_in_bytes	memcg_usage_in_bytes_test.sh
> >  memcg_stress		memcg_stress_test.sh
> >  memcg_control		memcg_control_test.sh
> > +memcg_kmem_limit_in_bytes memcg_kmem_limit_in_bytes
> >
> >  cgroup_fj_function_debug cgroup_fj_function.sh debug
> > cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset diff --git
> > a/testcases/kernel/controllers/memcg/.gitignore
> > b/testcases/kernel/controllers/memcg/.gitignore
> > index c0b6d0714..dce8412de 100644
> > --- a/testcases/kernel/controllers/memcg/.gitignore
> > +++ b/testcases/kernel/controllers/memcg/.gitignore
> > @@ -1,5 +1,6 @@
> >  /control/mem_process
> >  /functional/memcg_process
> > +/functional/memcg_kmem_limit_in_bytes
> >  /regression/memcg_test_1
> >  /regression/memcg_test_2
> >  /regression/memcg_test_3
> > diff --git
> >
> a/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in
> _by
> > tes.c
> >
> b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in
> _by
> > tes.c
> > new file mode 100644
> > index 000000000..61f3f6fd5
> > --- /dev/null
> > +++
> b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_i
> > +++ n_bytes.c
> > @@ -0,0 +1,87 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2021 HUAWEI LIMITED
> > + * Author: Zhao Gongyi <zhaogongyi@huawei.com>  */
> > +
> > +/*\
> > + * [Description]
> > + * Check that kmem overflow is controlled by kmem.limit_in_bytes.
> > + *
> > + * [Algorithm]
> > + * 1) mount memory cgroup
> > + * 2) set 0 to memory.kmem.limit_in_bytes
> > + * 3) set test process group to cgroup.procs
> > + * 4) fork in test process group to trig kmem overflow  */
> > +
> > +#include <sys/wait.h>
> > +#include "tst_test.h"
> > +
> > +#define MNT_POINT "memcg"
> > +#define TESTDIR "memcg/ltpmemcg"
> > +#define CGROUP_PROCS "memcg/ltpmemcg/cgroup.procs"
> > +#define KMEM_LIMIT_IN_BYTES
> "memcg/ltpmemcg/memory.kmem.limit_in_bytes"
> > +
> > +static void cleanup(void)
> > +{
> > +	SAFE_RMDIR(TESTDIR);
> > +	SAFE_UMOUNT(MNT_POINT);
> > +	SAFE_RMDIR(MNT_POINT);
> > +}
> > +
> > +static void setup(void)
> > +{
> > +	SAFE_MKDIR(MNT_POINT, 0755);
> > +	SAFE_MOUNT("memcg", MNT_POINT, "cgroup", 0, "memory");
> > +	SAFE_MKDIR(TESTDIR, 0755);
> > +}
> > +
> > +static void run(void)
> > +{
> > +	pid_t pid;
> > +	int st;
> > +
> > +	pid = SAFE_FORK();
> > +	if (!pid) {
> > +		SAFE_SETPGID(0, 0);
> > +
> > +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
> > +
> > +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
> 
> After setting this we should avoid doing any work in this process except
> calling fork. AFAIK there is no guarantee that some other syscall or tasklet
> etc. Won't try to allocate memory and also fail.
> 
> > +
> > +		TEST(fork());
> > +		if (TST_RET == -1) {
> > +			if (TST_ERR == ENOMEM)
> > +				tst_res(TPASS, "fork fail as expected");
> > +			else
> > +				tst_brk(TFAIL | TTERRNO,
> > +					"fork fail as unexpected");
> 
> This should be "fork failed with unexpected error".
> 
> > +		} else if (TST_RET == 0) {
> > +			tst_brk(TFAIL, "fork success as unexpected");
> > +		} else {
> > +			SAFE_WAIT(NULL);
> > +			tst_brk(TFAIL, "fork success as unexpected");
> > +		}
> > +	} else {
> > +		SAFE_WAIT(&st);
> > +		if (WIFEXITED(st))
> > +			if (!WEXITSTATUS(st)) {
> > +				tst_res(TPASS, "kmem alloc is controled "
> > +					"by memory.kmem.limit_in_bytes");
> > +				return;
> > +			}
> > +
> > +		tst_res(TFAIL,"kmem alloc is not controled "
> > +			"by memory.kmem.limit_in_bytes");
> > +	}
> > +}
> > +
> > +static struct tst_test test = {
> > +	.needs_root = 1,
> > +	.needs_tmpdir = 1,
> > +	.forks_child = 1,
> > +	.setup = setup,
> > +	.test_all = run,
> > +	.cleanup = cleanup,
> > +};
> > --
> > 2.17.1
> 
> 
> --
> Thank you,
> Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Richard Palethorpe]

Hello,

Zhao Gongyi <zhaogongyi@huawei.com> writes:

> Add memory cgroup testcase for checking that kmem overflow is controlled
> by kmem.limit_in_bytes.

I am currently working on a new LTP CGroups API, please see:
https://patchwork.ozlabs.org/project/ltp/list/?series=238773

If the new API is merged quickly, then this test will need to be
converted to it. Also please see comments below.

>
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
> v1->v2:
> 	1) add testcase to runtest/controllers
> 	2) add testcase to testcases/kernel/controllers/memcg/.gitignore
> ---
>  runtest/controllers                           |  1 +
>  testcases/kernel/controllers/memcg/.gitignore |  1 +
>  .../functional/memcg_kmem_limit_in_bytes.c    | 87 +++++++++++++++++++
>  3 files changed, 89 insertions(+)
>  create mode 100644 testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c
>
> diff --git a/runtest/controllers b/runtest/controllers
> index e3d0243f1..f13a112c7 100644
> --- a/runtest/controllers
> +++ b/runtest/controllers
> @@ -15,6 +15,7 @@ memcg_use_hierarchy	memcg_use_hierarchy_test.sh
>  memcg_usage_in_bytes	memcg_usage_in_bytes_test.sh
>  memcg_stress		memcg_stress_test.sh
>  memcg_control		memcg_control_test.sh
> +memcg_kmem_limit_in_bytes memcg_kmem_limit_in_bytes
>
>  cgroup_fj_function_debug cgroup_fj_function.sh debug
>  cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset
> diff --git a/testcases/kernel/controllers/memcg/.gitignore b/testcases/kernel/controllers/memcg/.gitignore
> index c0b6d0714..dce8412de 100644
> --- a/testcases/kernel/controllers/memcg/.gitignore
> +++ b/testcases/kernel/controllers/memcg/.gitignore
> @@ -1,5 +1,6 @@
>  /control/mem_process
>  /functional/memcg_process
> +/functional/memcg_kmem_limit_in_bytes
>  /regression/memcg_test_1
>  /regression/memcg_test_2
>  /regression/memcg_test_3
> diff --git a/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c
> new file mode 100644
> index 000000000..61f3f6fd5
> --- /dev/null
> +++ b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 HUAWEI LIMITED
> + * Author: Zhao Gongyi <zhaogongyi@huawei.com>
> + */
> +
> +/*\
> + * [Description]
> + * Check that kmem overflow is controlled by kmem.limit_in_bytes.
> + *
> + * [Algorithm]
> + * 1) mount memory cgroup
> + * 2) set 0 to memory.kmem.limit_in_bytes
> + * 3) set test process group to cgroup.procs
> + * 4) fork in test process group to trig kmem overflow
> + */
> +
> +#include <sys/wait.h>
> +#include "tst_test.h"
> +
> +#define MNT_POINT "memcg"
> +#define TESTDIR "memcg/ltpmemcg"
> +#define CGROUP_PROCS "memcg/ltpmemcg/cgroup.procs"
> +#define KMEM_LIMIT_IN_BYTES "memcg/ltpmemcg/memory.kmem.limit_in_bytes"
> +
> +static void cleanup(void)
> +{
> +	SAFE_RMDIR(TESTDIR);
> +	SAFE_UMOUNT(MNT_POINT);
> +	SAFE_RMDIR(MNT_POINT);
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_MKDIR(MNT_POINT, 0755);
> +	SAFE_MOUNT("memcg", MNT_POINT, "cgroup", 0, "memory");
> +	SAFE_MKDIR(TESTDIR, 0755);
> +}
> +
> +static void run(void)
> +{
> +	pid_t pid;
> +	int st;
> +
> +	pid = SAFE_FORK();
> +	if (!pid) {
> +		SAFE_SETPGID(0, 0);
> +
> +		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
> +
> +		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));

After setting this we should avoid doing any work in this process except
calling fork. AFAIK there is no guarantee that some other syscall or
tasklet etc. Won't try to allocate memory and also fail.

> +
> +		TEST(fork());
> +		if (TST_RET == -1) {
> +			if (TST_ERR == ENOMEM)
> +				tst_res(TPASS, "fork fail as expected");
> +			else
> +				tst_brk(TFAIL | TTERRNO,
> +					"fork fail as unexpected");

This should be "fork failed with unexpected error".

> +		} else if (TST_RET == 0) {
> +			tst_brk(TFAIL, "fork success as unexpected");
> +		} else {
> +			SAFE_WAIT(NULL);
> +			tst_brk(TFAIL, "fork success as unexpected");
> +		}
> +	} else {
> +		SAFE_WAIT(&st);
> +		if (WIFEXITED(st))
> +			if (!WEXITSTATUS(st)) {
> +				tst_res(TPASS, "kmem alloc is controled "
> +					"by memory.kmem.limit_in_bytes");
> +				return;
> +			}
> +
> +		tst_res(TFAIL,"kmem alloc is not controled "
> +			"by memory.kmem.limit_in_bytes");
> +	}
> +}
> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +	.setup = setup,
> +	.test_all = run,
> +	.cleanup = cleanup,
> +};
> --
> 2.17.1


-- 
Thank you,
Richard.

[LTP] [PATCH v2] controllers/memcg: Add testcase for kmem_limit_in_bytes of memory cgroup

[Zhao Gongyi]

Add memory cgroup testcase for checking that kmem overflow is controlled
by kmem.limit_in_bytes.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
v1->v2:
	1) add testcase to runtest/controllers
	2) add testcase to testcases/kernel/controllers/memcg/.gitignore
---
 runtest/controllers                           |  1 +
 testcases/kernel/controllers/memcg/.gitignore |  1 +
 .../functional/memcg_kmem_limit_in_bytes.c    | 87 +++++++++++++++++++
 3 files changed, 89 insertions(+)
 create mode 100644 testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c

diff --git a/runtest/controllers b/runtest/controllers
index e3d0243f1..f13a112c7 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -15,6 +15,7 @@ memcg_use_hierarchy	memcg_use_hierarchy_test.sh
 memcg_usage_in_bytes	memcg_usage_in_bytes_test.sh
 memcg_stress		memcg_stress_test.sh
 memcg_control		memcg_control_test.sh
+memcg_kmem_limit_in_bytes memcg_kmem_limit_in_bytes

 cgroup_fj_function_debug cgroup_fj_function.sh debug
 cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset
diff --git a/testcases/kernel/controllers/memcg/.gitignore b/testcases/kernel/controllers/memcg/.gitignore
index c0b6d0714..dce8412de 100644
--- a/testcases/kernel/controllers/memcg/.gitignore
+++ b/testcases/kernel/controllers/memcg/.gitignore
@@ -1,5 +1,6 @@
 /control/mem_process
 /functional/memcg_process
+/functional/memcg_kmem_limit_in_bytes
 /regression/memcg_test_1
 /regression/memcg_test_2
 /regression/memcg_test_3
diff --git a/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c
new file mode 100644
index 000000000..61f3f6fd5
--- /dev/null
+++ b/testcases/kernel/controllers/memcg/functional/memcg_kmem_limit_in_bytes.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 HUAWEI LIMITED
+ * Author: Zhao Gongyi <zhaogongyi@huawei.com>
+ */
+
+/*\
+ * [Description]
+ * Check that kmem overflow is controlled by kmem.limit_in_bytes.
+ *
+ * [Algorithm]
+ * 1) mount memory cgroup
+ * 2) set 0 to memory.kmem.limit_in_bytes
+ * 3) set test process group to cgroup.procs
+ * 4) fork in test process group to trig kmem overflow
+ */
+
+#include <sys/wait.h>
+#include "tst_test.h"
+
+#define MNT_POINT "memcg"
+#define TESTDIR "memcg/ltpmemcg"
+#define CGROUP_PROCS "memcg/ltpmemcg/cgroup.procs"
+#define KMEM_LIMIT_IN_BYTES "memcg/ltpmemcg/memory.kmem.limit_in_bytes"
+
+static void cleanup(void)
+{
+	SAFE_RMDIR(TESTDIR);
+	SAFE_UMOUNT(MNT_POINT);
+	SAFE_RMDIR(MNT_POINT);
+}
+
+static void setup(void)
+{
+	SAFE_MKDIR(MNT_POINT, 0755);
+	SAFE_MOUNT("memcg", MNT_POINT, "cgroup", 0, "memory");
+	SAFE_MKDIR(TESTDIR, 0755);
+}
+
+static void run(void)
+{
+	pid_t pid;
+	int st;
+
+	pid = SAFE_FORK();
+	if (!pid) {
+		SAFE_SETPGID(0, 0);
+
+		SAFE_FILE_PRINTF(KMEM_LIMIT_IN_BYTES, "%d", 0);
+
+		SAFE_FILE_PRINTF(CGROUP_PROCS, "%d", getpgid(getpid()));
+
+		TEST(fork());
+		if (TST_RET == -1) {
+			if (TST_ERR == ENOMEM)
+				tst_res(TPASS, "fork fail as expected");
+			else
+				tst_brk(TFAIL | TTERRNO,
+					"fork fail as unexpected");
+		} else if (TST_RET == 0) {
+			tst_brk(TFAIL, "fork success as unexpected");
+		} else {
+			SAFE_WAIT(NULL);
+			tst_brk(TFAIL, "fork success as unexpected");
+		}
+	} else {
+		SAFE_WAIT(&st);
+		if (WIFEXITED(st))
+			if (!WEXITSTATUS(st)) {
+				tst_res(TPASS, "kmem alloc is controled "
+					"by memory.kmem.limit_in_bytes");
+				return;
+			}
+
+		tst_res(TFAIL,"kmem alloc is not controled "
+			"by memory.kmem.limit_in_bytes");
+	}
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.test_all = run,
+	.cleanup = cleanup,
+};
--
2.17.1