[LTP] [PATCH 0/3] cpuset_regression_test: convert and improve

[LTP] [PATCH 0/3] cpuset_regression_test: convert and improve

[Joerg Vehlow]

Hi,

this is more or less a v2 of a patch I send previously (patch 3).
I know that richard is not entirely happy with this patch, I will
give it another try anyway...
It is either this patch or another patch, that has to look through
the cgroup hierarchy, to check if there is any group,that explicitely
uses cpu 0. 

To me, it is a better solution to just change groups for a short time,
and check if the bug exists. If ltp tests are running, the chance, that
there is anything running, that really needs a correct cpuset is very low.
I am comming from a system, where cgroups are setup by a container launcher,
that just happens to assign cpus to the containers - not even exclusively.
LTP tests are used as some part of the testsuite, to test as close to a
production system as possible.

The only way I could think of a process misbehaving by disabeling cpu pinning,
would be a badly written multithread application, that cannot correctly run,
if threads are really running in parallel, but this would also require a scheduling
policy, that makes scheduling points predicatable. While I know that software like
that exists (in fact I was working on something like that in the past), I think it
is highly unlikely, that it is running parallel to ltp.
And even then, this could be mitigated by not just setting cpu binding to undefined,
but to one fixed core. But with the changes in patch 2, this is not possible.

But anyhow ltp fiddles with lots of critical system parameters during it's runtime,
there is no guarantee, that an application that requires some very specific kernel
runtime settings survives this. That's why I would still vote for this patch.

J?rg

[LTP] [PATCH 1/3] cpuset_regression_test: Convert to new api

[Joerg Vehlow]

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 .../cpuset/cpuset_regression_test.sh          | 147 +++++++-----------
 1 file changed, 56 insertions(+), 91 deletions(-)

diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
index dccfd91cd..1dda19704 100755
--- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
+++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
@@ -1,82 +1,65 @@
 #!/bin/sh
-#
+# SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2015 Fujitsu Ltd.
 # Author: Zeng Linggang <zenglg.jy@cn.fujitsu.com>
 #
-# 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 St, Fifth Floor, Boston, MA  02110-1301  USA
-#
 # This is a regression test for commit:
-# http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/
-# ?id=bb2bc55
+# http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bb2bc55
 #
+# A newly created cpuset group crashed the kernel, if exclusive was set to 1,
+# before a cpuset was set.
+
+TST_SETUP=setup
+TST_CLEANUP=cleanup
+TST_TESTFUNC=test
+TST_NEEDS_ROOT=1
+TST_NEEDS_TMPDIR=1
+TST_MIN_KVER="3.18"
+
+. cgroup_lib.sh
+
+LOCAL_MOUNTPOINT="cpuset_test"
 
-TCID=cpuset_regression_test
-TST_TOTAL=1
-. test.sh
+root_cpuset_dir=
+cpu_exclusive="cpuset.cpu_exclusive"
+cpus="cpuset.cpus"
+old_cpu_exclusive_value=1
 
 setup()
 {
-	tst_require_root
+	local cpu_num
 
-	if tst_kvcmp -lt "3.18"; then
-		tst_brkm TCONF "Test must be run with kernel 3.18.0 or newer"
-	fi
-
-	local cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
+	cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
 	if [ $cpu_num -lt 2 ]; then
-		tst_brkm TCONF "We need 2 cpus at least to have test"
+		tst_brk TCONF "We need 2 cpus at least to have test"
 	fi
 
-	tst_tmpdir
-
-	TST_CLEANUP=cleanup
+	if ! is_cgroup_subsystem_available_and_enabled "cpuset"; then
+		tst_brk TCONF "Either kernel does not support cpuset controller or feature not enabled"
+	fi
 
 	# We need to mount cpuset if it is not found.
-	mount_flag=0
-	grep -w cpuset /proc/mounts > tmpfile
-	if [ $? -eq 0 ]; then
-		root_cpuset_dir=$(cat tmpfile | awk '{print $2}')
-	else
-		root_cpuset_dir="cpuset_test"
+	root_cpuset_dir=$(get_cgroup_mountpoint cpuset)
+	if [ -z "$root_cpuset_dir" ]; then
+		root_cpuset_dir="$LOCAL_MOUNTPOINT"
 
 		ROD_SILENT mkdir -p ${root_cpuset_dir}
-
 		ROD_SILENT mount -t cpuset cpuset ${root_cpuset_dir}
-
-		mount_flag=1
 	fi
 
-	if [ -f ${root_cpuset_dir}/cpuset.cpu_exclusive ]; then
-		cpu_exclusive=cpuset.cpu_exclusive
-		cpus=cpuset.cpus
-	elif [ -f ${root_cpuset_dir}/cpu_exclusive ]; then
+	if ! [ -f ${root_cpuset_dir}/${cpu_exclusive} ]; then
 		cpu_exclusive=cpu_exclusive
 		cpus=cpus
-	else
-		tst_brkm TBROK "Both cpuset.cpu_exclusive and cpu_exclusive" \
-			       "do not exist."
 	fi
 
-	cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
-	if [ "${cpu_exclusive_value}" != "1" ];then
+	if ! [ -f ${root_cpuset_dir}/${cpu_exclusive} ]; then
+		tst_brk TBROK "Both cpuset.cpu_exclusive and cpu_exclusive do not exist"
+	fi
+
+	old_cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
+	if [ "${old_cpu_exclusive_value}" != "1" ];then
 		echo 1 > ${root_cpuset_dir}/${cpu_exclusive}
-		if [ $? -ne 0 ]; then
-			tst_brkm TBROK "'echo 1 >" \
-				       "${root_cpuset_dir}/${cpu_exclusive}'" \
-				       "failed"
-		fi
+		[ $? -ne 0 ] && tst_brk TBROK "'echo 1 > ${root_cpuset_dir}/${cpu_exclusive}' failed"
 	fi
 }
 
