| Message ID | 20210614161413.82389-1-krzysztof.kozlowski@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] controllers/cpuacct: skip cpuacct_100_100 on small memory systems | expand |
Hi! > +check_free_memory() > +{ > + local memneeded > + local memfree=`awk '/MemAvailable/ {print $2}' /proc/meminfo` > + > + if [ $? -ne 0 ]; then > + local memcached > + > + memfree=`awk '/MemFree/ {print $2}' /proc/meminfo` > + test $? || return 0 > + > + memcached=`awk '/MemCached/ {print $2}' /proc/meminfo` > + test $? || return 0 I do not think that something as basic as awk on /proc/meminfo here will fail... > + memfree=$((memfree + memcached)) > + fi > + > + # On x86_64, each 100 of processes were using ~16 MB of memory, > + # so try to estimate the needed free memory based on this. > + memneeded=$((max * nbprocess * 16384 / 100)) > + > + if [ $memfree -lt $memneeded ]; then I would still add some memory margin to the memneeded here. At least add a hundred of megabytes before we do the check. > + tst_brk TCONF "not enough of free memory on this system (approximate need $memneeded kB, free $memfree kB)" > + fi > + tst_res TINFO "memory requirements fulfilled (approximate need $memneeded kB, free $memfree kB)" > + > + return 0 > +} > + > +check_limits() > +{ > + local realuid="$SUDO_UID" > + local tasksneeded=$((max * nbprocess + 100)) > + > + if [ "$realuid" = "" ]; then > + realuid=`id -u` > + test $? || return 0 > + fi > + > + local tasksmax=`systemctl show user-${real_uid}.slice | awk -F '=' '/TasksMax/ {print $2}'` > + test $? || return 0 > + > + if [ $tasksmax -le $tasksneeded ]; then > + tst_brk TCONF "limit of tasks is too low (approximate need $tasksneeded, limit $tasksmax)" > + fi > + tst_res TINFO "task limit fulfilled (approximate need $tasksneeded, limit $tasksmax)" Huh, is this really needed? The test is supposed to run under root. The user is supposed to login as a root or at least do 'su -' before executing LTP anyways. > + return 0 > +} > + > setup() > { > if ! grep -q -w cpuacct /proc/cgroups; then > tst_brk TCONF "cpuacct not supported on this system" > fi > > + check_limits > + # Don't bother with memory limit checks on smaller tests > + if [ $max -ge 100 ] && [ $nbprocess -ge 100 ]; then We should probably check if the $max * $mbprocess -ge 1000 or something like that just in a case someone addds a test with large enough $nbprocess. > + check_free_memory > + fi > + > mount_point=`grep -w cpuacct /proc/mounts | cut -f 2 | cut -d " " -f2` > tst_res TINFO "cpuacct: $mount_point" > if [ "$mount_point" = "" ]; then > -- > 2.27.0 >
On 15/06/2021 18:40, Cyril Hrubis wrote: > Hi! >> +check_free_memory() >> +{ >> + local memneeded >> + local memfree=`awk '/MemAvailable/ {print $2}' /proc/meminfo` >> + >> + if [ $? -ne 0 ]; then >> + local memcached >> + >> + memfree=`awk '/MemFree/ {print $2}' /proc/meminfo` >> + test $? || return 0 >> + >> + memcached=`awk '/MemCached/ {print $2}' /proc/meminfo` >> + test $? || return 0 > > I do not think that something as basic as awk on /proc/meminfo here will > fail... It's still nice pattern to check for return values but if you insist, I can drop it. > >> + memfree=$((memfree + memcached)) >> + fi >> + >> + # On x86_64, each 100 of processes were using ~16 MB of memory, >> + # so try to estimate the needed free memory based on this. >> + memneeded=$((max * nbprocess * 16384 / 100)) >> + >> + if [ $memfree -lt $memneeded ]; then > > I would still add some memory margin to the memneeded here. At least > add a hundred of megabytes before we do the check. Sure. > >> + tst_brk TCONF "not enough of free memory on this system (approximate need $memneeded kB, free $memfree kB)" >> + fi >> + tst_res TINFO "memory requirements fulfilled (approximate need $memneeded kB, free $memfree kB)" >> + >> + return 0 >> +} >> + >> +check_limits() >> +{ >> + local realuid="$SUDO_UID" >> + local tasksneeded=$((max * nbprocess + 100)) >> + >> + if [ "$realuid" = "" ]; then >> + realuid=`id -u` >> + test $? || return 0 >> + fi >> + >> + local tasksmax=`systemctl show user-${real_uid}.slice | awk -F '=' '/TasksMax/ {print $2}'` >> + test $? || return 0 >> + >> + if [ $tasksmax -le $tasksneeded ]; then >> + tst_brk TCONF "limit of tasks is too low (approximate need $tasksneeded, limit $tasksmax)" >> + fi >> + tst_res TINFO "task limit fulfilled (approximate need $tasksneeded, limit $tasksmax)" > > Huh, is this really needed? The test is supposed to run under root. The > user is supposed to login as a root or at least do 'su -' before > executing LTP anyways. Yeah, because even root sessions (user-0.slice) have the PID limit set by systemd. $ ssh root@.... $ grep . /sys/fs/cgroup/pids/user.slice/*/* /sys/fs/cgroup/pids/user.slice/user-0.slice/cgroup.clone_children:0 /sys/fs/cgroup/pids/user.slice/user-0.slice/notify_on_release:0 /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.current:5 /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.events:max 0 /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207 > >> + return 0 >> +} >> + >> setup() >> { >> if ! grep -q -w cpuacct /proc/cgroups; then >> tst_brk TCONF "cpuacct not supported on this system" >> fi >> >> + check_limits >> + # Don't bother with memory limit checks on smaller tests >> + if [ $max -ge 100 ] && [ $nbprocess -ge 100 ]; then > > We should probably check if the $max * $mbprocess -ge 1000 or something > like that just in a case someone addds a test with large enough > $nbprocess. Makes sense, optionally the check can be done always. Thanks for the review! Best regards, Krzysztof
On 16/06/2021 09:27, Krzysztof Kozlowski wrote: > On 15/06/2021 18:40, Cyril Hrubis wrote: >> Hi! >>> +check_free_memory() >>> +{ >>> + local memneeded >>> + local memfree=`awk '/MemAvailable/ {print $2}' /proc/meminfo` >>> + >>> + if [ $? -ne 0 ]; then >>> + local memcached >>> + >>> + memfree=`awk '/MemFree/ {print $2}' /proc/meminfo` >>> + test $? || return 0 >>> + >>> + memcached=`awk '/MemCached/ {print $2}' /proc/meminfo` >>> + test $? || return 0 >> >> I do not think that something as basic as awk on /proc/meminfo here will >> fail... > > It's still nice pattern to check for return values but if you insist, I > can drop it. > >> >>> + memfree=$((memfree + memcached)) >>> + fi >>> + >>> + # On x86_64, each 100 of processes were using ~16 MB of memory, >>> + # so try to estimate the needed free memory based on this. >>> + memneeded=$((max * nbprocess * 16384 / 100)) >>> + >>> + if [ $memfree -lt $memneeded ]; then >> >> I would still add some memory margin to the memneeded here. At least >> add a hundred of megabytes before we do the check. > > Sure. > >> >>> + tst_brk TCONF "not enough of free memory on this system (approximate need $memneeded kB, free $memfree kB)" >>> + fi >>> + tst_res TINFO "memory requirements fulfilled (approximate need $memneeded kB, free $memfree kB)" >>> + >>> + return 0 >>> +} >>> + >>> +check_limits() >>> +{ >>> + local realuid="$SUDO_UID" >>> + local tasksneeded=$((max * nbprocess + 100)) >>> + >>> + if [ "$realuid" = "" ]; then >>> + realuid=`id -u` >>> + test $? || return 0 >>> + fi >>> + >>> + local tasksmax=`systemctl show user-${real_uid}.slice | awk -F '=' '/TasksMax/ {print $2}'` >>> + test $? || return 0 >>> + >>> + if [ $tasksmax -le $tasksneeded ]; then >>> + tst_brk TCONF "limit of tasks is too low (approximate need $tasksneeded, limit $tasksmax)" >>> + fi >>> + tst_res TINFO "task limit fulfilled (approximate need $tasksneeded, limit $tasksmax)" >> >> Huh, is this really needed? The test is supposed to run under root. The >> user is supposed to login as a root or at least do 'su -' before >> executing LTP anyways. > > Yeah, because even root sessions (user-0.slice) have the PID limit set > by systemd. > > $ ssh root@.... > $ grep . /sys/fs/cgroup/pids/user.slice/*/* > /sys/fs/cgroup/pids/user.slice/user-0.slice/cgroup.clone_children:0 > /sys/fs/cgroup/pids/user.slice/user-0.slice/notify_on_release:0 > /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.current:5 > /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.events:max 0 > /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207 I forgot to mention: that's a default Ubuntu 20.04 LTS system on 2 GB RAM machine, with systemd 245.4-4ubuntu3.6. Best regards, Krzysztof
On 16/06/2021 09:28, Krzysztof Kozlowski wrote: > On 16/06/2021 09:27, Krzysztof Kozlowski wrote: >> On 15/06/2021 18:40, Cyril Hrubis wrote: >>> Hi! >>>> +check_free_memory() >>>> +{ >>>> + local memneeded >>>> + local memfree=`awk '/MemAvailable/ {print $2}' /proc/meminfo` >>>> + >>>> + if [ $? -ne 0 ]; then >>>> + local memcached >>>> + >>>> + memfree=`awk '/MemFree/ {print $2}' /proc/meminfo` >>>> + test $? || return 0 >>>> + >>>> + memcached=`awk '/MemCached/ {print $2}' /proc/meminfo` >>>> + test $? || return 0 >>> >>> I do not think that something as basic as awk on /proc/meminfo here will >>> fail... >> >> It's still nice pattern to check for return values but if you insist, I >> can drop it. >> >>> >>>> + memfree=$((memfree + memcached)) >>>> + fi >>>> + >>>> + # On x86_64, each 100 of processes were using ~16 MB of memory, >>>> + # so try to estimate the needed free memory based on this. >>>> + memneeded=$((max * nbprocess * 16384 / 100)) >>>> + >>>> + if [ $memfree -lt $memneeded ]; then >>> >>> I would still add some memory margin to the memneeded here. At least >>> add a hundred of megabytes before we do the check. >> >> Sure. >> >>> >>>> + tst_brk TCONF "not enough of free memory on this system (approximate need $memneeded kB, free $memfree kB)" >>>> + fi >>>> + tst_res TINFO "memory requirements fulfilled (approximate need $memneeded kB, free $memfree kB)" >>>> + >>>> + return 0 >>>> +} >>>> + >>>> +check_limits() >>>> +{ >>>> + local realuid="$SUDO_UID" >>>> + local tasksneeded=$((max * nbprocess + 100)) >>>> + >>>> + if [ "$realuid" = "" ]; then >>>> + realuid=`id -u` >>>> + test $? || return 0 >>>> + fi >>>> + >>>> + local tasksmax=`systemctl show user-${real_uid}.slice | awk -F '=' '/TasksMax/ {print $2}'` >>>> + test $? || return 0 >>>> + >>>> + if [ $tasksmax -le $tasksneeded ]; then >>>> + tst_brk TCONF "limit of tasks is too low (approximate need $tasksneeded, limit $tasksmax)" >>>> + fi >>>> + tst_res TINFO "task limit fulfilled (approximate need $tasksneeded, limit $tasksmax)" >>> >>> Huh, is this really needed? The test is supposed to run under root. The >>> user is supposed to login as a root or at least do 'su -' before >>> executing LTP anyways. >> >> Yeah, because even root sessions (user-0.slice) have the PID limit set >> by systemd. >> >> $ ssh root@.... >> $ grep . /sys/fs/cgroup/pids/user.slice/*/* >> /sys/fs/cgroup/pids/user.slice/user-0.slice/cgroup.clone_children:0 >> /sys/fs/cgroup/pids/user.slice/user-0.slice/notify_on_release:0 >> /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.current:5 >> /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.events:max 0 >> /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207 > > I forgot to mention: that's a default Ubuntu 20.04 LTS system on 2 GB > RAM machine, with systemd 245.4-4ubuntu3.6. Any further comments here or something to fix? I think there were no objections to this patch, right? Best regards, Krzysztof
diff --git a/testcases/kernel/controllers/cpuacct/cpuacct.sh b/testcases/kernel/controllers/cpuacct/cpuacct.sh index 323aa7513bf4..66a48dde679b 100755 --- a/testcases/kernel/controllers/cpuacct/cpuacct.sh +++ b/testcases/kernel/controllers/cpuacct/cpuacct.sh @@ -38,12 +38,68 @@ OPTIONS EOF } +check_free_memory() +{ + local memneeded + local memfree=`awk '/MemAvailable/ {print $2}' /proc/meminfo` + + if [ $? -ne 0 ]; then + local memcached + + memfree=`awk '/MemFree/ {print $2}' /proc/meminfo` + test $? || return 0 + + memcached=`awk '/MemCached/ {print $2}' /proc/meminfo` + test $? || return 0 + + memfree=$((memfree + memcached)) + fi + + # On x86_64, each 100 of processes were using ~16 MB of memory, + # so try to estimate the needed free memory based on this. + memneeded=$((max * nbprocess * 16384 / 100)) + + if [ $memfree -lt $memneeded ]; then + tst_brk TCONF "not enough of free memory on this system (approximate need $memneeded kB, free $memfree kB)" + fi + tst_res TINFO "memory requirements fulfilled (approximate need $memneeded kB, free $memfree kB)" + + return 0 +} + +check_limits() +{ + local realuid="$SUDO_UID" + local tasksneeded=$((max * nbprocess + 100)) + + if [ "$realuid" = "" ]; then + realuid=`id -u` + test $? || return 0 + fi + + local tasksmax=`systemctl show user-${real_uid}.slice | awk -F '=' '/TasksMax/ {print $2}'` + test $? || return 0 + + if [ $tasksmax -le $tasksneeded ]; then + tst_brk TCONF "limit of tasks is too low (approximate need $tasksneeded, limit $tasksmax)" + fi + tst_res TINFO "task limit fulfilled (approximate need $tasksneeded, limit $tasksmax)" + + return 0 +} + setup() { if ! grep -q -w cpuacct /proc/cgroups; then tst_brk TCONF "cpuacct not supported on this system" fi + check_limits + # Don't bother with memory limit checks on smaller tests + if [ $max -ge 100 ] && [ $nbprocess -ge 100 ]; then + check_free_memory + fi + mount_point=`grep -w cpuacct /proc/mounts | cut -f 2 | cut -d " " -f2` tst_res TINFO "cpuacct: $mount_point" if [ "$mount_point" = "" ]; then
Running cpuacct_100_100 on a system with less than ~4 GB of RAM fails: cpuacct 1 TINFO: timeout per run is 0h 5m 0s cpuacct 1 TINFO: cpuacct: /sys/fs/cgroup/cpu,cpuacct cpuacct 1 TINFO: Creating 100 subgroups each with 100 processes testcases/bin/cpuacct.sh: 0: Cannot fork In dmesg: LTP: starting cpuacct_100_100 (cpuacct.sh 100 100) cgroup: fork rejected by pids controller in /user.slice/user-1000.slice/session-1.scope The reason is cgroups pid limit set by systemd user.slice. For example on 2 GB RAM machine it is set as: /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207 Add a check for both free memory and maximum number of pids (when using systemd) to skip the test. Systems not using systemd might still fail. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> --- Changes since v1: 1. Add checking for pids. 2. Accurately check the memory constraints. --- .../kernel/controllers/cpuacct/cpuacct.sh | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+)