Re: [PATCH 23/23] cgroup/cpuset: Documentation and kselftest updates

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [Medium] The newly added `nohz_full` check in `check_isolcpus()` is placed before the state stabilization retry loop, leading to false test failures and bypassed validation.
- [Medium] The selftest script enables kernel dynamic debugging globally but fails to restore its state when the test finishes or aborts.
- [Medium] The new `CPU0_ISOLCPUS_MATRIX` test cases omit the 12th field (`ISOLCPUS`), silently bypassing validation of the `cpuset.cpus.isolated` state.
- [Low] Dead code: the variable `CHK_NOHZ_FULL` is defined and assigned but never used.
- [Low] Grammatical errors and typos in documentation and commit message.
--

commit 84d90aacf6dde2c021ed021a8f60057d923655be
Author: Waiman Long <longman@redhat.com>

cgroup/cpuset: Documentation and kselftest updates

This commit updates cpuset tests and documentation to reflect new CPU
hotplug capabilities for nohz_full. It adds tests to prevent using CPU 0
in isolated partitions when it cannot be brought offline, and updates
documentation in cgroup-v2.rst.

> A new test will also be run in offline isn't allowed in CPU 0 to verify
> that using CPU 0 as part of an isolated partition will fail.

This isn't a bug, but should "run in offline" be "run if offline"?

> Since there is a slight performance overhead to enable runtime changes
> to nohz_full CPU list, users have to explicitly opt in by adding a
> "nohz_ful" kernel command line parameter with or without a CPU list.

This isn't a bug, but there's a typo ("nohz_ful") in the commit message.

> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 91beaa6798ce0..81181dc100ead 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
[ ... ]
> @@ -2647,11 +2648,29 @@ Cpuset Interface Files
>  	partition or scheduling domain.  The set of exclusive CPUs is
>  	determined by the value of its "cpuset.cpus.exclusive.effective".
>  
> -	When set to "isolated", the CPUs in that partition will be in
> -	an isolated state without any load balancing from the scheduler
> -	and excluded from the unbound workqueues.  Tasks placed in such
> -	a partition with multiple CPUs should be carefully distributed
> -	and bound to each of the individual CPUs for optimal performance.
> +	When set to "isolated", the CPUs in that partition will be in an
> +	isolated state without any load balancing from the scheduler and
> +	excluded from the unbound workqueues as well as other OS noises.
> +	Tasks placed in such a partition with multiple CPUs should be
> +	carefully distributed and bound to each of the individual CPUs
> +	for optimal performance.
> +
> +	As CPU hotplug, if supported, is used to improve the degree of
> +	CPU isolation close to the "nohz_full" kernel boot parameter.

This isn't a bug, but this sentence seems to be a subordinate clause lacking
a main clause.

> diff --git a/tools/testing/selftests/cgroup/test_cpuset_prs.sh b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
> index a56f4153c64df..eebb4122b581c 100755
> --- a/tools/testing/selftests/cgroup/test_cpuset_prs.sh
> +++ b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
> @@ -67,6 +67,12 @@ then
>  	echo Y > /sys/kernel/debug/sched/verbose
>  fi
>  
> +# Enable dynamic debug message if available
> +DYN_DEBUG=/proc/dynamic_debug/control
> +[[ -f $DYN_DEBUG ]] && {
> +	echo "file kernel/cpu.c +p" > $DYN_DEBUG
> +}

Does this leak the debug state after the test finishes?

The script enables dynamic debug messages for the CPU hotplug subsystem here.

Looking at the cleanup() function triggered via trap, it does not restore or
disable this configuration, which permanently alters the system's global debug
state.

Should the cleanup() function revert this change?

[ ... ]
> @@ -84,6 +90,15 @@ echo member > test/cpuset.cpus.partition
>  echo "" > test/cpuset.cpus
>  [[ $RESULT -eq 0 ]] && skip_test "Child cgroups are using cpuset!"
>  
> +#
> +# If nohz_full parameter is specified and nohz_full file exists, CPU hotplug
> +# will be used to modify nohz_full cpumask to include all the isolated CPUs
> +# in cpuset isolated partitions.
> +#
> +NOHZ_FULL=/sys/devices/system/cpu/nohz_full
> +BOOT_NOHZ_FULL=$(fmt -1 /proc/cmdline | grep "^nohz_full")
> +[[ "$BOOT_NOHZ_FULL" = nohz_full ]] && CHK_NOHZ_FULL=1

Is CHK_NOHZ_FULL used anywhere else in the script?

It looks like it is defined and assigned here, but the same string
comparison is redundantly evaluated later inside check_isolcpus() without
using this variable.

[ ... ]
> +#
> +# Test matrix to verify that using CPU 0 in isolated (local or remote) partition
> +# will fail when offline isn't allowed for CPU 0.
> +#
> +CPU0_ISOLCPUS_MATRIX=(
> +	#  old-A1 old-A2 old-A3 old-B1 new-A1 new-A2 new-A3 new-B1 fail ECPUs Pstate ISOLCPUS
> +	#  ------ ------ ------ ------ ------ ------ ------ ------ ---- ----- ------ --------
> +	"   C0-3     .      .    C4-5     P2     .      .      .     0 A1:0-3|B1:4-5 A1:P-2"
> +	"   C1-3     .      .      .      P2     .      .      .     0 A1:1-3 A1:P2"
> +	"   C1-3     .      .      .    P2:C0-3  .      .      .     0 A1:0-3 A1:P-2"
> +	"  CX0-3   C0-3     .      .       .     P2     .      .     0 A1:0-3|A2:0-3 A2:P-2"
> +	"  CX0-3 C0-3:X1-3  .      .       .     P2     .      .     0 A1:0|A2:1-3 A2:P2"
> +	"  CX0-3 C0-3:X1-3  .      .       .   P2:X0-3  .      .     0 A1:0-3|A2:0-3 A2:P-2"
> +)

Are these test cases bypassing the validation of the cpuset.cpus.isolated
state?

The test matrix headers indicate 12 fields, but the entries only provide
11 fields, omitting the final ISOLCPUS field.

Because the 12th field is missing, check_test_results() will evaluate if
ISOLCPUS is non-empty as false and skip calling check_isolcpus(), leaving
the isolated state unchecked.

[ ... ]
> @@ -789,6 +821,18 @@ check_isolcpus()
>  		EXPECTED_SDOMAIN=$EXPECTED_ISOLCPUS
>  	fi
>  
> +	#
> +	# Check if nohz_full match cpuset.cpus.isolated if nohz_boot parameter
> +	# specified with no parameter.
> +	#

This isn't a bug, but there's a typo ("nohz_boot" instead of "nohz_full")
in this comment.

> +	[[ -f $NOHZ_FULL && "$BOOT_NOHZ_FULL" = nohz_full ]] && {
> +		NOHZ_FULL_CPUS=$(cat $NOHZ_FULL)
> +		[[ "$ISOLCPUS" != "$NOHZ_FULL_CPUS" ]] && {
> +			echo "nohz_full ($NOHZ_FULL_CPUS) does not match cpuset.cpus.isolated ($ISOLCPUS)"
> +			return 1
> +		}
> +	}
> +

Will this check cause false test failures or bypass validation due to
asynchronous state updates?

The script reads the state and immediately returns an error if they mismatch.

However, later in the function there is a 50ms retry loop to account for
transient states.

If the files update at slightly different times, the check instantly fails.
If both files are in their old state, they match, passing the check, but
then the script pauses and reads the new state without re-evaluating
nohz_full against the final state.

Could this check be moved after the state stabilization retry loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260421030351.281436-1-longman@redhat.com?part=23