@@ -86,65 +69,47 @@ cleanup()
 		rmdir ${root_cpuset_dir}/testdir
 	fi
 
-	if [ "$cpu_exclusive_value" != 1 ]; then
-		# Need to flush, or may be output:
-		# "write error: Device or resource busy"
+	if [ "$old_cpu_exclusive_value" != 1 ]; then
+		# Need to flush, or write may fail with: "Device or resource busy"
 		sync
-
-		echo ${cpu_exclusive_value} > \
-		     ${root_cpuset_dir}/${cpu_exclusive}
+		echo ${old_cpu_exclusive_value} > ${root_cpuset_dir}/${cpu_exclusive}
 	fi
 
-	if [ "${mount_flag}" = "1" ]; then
-		umount ${root_cpuset_dir}
+	if [ -d "$LOCAL_MOUNTPOINT" ]; then
+		umount ${LOCAL_MOUNTPOINT}
 		if [ $? -ne 0 ]; then
-			tst_resm TWARN "'umount ${root_cpuset_dir}' failed"
+			tst_res TWARN "'umount ${LOCAL_MOUNTPOINT}' failed"
 		fi
 
-		if [ -d "${root_cpuset_dir}" ]; then
-			rmdir ${root_cpuset_dir}
-		fi
+		rmdir ${LOCAL_MOUNTPOINT}
 	fi
-
-	tst_rmdir
 }
 
-cpuset_test()
+test()
 {
+	local cpu_exclusive_tmp cpus_value
+
 	ROD_SILENT mkdir ${root_cpuset_dir}/testdir
 
 	# Creat an exclusive cpuset.
 	echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}
-	if [ $? -ne 0 ]; then
-		tst_brkm TFAIL "'echo 1 >" \
-			       "${root_cpuset_dir}/testdir/${cpu_exclusive}'" \
-			       "failed"
-	fi
-
-	local cpu_exclusive_tmp=$(cat \
-				  ${root_cpuset_dir}/testdir/${cpu_exclusive})
+	[ $? -ne 0 ] && tst_brk TFAIL "'echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}' failed"
+	
+	cpu_exclusive_tmp=$(cat ${root_cpuset_dir}/testdir/${cpu_exclusive})
 	if [ "${cpu_exclusive_tmp}" != "1" ]; then
-		tst_brkm TFAIL "${cpu_exclusive} is '${cpu_exclusive_tmp}'," \
-			       "expected '1'"
+		tst_brk TFAIL "${cpu_exclusive} is '${cpu_exclusive_tmp}', expected '1'"
 	fi
 
-	# ${cpus} is empty at the begin, that maybe make the system *crash*.
+	# This may trigger the kernel crash
 	echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}
-	if [ $? -ne 0 ]; then
-		tst_brkm TFAIL "'echo 0-1 >" \
-			       "${root_cpuset_dir}/testdir/${cpus}' failed"
-	fi
+	[ $? -ne 0 ] && tst_brk TFAIL "'echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}' failed"
 
-	local cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus})
+	cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus})
 	if [ "${cpus_value}" != "0-1" ]; then
-		tst_brkm TFAIL "${cpus} is '${cpus_value}', expected '0-1'"
+		tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0-1'"
 	fi
 
-	tst_resm TPASS "Bug is not reproduced"
+	tst_res TPASS "Bug is not reproducible"
 }
 
-setup
-
-cpuset_test
-
-tst_exit
+tst_run
-- 
2.25.1

[LTP] [PATCH 2/3] cpuset_regression_test: Drop min cpu requirement

[Joerg Vehlow]

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

