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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

-- 
Thank you,
Richard.

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