diff mbox series

cpuset_regression_test: Fix test, if cpuset groups exist already

Message ID 20211122141355.299621-1-lkml@jv-coder.de
State New
Headers show
Series cpuset_regression_test: Fix test, if cpuset groups exist already | expand

Commit Message

Joerg Vehlow Nov. 22, 2021, 2:13 p.m. UTC
From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

Fix three errors:
 1. read -d is not posix, but not even required,
    because find and read work  line-based
 2. Setting cpuset.cpus to an empty string is not allowed.
    -> If there are cpuset groups defined already, we need at least to cpus.
    One is used for the test, the other one is used for existing groups.
 3. Existing cgroup hierarchies were not handled correctly.
    When setting  the cpuset.cpus, it must be done breadth-first, otherwise
    cpu constraints for parent groups can be violated.

Fixes: 6950e96eabb2 ("cpuset_regression_test: Allow running, if groups exist")
Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---

@Richard Sorry for so many bugs in the patch... I guess I way a bit tired


 .../cpuset/cpuset_regression_test.sh          | 84 ++++++++++++++++---
 1 file changed, 72 insertions(+), 12 deletions(-)

Comments

Richard Palethorpe Nov. 22, 2021, 3:09 p.m. UTC | #1
Hello Joerg,

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

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> Fix three errors:
>  1. read -d is not posix, but not even required,
>     because find and read work  line-based
>  2. Setting cpuset.cpus to an empty string is not allowed.
>     -> If there are cpuset groups defined already, we need at least to
> cpus.

two* cpus
>     One is used for the test, the other one is used for existing groups.
>  3. Existing cgroup hierarchies were not handled correctly.
>     When setting  the cpuset.cpus, it must be done breadth-first, otherwise
>     cpu constraints for parent groups can be violated.
>
> Fixes: 6950e96eabb2 ("cpuset_regression_test: Allow running, if groups exist")
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
> ---
>
> @Richard Sorry for so many bugs in the patch... I guess I way a bit
> tired

I'm not surprised that there are issues saving a restoring these
settings :-p. OTOH the solution looks OK overall, but please see
comments below.

>
>
>  .../cpuset/cpuset_regression_test.sh          | 84 ++++++++++++++++---
>  1 file changed, 72 insertions(+), 12 deletions(-)
>
> diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> index cc6bfdc64..f6447a656 100755
> --- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> +++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
> @@ -21,32 +21,80 @@ TST_MIN_KVER="3.18"
>  LOCAL_MOUNTPOINT="cpuset_test"
>  BACKUP_DIRECTORY="cpuset_backup"
>  
> +cpu_num=
>  root_cpuset_dir=
>  cpu_exclusive="cpuset.cpu_exclusive"
>  cpus="cpuset.cpus"
>  old_cpu_exclusive_value=1
>  
> -# cpuset_backup_and_update <backup_dir> <what> <value>
> +# Check if there are cpuset groups
> +cpuset_has_groups()
> +{
> +	local old_dir=$PWD
> +	local result=0

Why are these set as local here?

> +
> +	find ${root_cpuset_dir} -mindepth 1 -type d -exec echo 1 \;
> -quit
> +}
> +
> +# cpuset_find_breadth_first <what>
> +# Do a breath first find of <what>
> +cpuset_find_breadth_first()
> +{
> +	local what=$1
> +
> +	# Breath first find implementation:
> +	# Use awk to prepend the length of the path, sort it
> +	# and remove the length again.
> +	# Since all commands work line-based,
> +	# this works with meta characters and spaces.
> +	find . -mindepth 2 -name ${what} | 
> +	    awk '{print length($0) " " $0}' |

This is the length of the path in characters. I think you want to count
the path components instead. The below is from my system

find /sys/fs/cgroup  -type d | awk '{print length($0) " " $0}' | sort -n
...
43 /sys/fs/cgroup/system.slice/wickedd.service
44 /sys/fs/cgroup/sys-fs-fuse-connections.mount
...

sys-fs-fuse-connections.mount should come first in a breadth first
search.


> +	    sort -n | cut -d " " -f 2-
> +}
> +
> +# cpuset_backup_and_update <backup_dir> <what>
>  # Create backup of the values of a specific file (<what>)
> -# in all cpuset groups and set the value to <value>
> +# in all cpuset groups and set the value to 1
>  # 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
> +	local cpu_max=$((cpu_num - 1))
> +	local res
>  
>  	cd ${root_cpuset_dir}
> -	find . -mindepth 2 -name ${what} -print0 |
> -	while IFS= read -r -d '' file; do
> +
> +	# First do breath-first search and set all groups to use all cpus.
> +	# Breath-first is important, because the parent groups
> +	# must have all cpus assigned to a child group first