The bug can also be reproduced with only one cpu.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 .../controllers/cpuset/cpuset_regression_test.sh  | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
index 1dda19704..369fbedae 100755
--- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
+++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
@@ -27,13 +27,6 @@ old_cpu_exclusive_value=1
 
 setup()
 {
-	local cpu_num
-
-	cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
-	if [ $cpu_num -lt 2 ]; then
-		tst_brk TCONF "We need 2 cpus at least to have test"
-	fi
-
 	if ! is_cgroup_subsystem_available_and_enabled "cpuset"; then
 		tst_brk TCONF "Either kernel does not support cpuset controller or feature not enabled"
 	fi
@@ -101,12 +94,12 @@ test()
 	fi
 
 	# This may trigger the kernel crash
-	echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}
-	[ $? -ne 0 ] && tst_brk TFAIL "'echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}' failed"
+	echo 0 > ${root_cpuset_dir}/testdir/${cpus}
+	[ $? -ne 0 ] && tst_brk TFAIL "'echo 0 > ${root_cpuset_dir}/testdir/${cpus}' failed"
 
 	cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus})
-	if [ "${cpus_value}" != "0-1" ]; then
-		tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0-1'"
+	if [ "${cpus_value}" != "0" ]; then
+		tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0'"
 	fi
 
 	tst_res TPASS "Bug is not reproducible"
-- 
2.25.1

[LTP] [PATCH 3/3] cpuset_regression_test: Allow running, if groups exist

[Joerg Vehlow]

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Even if groups with a set cpuset.cpus exist, the original bug can still be
in the kernel. It was possible to create a group, set cpuset.cpus and only
then set cpuset.cpu_exclusive to 1. This did not trigger the bug.

This patche sets cpuset.cpus for all groups to an empty value, before
running the test and resets them to their original value after the test.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 .../cpuset/cpuset_regression_test.sh          | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
index 369fbedae..de1d6e649 100755
--- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
+++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
@@ -19,12 +19,54 @@ TST_MIN_KVER="3.18"
 . cgroup_lib.sh
 
 LOCAL_MOUNTPOINT="cpuset_test"
+BACKUP_DIRECTORY="cpuset_backup"
 
 root_cpuset_dir=
 cpu_exclusive="cpuset.cpu_exclusive"
 cpus="cpuset.cpus"
 old_cpu_exclusive_value=1
 
