[LTP] [PATCH] cpuset_regression_test: Fix test, if cpuset groups exist already

[Joerg Vehlow]

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