This is confusing. Inline comments are not encouraged either. IMO the
commit message is enough or else you can add a brief explanation of the
saving and restore procedure at the top.
Joerg Vehlow Nov. 23, 2021, 5:46 a.m. UTC | #2
Hi Richard,

On 11/22/2021 4:09 PM, Richard Palethorpe wrote:
> Hello Joerg,
>
> Joerg Vehlow <lkml@jv-coder.de> writes:
>
>> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>>
>> Fix three errors:
>>   1. read -d is not posix, but not even required,
>>      because find and read work  line-based
>>   2. Setting cpuset.cpus to an empty string is not allowed.
>>      -> If there are cpuset groups defined already, we need at least to
>> cpus.
> two* cpus
Right I'll fix it.
>>      One is used for the test, the other one is used for existing groups.
>>   3. Existing cgroup hierarchies were not handled correctly.
>>      When setting  the cpuset.cpus, it must be done breadth-first, otherwise
>>      cpu constraints for parent groups can be violated.
>>
>> Fixes: 6950e96eabb2 ("cpuset_regression_test: Allow running, if groups exist")
>> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>> ---
>>
>> @Richard Sorry for so many bugs in the patch... I guess I way a bit
>> tired
> I'm not surprised that there are issues saving a restoring these
> settings :-p. OTOH the solution looks OK overall, but please see
> comments below.
Yeah some issues would have been ok, but a total failure not ;)
>
>>
>>   .../cpuset/cpuset_regression_test.sh          | 84 ++++++++++++++++---
>>   1 file changed, 72 insertions(+), 12 deletions(-)
>>
>> diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
>> index cc6bfdc64..f6447a656 100755
>> --- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
>> +++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
>> @@ -21,32 +21,80 @@ TST_MIN_KVER="3.18"
>>   LOCAL_MOUNTPOINT="cpuset_test"
>>   BACKUP_DIRECTORY="cpuset_backup"
>>   
>> +cpu_num=
>>   root_cpuset_dir=
>>   cpu_exclusive="cpuset.cpu_exclusive"
>>   cpus="cpuset.cpus"
>>   old_cpu_exclusive_value=1
>>   
>> -# cpuset_backup_and_update <backup_dir> <what> <value>
>> +# Check if there are cpuset groups
>> +cpuset_has_groups()
>> +{
>> +	local old_dir=$PWD
>> +	local result=0
> Why are these set as local here?
Sorry forgot to delete them
>
>> +
>> +	find ${root_cpuset_dir} -mindepth 1 -type d -exec echo 1 \;
>> -quit
>> +}
>> +
>> +# cpuset_find_breadth_first <what>
>> +# Do a breath first find of <what>
>> +cpuset_find_breadth_first()
>> +{
>> +	local what=$1
>> +
>> +	# Breath first find implementation:
>> +	# Use awk to prepend the length of the path, sort it
>> +	# and remove the length again.
>> +	# Since all commands work line-based,
>> +	# this works with meta characters and spaces.
>> +	find . -mindepth 2 -name ${what} |
>> +	    awk '{print length($0) " " $0}' |
> This is the length of the path in characters. I think you want to count
> the path components instead. The below is from my system
>
> find /sys/fs/cgroup  -type d | awk '{print length($0) " " $0}' | sort -n
> ...
> 43 /sys/fs/cgroup/system.slice/wickedd.service
> 44 /sys/fs/cgroup/sys-fs-fuse-connections.mount
> ...
>
> sys-fs-fuse-connections.mount should come first in a breadth first
> search.
Actually it doesn't matter. The only thing, that matters here is that 
parent groups
are handled before child groups. That is ensured, because the prefix of 
a child group is
always the parent group and so the child is longer.
At first I had it named "somewhat breadth-first" ;)
Counting path components is not a trivial task, because there may be 
escaped slashes.
Maybe not calling it breadth first, but parent-first would be a solution?
>
>
>> +	    sort -n | cut -d " " -f 2-
>> +}
>> +
>> +# cpuset_backup_and_update <backup_dir> <what>
>>   # Create backup of the values of a specific file (<what>)
>> -# in all cpuset groups and set the value to <value>
>> +# in all cpuset groups and set the value to 1
>>   # 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
>> +	local cpu_max=$((cpu_num - 1))
>> +	local res
>>   
>>   	cd ${root_cpuset_dir}
>> -	find . -mindepth 2 -name ${what} -print0 |
>> -	while IFS= read -r -d '' file; do
>> +
>> +	# First do breath-first search and set all groups to use all cpus.
>> +	# Breath-first is important, because the parent groups
>> +	# must have all cpus assigned to a child group first
> This is confusing. Inline comments are not encouraged either. IMO the
> commit message is enough or else you can add a brief explanation of the
> saving and restore procedure at the top.
Ok, I thought a bit of documentation why this is done would be a good 
idea, because it is not very obvious.