+# cpuset_backup_and_update <backup_dir> <what> <value>
+# Create backup of the values of a specific file (<what>)
+# in all cpuset groups and set the value to <value>
+# The backup is written to <backup_dir> in the same structure
+# as in the cpuset filesystem
+cpuset_backup_and_update()
+{
+	local backup_dir=$1
+	local what=$2
+	local value=$3
+	local old_dir=$PWD
+
+	cd ${root_cpuset_dir}
+	find . -mindepth 2 -name ${what} -print0 |
+	while IFS= read -r -d '' file; do
+		mkdir -p "$(dirname "${backup_dir}/${file}")"
+		cat "${file}" > "${backup_dir}/${file}"
+		echo "${value}" > "${file}"
+	done
+
+	cd $old_dir
+}
+
+# cpuset_restore <backup_dir> <what>
+# Restores the value of a file (<what>) in all cpuset
+# groups from the backup created by cpuset_backup_and_update
+cpuset_restore()
+{
+	local backup_dir=$1
+	local what=$2
+	local old_dir=$PWD
+
+	cd ${backup_dir}
+	find . -mindepth 2 -name ${what} -print0 |
+	while IFS= read -r -d '' file; do
+		cat "${file}" > "${root_cpuset_dir}/${file}"
+	done
+
+	cd $old_dir
+}
+
 setup()
 {
 	if ! is_cgroup_subsystem_available_and_enabled "cpuset"; then
@@ -49,6 +91,11 @@ setup()
 		tst_brk TBROK "Both cpuset.cpu_exclusive and cpu_exclusive do not exist"
 	fi
 
+	# Ensure that no group explicitely uses a cpu,
+	# otherwise setting cpuset.cpus for the testgroup will fail
+	mkdir ${BACKUP_DIRECTORY}
+	cpuset_backup_and_update "${PWD}/${BACKUP_DIRECTORY}" ${cpus} ""
+
 	old_cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
 	if [ "${old_cpu_exclusive_value}" != "1" ];then
 		echo 1 > ${root_cpuset_dir}/${cpu_exclusive}
@@ -62,6 +109,10 @@ cleanup()
 		rmdir ${root_cpuset_dir}/testdir
 	fi
 
+	if [ -d "${BACKUP_DIRECTORY}" ]; then
+		cpuset_restore "${PWD}/${BACKUP_DIRECTORY}" ${cpus}
+	fi
+
 	if [ "$old_cpu_exclusive_value" != 1 ]; then
 		# Need to flush, or write may fail with: "Device or resource busy"
 		sync
-- 
2.25.1

[LTP] [PATCH 0/3] cpuset_regression_test: convert and improve

[Richard Palethorpe]

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi,
>
> this is more or less a v2 of a patch I send previously (patch 3).
> I know that richard is not entirely happy with this patch, I will
> give it another try anyway...
> It is either this patch or another patch, that has to look through
> the cgroup hierarchy, to check if there is any group,that explicitely
> uses cpu 0.

If it is already being used then can you set it?

>
> To me, it is a better solution to just change groups for a short time,
> and check if the bug exists. If ltp tests are running, the chance, that
> there is anything running, that really needs a correct cpuset is very low.
> I am comming from a system, where cgroups are setup by a container launcher,
> that just happens to assign cpus to the containers - not even exclusively.
> LTP tests are used as some part of the testsuite, to test as close to a
> production system as possible.

I was thinking that if you are already using CPU sets then you either
don't have the bug or you won't hit it on your setup(s)? If so then the
test is redundant.

>
> The only way I could think of a process misbehaving by disabeling cpu pinning,
> would be a badly written multithread application, that cannot correctly run,
> if threads are really running in parallel, but this would also require a scheduling
> policy, that makes scheduling points predicatable. While I know that software like
> that exists (in fact I was working on something like that in the past), I think it
> is highly unlikely, that it is running parallel to ltp.
> And even then, this could be mitigated by not just setting cpu binding to undefined,
> but to one fixed core. But with the changes in patch 2, this is not
> possible.
>
> But anyhow ltp fiddles with lots of critical system parameters during it's runtime,
> there is no guarantee, that an application that requires some very specific kernel
> runtime settings survives this. That's why I would still vote for this patch.
>
> J?rg

I still think it has a small chance of causing problems for us. There
are some heterogeneous CPU systems where control software should run on
a given CPU. I don't know whether CGroups are used to control that or if
it would matter if the process is moved temporarily. It's just a small
risk I would avoid if the test is not really worth it.

OTOH the patch looks good otherwise, so it should be merged if no one
else agrees with me.

-- 
Thank you,
Richard.

[LTP] [PATCH 0/3] cpuset_regression_test: convert and improve

[Joerg Vehlow]

Hi Richard,

On 6/23/2021 1:11 PM, Richard Palethorpe wrote:
> Hello Joerg,
>
> Joerg Vehlow <lkml@jv-coder.de> writes:
>
>> Hi,
>>
>> this is more or less a v2 of a patch I send previously (patch 3).
>> I know that richard is not entirely happy with this patch, I will
>> give it another try anyway...
>> It is either this patch or another patch, that has to look through
>> the cgroup hierarchy, to check if there is any group,that explicitely
>> uses cpu 0.
> If it is already being used then can you set it?
The test can use any cpu, that is not explicitly assigned to a group 
already.
What I mean by "either this or another patch" (and forgot to type), the 
alternative
patch has to check if anything is using cpu 0 explicitly and then fail 
with TCONF.
Or it has to look for an used cpu core. That would be another possibility...

>
>> To me, it is a better solution to just change groups for a short time,
>> and check if the bug exists. If ltp tests are running, the chance, that
>> there is anything running, that really needs a correct cpuset is very low.
>> I am comming from a system, where cgroups are setup by a container launcher,
>> that just happens to assign cpus to the containers - not even exclusively.
>> LTP tests are used as some part of the testsuite, to test as close to a
>> production system as possible.
> I was thinking that if you are already using CPU sets then you either
> don't have the bug or you won't hit it on your setup(s)? If so then the
> test is redundant.
True about the "don't hit it part", at least with the setup, but I guess 
the reason for a regression test,
is to prevent regressions. This was clearly a bug in the kernel and not 
only an inconvenience. And since
there? is not "the one kernel source", I think it is important to run 
tests like this for as many different
kernels as possible. Apart from the already setup cgroups, there may be 
other uses of cgroups as well,
that try to set them up the other way around (first exclusive, then cpus).
>
>> The only way I could think of a process misbehaving by disabeling cpu pinning,
>> would be a badly written multithread application, that cannot correctly run,
>> if threads are really running in parallel, but this would also require a scheduling
>> policy, that makes scheduling points predicatable. While I know that software like
>> that exists (in fact I was working on something like that in the past), I think it
>> is highly unlikely, that it is running parallel to ltp.
>> And even then, this could be mitigated by not just setting cpu binding to undefined,
>> but to one fixed core. But with the changes in patch 2, this is not
>> possible.
>>
>> But anyhow ltp fiddles with lots of critical system parameters during it's runtime,
>> there is no guarantee, that an application that requires some very specific kernel
>> runtime settings survives this. That's why I would still vote for this patch.
>>
>> J?rg
> I still think it has a small chance of causing problems for us. There
> are some heterogeneous CPU systems where control software should run on
> a given CPU. I don't know whether CGroups are used to control that or if
> it would matter if the process is moved temporarily. It's just a small
> risk I would avoid if the test is not really worth it.
I get that, but these systems may have to opt-out of some tests anyway.
>
> OTOH the patch looks good otherwise, so it should be merged if no one
> else agrees with me.
Ok, lets see what the others have to say :)

