| Message ID | 3b78e25c8eba247d0ce2d99cbbdcaeba7994e26c.1642601554.git.luke.nowakowskikrijger@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Expand Cgroup lib and modify controller tests | expand |
Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote: > +test1() > { > - TST_COUNT=1 > - tst_resm TINFO "Test #1: Checking if the memory usage limit imposed by the topmost group is enforced" > + cd $TST_TMPDIR > + > + tst_res TINFO "Test #1: Checking if the memory usage limit imposed by the topmost group is enforced" > > - echo "$ACTIVE_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.limit_in_bytes > - echo "$TOT_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes > + ROD echo "$ACTIVE_MEM_LIMIT" > "$test_dir/$memory_limit" > + ROD echo "$TOT_MEM_LIMIT" > "$test_dir/$memsw_memory_limit" ... > } > +setup() > { > - RES=$1 > - INFO=$2 > - > - if [ $RES -eq $PASS ]; then > - tst_resm TPASS "$INFO" > + cgroup_require "memory" > + cgroup_v=$(cgroup_get_version "memory") > + test_dir=$(cgroup_get_test_path "memory") > + task_list=$(cgroup_get_task_list "memory") > + > + if [ "$cgroup_v" = "V2" ]; then > + memory_limit="memory.max" > + memsw_memory_limit="memory.swap.max" As we already built the controller files mapping from V2 to V1 in C library and you actually add many new (in patch 5/16). I'm thinking maybe we could make use of it in tst_cgctl.c to avoid handling these (in shell) separately. Something like: # ./tst_cgctl set "$pid" "cgroup.procs" "$target_pid" # ./tst_cgctl set "$pid" "memory.max" "$ACTIVE_MEM_LIMIT" Otherwise, it seems to make no sense to add so many new files mapping (like that patch 5/16) at this moment. What do you think? > else > - : $((FAILED_CNT += 1)) > - tst_resm TFAIL "$INFO" > + memory_limit="memory.limit_in_bytes" > + memsw_memory_limit="memory.memsw.limit_in_bytes" > fi > -}
Hello Li, Li Wang <liwang@redhat.com> writes: > Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote: > >> +test1() >> { >> - TST_COUNT=1 >> - tst_resm TINFO "Test #1: Checking if the memory usage limit imposed by the topmost group is enforced" >> + cd $TST_TMPDIR >> + >> + tst_res TINFO "Test #1: Checking if the memory usage limit imposed by the topmost group is enforced" >> >> - echo "$ACTIVE_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.limit_in_bytes >> - echo "$TOT_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes >> + ROD echo "$ACTIVE_MEM_LIMIT" > "$test_dir/$memory_limit" >> + ROD echo "$TOT_MEM_LIMIT" > "$test_dir/$memsw_memory_limit" > ... >> } > >> +setup() >> { >> - RES=$1 >> - INFO=$2 >> - >> - if [ $RES -eq $PASS ]; then >> - tst_resm TPASS "$INFO" >> + cgroup_require "memory" >> + cgroup_v=$(cgroup_get_version "memory") >> + test_dir=$(cgroup_get_test_path "memory") >> + task_list=$(cgroup_get_task_list "memory") >> + >> + if [ "$cgroup_v" = "V2" ]; then >> + memory_limit="memory.max" >> + memsw_memory_limit="memory.swap.max" > > As we already built the controller files mapping from V2 to V1 > in C library and you actually add many new (in patch 5/16). > > I'm thinking maybe we could make use of it in tst_cgctl.c to > avoid handling these (in shell) separately. > > Something like: > > # ./tst_cgctl set "$pid" "cgroup.procs" "$target_pid" > # ./tst_cgctl set "$pid" "memory.max" "$ACTIVE_MEM_LIMIT" > > Otherwise, it seems to make no sense to add so many new > files mapping (like that patch 5/16) at this moment. > > What do you think? I think it looks nice! > > >> else >> - : $((FAILED_CNT += 1)) >> - tst_resm TFAIL "$INFO" >> + memory_limit="memory.limit_in_bytes" >> + memsw_memory_limit="memory.memsw.limit_in_bytes" >> fi >> -}
Hi Li, On Mon, Jan 24, 2022 at 1:44 AM Li Wang <liwang@redhat.com> wrote: > Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote: > > As we already built the controller files mapping from V2 to V1 > in C library and you actually add many new (in patch 5/16). > > I'm thinking maybe we could make use of it in tst_cgctl.c to > avoid handling these (in shell) separately. > > Something like: > > # ./tst_cgctl set "$pid" "cgroup.procs" "$target_pid" > # ./tst_cgctl set "$pid" "memory.max" "$ACTIVE_MEM_LIMIT" > > Otherwise, it seems to make no sense to add so many new > files mapping (like that patch 5/16) at this moment. > > What do you think? > > > I think this would be nice except that we would need to keep track of the tst_cg_cgroup structs if we wanted to use safe_cg_* functions in the C lib. This would be fine if we only wanted to use control files in the test_dir but it gets more complicated if there are other directories below it that we would want to set. At least as far as I understand it. And as Richard mentioned its probably a better idea to just only add the control files for controllers as we absolutely need them so this wouldn't be too useful. Plus I think it's easy enough from shell to do a version check and write to the right control file/directory directly. So I personally don't think its as important, but I could see in the future implementing something like this so it mimics the C api. What do you think? > -- > Regards, > Li Wang > > Apologies for the hiatus, I know it's not easy to get back into review mode for patches you haven't thought about in a while. Thanks, - Luke
Hi Luke, Thanks for looking back and working on this. On Thu, Mar 3, 2022 at 5:38 AM Luke Nowakowski-Krijger < luke.nowakowskikrijger@canonical.com> wrote: > Hi Li, > > On Mon, Jan 24, 2022 at 1:44 AM Li Wang <liwang@redhat.com> wrote: > >> Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote: >> >> As we already built the controller files mapping from V2 to V1 >> in C library and you actually add many new (in patch 5/16). >> >> I'm thinking maybe we could make use of it in tst_cgctl.c to >> avoid handling these (in shell) separately. >> >> Something like: >> >> # ./tst_cgctl set "$pid" "cgroup.procs" "$target_pid" >> # ./tst_cgctl set "$pid" "memory.max" "$ACTIVE_MEM_LIMIT" >> >> Otherwise, it seems to make no sense to add so many new >> files mapping (like that patch 5/16) at this moment. >> >> What do you think? >> >> >> I think this would be nice except that we would need to keep track of the > tst_cg_cgroup structs if we wanted to use safe_cg_* functions in the C lib. > This would be fine if we only wanted to use control files in the test_dir > but it gets more complicated if there are other directories below it that > we would want to set. At least as far as I understand it. > Right, but so far it seems we don't have more (than two) sub-layer directories tests. (or maybe I didn't aware that we have) > > And as Richard mentioned its probably a better idea to just only add the > control files for controllers as we absolutely need them so this wouldn't > be too useful. Plus I think it's easy enough from shell to do a version > check and write to the right control file/directory directly. > > So I personally don't think its as important, but I could see in the > future implementing something like this so it mimics the C api. What do you > think? > Yes, it will be a little bit complex to achieve if we decide to encapsulate more details in tst_cgctl.c. But I just hope to provide a simple enough and intuitive way to use CGroup to LTP shell users. Giving more flexible to shell API also means giving more complexity to handle problem and easy to make mistakes. Anyway, I don't strongly insist on going like that, feel free to send out the next patch version as you wanted. I believe we will keep improving the API and tests later, or we can change to that if we find it is really neccesary.
diff --git a/testcases/kernel/controllers/memcg/control/mem_process.c b/testcases/kernel/controllers/memcg/control/mem_process.c index 6c1b36ca6..8ecabb272 100644 --- a/testcases/kernel/controllers/memcg/control/mem_process.c +++ b/testcases/kernel/controllers/memcg/control/mem_process.c @@ -1,28 +1,6 @@ -/*****************************************************************************/ -/* */ -/* Copyright (c) 2010 Mohamed Naufal Basheer */ -/* */ -/* This program is free software; you can redistribute it and/or modify */ -/* it under the terms of the GNU General Public License as published by */ -/* the Free Software Foundation; either version 2 of the License, or */ -/* (at your option) any later version. */ -/* */ -/* This program is distributed in the hope that it will be useful, */ -/* but WITHOUT ANY WARRANTY; without even the implied warranty of */ -/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See */ -/* the GNU General Public License for more details. */ -/* */ -/* You should have received a copy of the GNU General Public License */ -/* along with this program; if not, write to the Free Software */ -/* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -/* */ -/* File: mem_process.c */ -/* */ -/* Purpose: act as a memory hog for the memcg_control tests */ -/* */ -/* Author: Mohamed Naufal Basheer <naufal11@gmail.com > */ -/* */ -/*****************************************************************************/ +// SPDX-License-Identifier: GPL-2.0-or-later +// Copyright (c) 2010 Mohamed Naufal Basheer +// Author: Mohamed Naufal Basheer #include <sys/types.h> #include <sys/mman.h> diff --git a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh index 4d9f1bb5d..66316a9f9 100644 --- a/testcases/kernel/controllers/memcg/control/memcg_control_test.sh +++ b/testcases/kernel/controllers/memcg/control/memcg_control_test.sh @@ -1,45 +1,16 @@ #!/bin/sh +# SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (c) 2010 Mohamed Naufal Basheer +# Author: Mohamed Naufal Basheer -################################################################################ -## ## -## Copyright (c) 2010 Mohamed Naufal Basheer ## -## ## -## This program is free software; you can redistribute it and/or modify ## -## it under the terms of the GNU General Public License as published by ## -## the Free Software Foundation; either version 2 of the License, or ## -## (at your option) any later version. ## -## ## -## This program is distributed in the hope that it will be useful, ## -## but WITHOUT ANY WARRANTY; without even the implied warranty of ## -## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See ## -## the GNU General Public License for more details. ## -## ## -## You should have received a copy of the GNU General Public License ## -## along with this program; if not, write to the Free Software ## -## Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA ## -## ## -## ## -## File: memcg_control_test.sh ## -## ## -## Purpose: Implement various memory controller tests ## -## ## -## Author: Mohamed Naufal Basheer <naufal11@gmail.com> ## -## ## -################################################################################ - -if [ "x$(grep -w memory /proc/cgroups | cut -f4)" != "x1" ]; then - echo "WARNING:" - echo "Either kernel does not support memory resource controller or feature not enabled" - echo "Skipping all memcg_control testcases...." - exit 0 -fi - -export TCID="memcg_control" -export TST_TOTAL=1 -export TST_COUNT=0 - -export TMP=${TMP:-/tmp} -cd $TMP +TST_TESTFUNC=test +TST_SETUP=setup +TST_CLEANUP=cleanup +TST_CNT=1 +TST_NEEDS_ROOT=1 +TST_NEEDS_TMPDIR=1 + +. cgroup_lib.sh PAGE_SIZE=$(tst_getconf PAGESIZE) @@ -47,20 +18,14 @@ TOT_MEM_LIMIT=$PAGE_SIZE ACTIVE_MEM_LIMIT=$PAGE_SIZE PROC_MEM=$((PAGE_SIZE * 2)) -TST_PATH=$PWD -STATUS_PIPE="$TMP/status_pipe" - -PASS=0 -FAIL=1 +STATUS_PIPE="status_pipe" # Check if the test process is killed on crossing boundary test_proc_kill() { - cd $TMP mem_process -m $PROC_MEM & - cd $OLDPWD sleep 1 - echo $! > tasks + ROD echo $! > "$test_dir/$task_list" #Instruct the test process to start acquiring memory echo m > $STATUS_PIPE @@ -77,87 +42,46 @@ test_proc_kill() } # Validate the memory usage limit imposed by the hierarchically topmost group -testcase_1() +test1() { - TST_COUNT=1 - tst_resm TINFO "Test #1: Checking if the memory usage limit imposed by the topmost group is enforced" + cd $TST_TMPDIR + + tst_res TINFO "Test #1: Checking if the memory usage limit imposed by the topmost group is enforced" - echo "$ACTIVE_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.limit_in_bytes - echo "$TOT_MEM_LIMIT" > $TST_PATH/mnt/$TST_NUM/memory.memsw.limit_in_bytes + ROD echo "$ACTIVE_MEM_LIMIT" > "$test_dir/$memory_limit" + ROD echo "$TOT_MEM_LIMIT" > "$test_dir/$memsw_memory_limit" - mkdir sub - (cd sub KILLED_CNT=0 test_proc_kill if [ $PROC_MEM -gt $TOT_MEM_LIMIT ] && [ $KILLED_CNT -eq 0 ]; then - result $FAIL "Test #1: failed" + tst_res TFAIL "Test #1: failed" else - result $PASS "Test #1: passed" - fi) - rmdir sub + tst_res TPASS "Test #1: passed" + fi } -# Record the test results -# -# $1: Result of the test case, $PASS or $FAIL -# $2: Output information -result() +setup() { - RES=$1 - INFO=$2 - - if [ $RES -eq $PASS ]; then - tst_resm TPASS "$INFO" + cgroup_require "memory" + cgroup_v=$(cgroup_get_version "memory") + test_dir=$(cgroup_get_test_path "memory") + task_list=$(cgroup_get_task_list "memory") + + if [ "$cgroup_v" = "V2" ]; then + memory_limit="memory.max" + memsw_memory_limit="memory.swap.max" else - : $((FAILED_CNT += 1)) - tst_resm TFAIL "$INFO" + memory_limit="memory.limit_in_bytes" + memsw_memory_limit="memory.memsw.limit_in_bytes" fi -} -cleanup() -{ - if [ -e $TST_PATH/mnt ]; then - umount $TST_PATH/mnt 2> /dev/null - rm -rf $TST_PATH/mnt - fi + tst_res TINFO "Test starts with cgroup $cgroup_v" } -do_mount() +cleanup() { - cleanup - - mkdir $TST_PATH/mnt - mount -t cgroup -o memory cgroup $TST_PATH/mnt 2> /dev/null - if [ $? -ne 0 ]; then - tst_brkm TBROK NULL "Mounting cgroup to temp dir failed" - rmdir $TST_PATH/mnt - exit 1 - fi + cgroup_cleanup } -do_mount - -echo 1 > mnt/memory.use_hierarchy 2> /dev/null - -FAILED_CNT=0 - -TST_NUM=1 -while [ $TST_NUM -le $TST_TOTAL ]; do - mkdir $TST_PATH/mnt/$TST_NUM - (cd $TST_PATH/mnt/$TST_NUM && testcase_$TST_NUM) - rmdir $TST_PATH/mnt/$TST_NUM - : $((TST_NUM += 1)) -done - -echo 0 > mnt/memory.use_hierarchy 2> /dev/null - -cleanup - -if [ "$FAILED_CNT" -ne 0 ]; then - tst_resm TFAIL "memcg_control: failed" - exit 1 -else - tst_resm TPASS "memcg_control: passed" - exit 0 -fi +tst_run
Update memcg_control_test to use the newer test lib and to use the newer cgroup lib which enables the memory v1 and v2 controller to be tested. Also updated to newer SPDX license identifier. Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> --- .../controllers/memcg/control/mem_process.c | 28 +--- .../memcg/control/memcg_control_test.sh | 150 +++++------------- 2 files changed, 40 insertions(+), 138 deletions(-)