Joerg
Richard Palethorpe Nov. 23, 2021, 9:46 a.m. UTC | #3
Hello Joerg,

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

>>> +cpuset_find_breadth_first()
>>> +{
>>> +	local what=$1
>>> +
>>> +	# Breath first find implementation:
>>> +	# Use awk to prepend the length of the path, sort it
>>> +	# and remove the length again.
>>> +	# Since all commands work line-based,
>>> +	# this works with meta characters and spaces.
>>> +	find . -mindepth 2 -name ${what} |
>>> +	    awk '{print length($0) " " $0}' |
>> This is the length of the path in characters. I think you want to count
>> the path components instead. The below is from my system
>>
>> find /sys/fs/cgroup  -type d | awk '{print length($0) " " $0}' | sort -n
>> ...
>> 43 /sys/fs/cgroup/system.slice/wickedd.service
>> 44 /sys/fs/cgroup/sys-fs-fuse-connections.mount
>> ...
>>
>> sys-fs-fuse-connections.mount should come first in a breadth first
>> search.
> Actually it doesn't matter. The only thing, that matters here is that
> parent groups
> are handled before child groups. That is ensured, because the prefix
> of a child group is
> always the parent group and so the child is longer.
> At first I had it named "somewhat breadth-first" ;)
> Counting path components is not a trivial task, because there may be
> escaped slashes.
> Maybe not calling it breadth first, but parent-first would be a
> solution?

Sounds good, +1

>>
>>
>>> +	    sort -n | cut -d " " -f 2-
>>> +}
>>> +
>>> +# cpuset_backup_and_update <backup_dir> <what>
>>>   # Create backup of the values of a specific file (<what>)
>>> -# in all cpuset groups and set the value to <value>
>>> +# in all cpuset groups and set the value to 1
>>>   # 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
>>> +	local cpu_max=$((cpu_num - 1))
>>> +	local res
>>>     	cd ${root_cpuset_dir}
>>> -	find . -mindepth 2 -name ${what} -print0 |
>>> -	while IFS= read -r -d '' file; do
>>> +
>>> +	# First do breath-first search and set all groups to use all cpus.
>>> +	# Breath-first is important, because the parent groups
>>> +	# must have all cpus assigned to a child group first
>> This is confusing. Inline comments are not encouraged either. IMO the
>> commit message is enough or else you can add a brief explanation of the
>> saving and restore procedure at the top.
> Ok, I thought a bit of documentation why this is done would be a good
> idea, because it is not very obvious.

The main thing is that the comment doesn't make sense to me. In
particular the last sentence. Inline comments are also discouraged in
the style guide.

Also you can handle this depth-first, you just need to lift restrictions
before descending into the tree. So breadth-first is not really
important.

I think it's enough to explain at the top that descendents are bounded
by their ancestors. Then it's down to the reviewer or future developers
to figure out how the shell code handles that.

>
> Joerg
diff mbox series

Patch