J?rg

Re: [LTP] [PATCH 0/3] cpuset_regression_test: convert and improve

[Richard Palethorpe]

Hello Joerg,

Joerg Vehlow <lkml@jv-coder.de> writes:

> Hi Richard,
>
> On 6/23/2021 1:11 PM, Richard Palethorpe wrote:
>> Hello Joerg,
>>
>> Joerg Vehlow <lkml@jv-coder.de> writes:
>>
>>> Hi,
>>>
>>> this is more or less a v2 of a patch I send previously (patch 3).
>>> I know that richard is not entirely happy with this patch, I will
>>> give it another try anyway...
>>> It is either this patch or another patch, that has to look through
>>> the cgroup hierarchy, to check if there is any group,that explicitely
>>> uses cpu 0.
>> If it is already being used then can you set it?
> The test can use any cpu, that is not explicitly assigned to a group
> already.
> What I mean by "either this or another patch" (and forgot to type),
> the alternative
> patch has to check if anything is using cpu 0 explicitly and then fail
> with TCONF.
> Or it has to look for an used cpu core. That would be another possibility...
>
>>
>>> To me, it is a better solution to just change groups for a short time,
>>> and check if the bug exists. If ltp tests are running, the chance, that
>>> there is anything running, that really needs a correct cpuset is very low.
>>> I am comming from a system, where cgroups are setup by a container launcher,
>>> that just happens to assign cpus to the containers - not even exclusively.
>>> LTP tests are used as some part of the testsuite, to test as close to a
>>> production system as possible.
>> I was thinking that if you are already using CPU sets then you either
>> don't have the bug or you won't hit it on your setup(s)? If so then the
>> test is redundant.
> True about the "don't hit it part", at least with the setup, but I
> guess the reason for a regression test,
> is to prevent regressions. This was clearly a bug in the kernel and
> not only an inconvenience. And since
> there  is not "the one kernel source", I think it is important to run
> tests like this for as many different
> kernels as possible. Apart from the already setup cgroups, there may
> be other uses of cgroups as well,
> that try to set them up the other way around (first exclusive, then cpus).
>>
>>> The only way I could think of a process misbehaving by disabeling cpu pinning,
>>> would be a badly written multithread application, that cannot correctly run,
>>> if threads are really running in parallel, but this would also require a scheduling
>>> policy, that makes scheduling points predicatable. While I know that software like
>>> that exists (in fact I was working on something like that in the past), I think it
>>> is highly unlikely, that it is running parallel to ltp.
>>> And even then, this could be mitigated by not just setting cpu binding to undefined,
>>> but to one fixed core. But with the changes in patch 2, this is not
>>> possible.
>>>
>>> But anyhow ltp fiddles with lots of critical system parameters during it's runtime,
>>> there is no guarantee, that an application that requires some very specific kernel
>>> runtime settings survives this. That's why I would still vote for this patch.
>>>
>>> Jörg
>> I still think it has a small chance of causing problems for us. There
>> are some heterogeneous CPU systems where control software should run on
>> a given CPU. I don't know whether CGroups are used to control that or if
>> it would matter if the process is moved temporarily. It's just a small
>> risk I would avoid if the test is not really worth it.
> I get that, but these systems may have to opt-out of some tests anyway.
>>
>> OTOH the patch looks good otherwise, so it should be merged if no one
>> else agrees with me.
> Ok, lets see what the others have to say :)
>
> Jörg

So a few months later there are no comments. The patch-set as a whole
looks a like an improvement. So let's merge it.


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/3] cpuset_regression_test: Convert to new api

[Richard Palethorpe]

Joerg Vehlow <lkml@jv-coder.de> writes:

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

> ---
>  .../cpuset/cpuset_regression_test.sh          | 147 +++++++-----------
>  1 file changed, 56 insertions(+), 91 deletions(-)
>
> diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> index dccfd91cd..1dda19704 100755
> --- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> +++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> @@ -1,82 +1,65 @@
>  #!/bin/sh
> -#
> +# SPDX-License-Identifier: GPL-2.0-or-later
>  # Copyright (c) 2015 Fujitsu Ltd.
>  # Author: Zeng Linggang <zenglg.jy@cn.fujitsu.com>
>  #
> -# 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 St, Fifth Floor, Boston, MA  02110-1301  USA
> -#
>  # This is a regression test for commit:
> -# http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/
> -# ?id=bb2bc55
> +# http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bb2bc55
>  #
> +# A newly created cpuset group crashed the kernel, if exclusive was set to 1,
> +# before a cpuset was set.
> +
> +TST_SETUP=setup
> +TST_CLEANUP=cleanup
> +TST_TESTFUNC=test
> +TST_NEEDS_ROOT=1
> +TST_NEEDS_TMPDIR=1
> +TST_MIN_KVER="3.18"
> +
> +. cgroup_lib.sh
> +
> +LOCAL_MOUNTPOINT="cpuset_test"
>  
> -TCID=cpuset_regression_test
> -TST_TOTAL=1
> -. test.sh
> +root_cpuset_dir=
> +cpu_exclusive="cpuset.cpu_exclusive"
> +cpus="cpuset.cpus"
> +old_cpu_exclusive_value=1
>  
>  setup()
>  {
> -	tst_require_root
> +	local cpu_num
>  
> -	if tst_kvcmp -lt "3.18"; then
> -		tst_brkm TCONF "Test must be run with kernel 3.18.0 or newer"
> -	fi
> -
> -	local cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
> +	cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
>  	if [ $cpu_num -lt 2 ]; then
> -		tst_brkm TCONF "We need 2 cpus at least to have test"
> +		tst_brk TCONF "We need 2 cpus at least to have test"
>  	fi
>  
> -	tst_tmpdir
> -
> -	TST_CLEANUP=cleanup
> +	if ! is_cgroup_subsystem_available_and_enabled "cpuset"; then
> +		tst_brk TCONF "Either kernel does not support cpuset controller or feature not enabled"
> +	fi
>  
>  	# We need to mount cpuset if it is not found.
> -	mount_flag=0
> -	grep -w cpuset /proc/mounts > tmpfile
> -	if [ $? -eq 0 ]; then
> -		root_cpuset_dir=$(cat tmpfile | awk '{print $2}')
> -	else
> -		root_cpuset_dir="cpuset_test"
> +	root_cpuset_dir=$(get_cgroup_mountpoint cpuset)
> +	if [ -z "$root_cpuset_dir" ]; then
> +		root_cpuset_dir="$LOCAL_MOUNTPOINT"
>  
>  		ROD_SILENT mkdir -p ${root_cpuset_dir}
> -
>  		ROD_SILENT mount -t cpuset cpuset ${root_cpuset_dir}
> -
> -		mount_flag=1
>  	fi
>  
> -	if [ -f ${root_cpuset_dir}/cpuset.cpu_exclusive ]; then
> -		cpu_exclusive=cpuset.cpu_exclusive
> -		cpus=cpuset.cpus
> -	elif [ -f ${root_cpuset_dir}/cpu_exclusive ]; then
> +	if ! [ -f ${root_cpuset_dir}/${cpu_exclusive} ]; then
>  		cpu_exclusive=cpu_exclusive
>  		cpus=cpus
> -	else
> -		tst_brkm TBROK "Both cpuset.cpu_exclusive and cpu_exclusive" \
> -			       "do not exist."
>  	fi
>  
> -	cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
> -	if [ "${cpu_exclusive_value}" != "1" ];then
> +	if ! [ -f ${root_cpuset_dir}/${cpu_exclusive} ]; then
> +		tst_brk TBROK "Both cpuset.cpu_exclusive and cpu_exclusive do not exist"
> +	fi
> +
> +	old_cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
> +	if [ "${old_cpu_exclusive_value}" != "1" ];then
>  		echo 1 > ${root_cpuset_dir}/${cpu_exclusive}
> -		if [ $? -ne 0 ]; then
> -			tst_brkm TBROK "'echo 1 >" \
> -				       "${root_cpuset_dir}/${cpu_exclusive}'" \
> -				       "failed"
> -		fi
> +		[ $? -ne 0 ] && tst_brk TBROK "'echo 1 > ${root_cpuset_dir}/${cpu_exclusive}' failed"
>  	fi
>  }
>  
> @@ -86,65 +69,47 @@ cleanup()
>  		rmdir ${root_cpuset_dir}/testdir
>  	fi
>  
> -	if [ "$cpu_exclusive_value" != 1 ]; then
> -		# Need to flush, or may be output:
> -		# "write error: Device or resource busy"
> +	if [ "$old_cpu_exclusive_value" != 1 ]; then
> +		# Need to flush, or write may fail with: "Device or resource busy"
>  		sync
> -
> -		echo ${cpu_exclusive_value} > \
> -		     ${root_cpuset_dir}/${cpu_exclusive}
> +		echo ${old_cpu_exclusive_value} > ${root_cpuset_dir}/${cpu_exclusive}
>  	fi
>  
> -	if [ "${mount_flag}" = "1" ]; then
> -		umount ${root_cpuset_dir}
> +	if [ -d "$LOCAL_MOUNTPOINT" ]; then
> +		umount ${LOCAL_MOUNTPOINT}
>  		if [ $? -ne 0 ]; then
> -			tst_resm TWARN "'umount ${root_cpuset_dir}' failed"
> +			tst_res TWARN "'umount ${LOCAL_MOUNTPOINT}' failed"
>  		fi
>  
> -		if [ -d "${root_cpuset_dir}" ]; then
> -			rmdir ${root_cpuset_dir}
> -		fi
> +		rmdir ${LOCAL_MOUNTPOINT}
>  	fi
> -
> -	tst_rmdir
>  }
>  
> -cpuset_test()
> +test()
>  {
> +	local cpu_exclusive_tmp cpus_value
> +
>  	ROD_SILENT mkdir ${root_cpuset_dir}/testdir
>  
>  	# Creat an exclusive cpuset.
>  	echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}
> -	if [ $? -ne 0 ]; then
> -		tst_brkm TFAIL "'echo 1 >" \
> -			       "${root_cpuset_dir}/testdir/${cpu_exclusive}'" \
> -			       "failed"
> -	fi
> -
> -	local cpu_exclusive_tmp=$(cat \
> -				  ${root_cpuset_dir}/testdir/${cpu_exclusive})
> +	[ $? -ne 0 ] && tst_brk TFAIL "'echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}' failed"
> +	
> +	cpu_exclusive_tmp=$(cat ${root_cpuset_dir}/testdir/${cpu_exclusive})
>  	if [ "${cpu_exclusive_tmp}" != "1" ]; then
> -		tst_brkm TFAIL "${cpu_exclusive} is '${cpu_exclusive_tmp}'," \
> -			       "expected '1'"
> +		tst_brk TFAIL "${cpu_exclusive} is '${cpu_exclusive_tmp}', expected '1'"
>  	fi
>  
> -	# ${cpus} is empty at the begin, that maybe make the system *crash*.
> +	# This may trigger the kernel crash
>  	echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}
> -	if [ $? -ne 0 ]; then
> -		tst_brkm TFAIL "'echo 0-1 >" \
> -			       "${root_cpuset_dir}/testdir/${cpus}' failed"
> -	fi
> +	[ $? -ne 0 ] && tst_brk TFAIL "'echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}' failed"
>  
> -	local cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus})
> +	cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus})
>  	if [ "${cpus_value}" != "0-1" ]; then
> -		tst_brkm TFAIL "${cpus} is '${cpus_value}', expected '0-1'"
> +		tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0-1'"
>  	fi
>  
> -	tst_resm TPASS "Bug is not reproduced"
> +	tst_res TPASS "Bug is not reproducible"
>  }
>  
> -setup
> -
> -cpuset_test
> -
> -tst_exit
> +tst_run
> -- 
> 2.25.1


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 2/3] cpuset_regression_test: Drop min cpu requirement