diff --git a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
index cc6bfdc64..f6447a656 100755
--- a/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
+++ b/testcases/kernel/controllers/cpuset/cpuset_regression_test.sh
@@ -21,32 +21,80 @@  TST_MIN_KVER="3.18"
 LOCAL_MOUNTPOINT="cpuset_test"
 BACKUP_DIRECTORY="cpuset_backup"
 
+cpu_num=
 root_cpuset_dir=
 cpu_exclusive="cpuset.cpu_exclusive"
 cpus="cpuset.cpus"
 old_cpu_exclusive_value=1
 
-# cpuset_backup_and_update <backup_dir> <what> <value>
+# Check if there are cpuset groups
+cpuset_has_groups()
+{
+	local old_dir=$PWD
+	local result=0
+
+	find ${root_cpuset_dir} -mindepth 1 -type d -exec echo 1 \; -quit
+}
+
+# cpuset_find_breadth_first <what>
+# Do a breath first find of <what>
+cpuset_find_breadth_first()
+{
+	local what=$1
+
+	# Breath first find implementation:
+	# Use awk to prepend the length of the path, sort it
+	# and remove the length again.
+	# Since all commands work line-based,
+	# this works with meta characters and spaces.
+	find . -mindepth 2 -name ${what} | 
+	    awk '{print length($0) " " $0}' | 
+	    sort -n | cut -d " " -f 2-
+}
+
+# cpuset_backup_and_update <backup_dir> <what>
 # Create backup of the values of a specific file (<what>)
-# in all cpuset groups and set the value to <value>
+# in all cpuset groups and set the value to 1
 # 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
+	local cpu_max=$((cpu_num - 1))
+	local res
 
 	cd ${root_cpuset_dir}
-	find . -mindepth 2 -name ${what} -print0 |
-	while IFS= read -r -d '' file; do
+
+	# First do breath-first search and set all groups to use all cpus.
+	# Breath-first is important, because the parent groups
+	# must have all cpus assigned to a child group first
+	cpuset_find_breadth_first ${what} |
+	while read -r file; do
+		# Skip cgroups with no set cpuset
+		[ "$(cat "${file}")" = "" ] && continue
+
 		mkdir -p "$(dirname "${backup_dir}/${file}")"
 		cat "${file}" > "${backup_dir}/${file}"
-		echo "${value}" > "${file}"
+		echo "0-${cpu_max}" > "${file}" || exit 1
 	done
+	if [ $? -ne 0 ]; then
+		cd $old_dir
+	 	return 1
+	fi
+
+	# Now do depth first and limit the cpus to cpu 1
+	find . -depth -mindepth 2 -name ${what} |
+	while read -r file; do
+		[ "$(cat "${file}")" = "" ] && continue
+		echo "1" > "${file}"
+	done
+	res=$?
 
 	cd $old_dir
+
+	return $res
 }
 
 # cpuset_restore <backup_dir> <what>
@@ -59,8 +107,12 @@  cpuset_restore()
 	local old_dir=$PWD
 
 	cd ${backup_dir}
-	find . -mindepth 2 -name ${what} -print0 |
-	while IFS= read -r -d '' file; do
+
+	# Do a breath-first restore.
+	# This works, because cpusets set on parents, automatically
+	# limit the set for the child.
+	cpuset_find_breadth_first ${what} |
+	while read -r file; do
 		cat "${file}" > "${root_cpuset_dir}/${file}"
 	done
 
@@ -91,10 +143,18 @@  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} ""
+	# Ensure that we can use cpu 0 exclusively
+	if [ "$(cpuset_has_groups)" = "1" ]; then
+		cpu_num=$(tst_getconf _NPROCESSORS_ONLN)
+		if [ $cpu_num -lt 2 ]; then
+			tst_brk TCONF "There are already cpuset groups, so at least two cpus are required."
+		fi
+
+		# Use cpu 1 for all existing cpuset cgroups
+		mkdir ${BACKUP_DIRECTORY}
+		cpuset_backup_and_update "${PWD}/${BACKUP_DIRECTORY}" ${cpus}
+		[ $? -ne 0 ] && tst_brk TBROK "Unable to prepare existing cpuset cgroups"
+	fi
 
 	old_cpu_exclusive_value=$(cat ${root_cpuset_dir}/${cpu_exclusive})
 	if [ "${old_cpu_exclusive_value}" != "1" ];then