[Richard Palethorpe]

Joerg Vehlow <lkml@jv-coder.de> writes:

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> The bug can also be reproduced with only one cpu.
>
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  .../controllers/cpuset/cpuset_regression_test.sh  | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> index 1dda19704..369fbedae 100755
> --- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> +++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> @@ -27,13 +27,6 @@ old_cpu_exclusive_value=1
>  
>  setup()
>  {
> -	local cpu_num
> -
> -	cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
> -	if [ $cpu_num -lt 2 ]; then
> -		tst_brk TCONF "We need 2 cpus at least to have test"
> -	fi
> -
>  	if ! is_cgroup_subsystem_available_and_enabled "cpuset"; then
>  		tst_brk TCONF "Either kernel does not support cpuset controller or feature not enabled"
>  	fi
> @@ -101,12 +94,12 @@ test()
>  	fi
>  
>  	# This may trigger the kernel crash
> -	echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}
> -	[ $? -ne 0 ] && tst_brk TFAIL "'echo 0-1 > ${root_cpuset_dir}/testdir/${cpus}' failed"
> +	echo 0 > ${root_cpuset_dir}/testdir/${cpus}
> +	[ $? -ne 0 ] && tst_brk TFAIL "'echo 0 > ${root_cpuset_dir}/testdir/${cpus}' failed"
>  
>  	cpus_value=$(cat ${root_cpuset_dir}/testdir/${cpus})
> -	if [ "${cpus_value}" != "0-1" ]; then
> -		tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0-1'"
> +	if [ "${cpus_value}" != "0" ]; then
> +		tst_brk TFAIL "${cpus} is '${cpus_value}', expected '0'"
>  	fi
>  
>  	tst_res TPASS "Bug is not reproducible"


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 3/3] cpuset_regression_test: Allow running, if groups exist

[Richard Palethorpe]

Joerg Vehlow <lkml@jv-coder.de> writes:

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> Even if groups with a set cpuset.cpus exist, the original bug can still be
> in the kernel. It was possible to create a group, set cpuset.cpus and only
> then set cpuset.cpu_exclusive to 1. This did not trigger the bug.
>
> This patche sets cpuset.cpus for all groups to an empty value, before
> running the test and resets them to their original value after the test.
>
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Acked-by: Richard Palethorpe <rpalethorpe@suse.com>

> ---
>  .../cpuset/cpuset_regression_test.sh          | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> index 369fbedae..de1d6e649 100755
> --- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> +++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> @@ -19,12 +19,54 @@ TST_MIN_KVER="3.18"
>  . cgroup_lib.sh
>  
>  LOCAL_MOUNTPOINT="cpuset_test"
> +BACKUP_DIRECTORY="cpuset_backup"
>  
>  root_cpuset_dir=
>  cpu_exclusive="cpuset.cpu_exclusive"
>  cpus="cpuset.cpus"
>  old_cpu_exclusive_value=1
>  
> +# cpuset_backup_and_update <backup_dir> <what> <value>
> +# Create backup of the values of a specific file (<what>)
> +# in all cpuset groups and set the value to <value>
> +# The backup is written to <backup_dir> in the same structure
> +# as in the cpuset filesystem
> +cpuset_backup_and_update()
> +{
> +	local backup_dir=$1
> +	local what=$2
> +	local value=$3
> +	local old_dir=$PWD
> +
> +	cd ${root_cpuset_dir}
> +	find . -mindepth 2 -name ${what} -print0 |
> +	while IFS= read -r -d '' file; do
> +		mkdir -p "$(dirname "${backup_dir}/${file}")"
> +		cat "${file}" > "${backup_dir}/${file}"
> +		echo "${value}" > "${file}"
> +	done
> +
> +	cd $old_dir
> +}
> +
> +# cpuset_restore <backup_dir> <what>
> +# Restores the value of a file (<what>) in all cpuset
> +# groups from the backup created by cpuset_backup_and_update
> +cpuset_restore()
> +{
> +	local backup_dir=$1
> +	local what=$2
> +	local old_dir=$PWD
> +
> +	cd ${backup_dir}
> +	find . -mindepth 2 -name ${what} -print0 |
> +	while IFS= read -r -d '' file; do
> +		cat "${file}" > "${root_cpuset_dir}/${file}"
> +	done
> +
> +	cd $old_dir
> +}
> +
>  setup()
>  {
>  	if ! is_cgroup_subsystem_available_and_enabled "cpuset"; then
> @@ -49,6 +91,11 @@ setup()
>  		tst_brk TBROK "Both cpuset.cpu_exclusive and cpu_exclusive do not exist"
>  	fi
>  
> +	# Ensure that no group explicitely uses a cpu,
> +	# otherwise setting cpuset.cpus for the testgroup will fail
> +	mkdir ${BACKUP_DIRECTORY}
> +	cpuset_backup_and_update "${PWD}/${BACKUP_DIRECTORY}" ${cpus} ""
> +
>  	old_cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
>  	if [ "${old_cpu_exclusive_value}" != "1" ];then
>  		echo 1 > ${root_cpuset_dir}/${cpu_exclusive}
> @@ -62,6 +109,10 @@ cleanup()
>  		rmdir ${root_cpuset_dir}/testdir
>  	fi
>  
> +	if [ -d "${BACKUP_DIRECTORY}" ]; then
> +		cpuset_restore "${PWD}/${BACKUP_DIRECTORY}" ${cpus}
> +	fi
> +
>  	if [ "$old_cpu_exclusive_value" != 1 ]; then
>  		# Need to flush, or write may fail with: "Device or resource busy"
>  		sync


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/3] cpuset_regression_test: Convert to new api

[Richard Palethorpe]

Hi,

> -cpuset_test()
> +test()
>  {
> +	local cpu_exclusive_tmp cpus_value
> +
>  	ROD_SILENT mkdir ${root_cpuset_dir}/testdir
>  
>  	# Creat an exclusive cpuset.
>  	echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}
> -	if [ $? -ne 0 ]; then
> -		tst_brkm TFAIL "'echo 1 >" \
> -			       "${root_cpuset_dir}/testdir/${cpu_exclusive}'" \
> -			       "failed"
> -	fi
> -
> -	local cpu_exclusive_tmp=$(cat \
> -				  ${root_cpuset_dir}/testdir/${cpu_exclusive})
> +	[ $? -ne 0 ] && tst_brk TFAIL "'echo 1 > ${root_cpuset_dir}/testdir/${cpu_exclusive}' failed"
> +	

Merged, but with the trailing whitespace in the line above removed.

-- 
Thank you,
Richard.

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