Re: [PATCH 2/2] selftests/cgroup: Preserve CPU hotplug write errors

[Waiman Long <longman@redhat.com>]

On 8/10/26 11:25 PM, Rui Qi wrote:
> On 8/10/26 10:53 PM, Waiman Long wrote:
>> On 8/9/26 4:21 AM, Rui Qi wrote:
>>> The cpuset partition root state selftest checks several CPU hotplug
>>> transitions. If writing to a CPU online file fails, the helper still
>>> runs pause afterwards and returns the status of pause instead of the
>>> failed write.
>>>
>>> This hides the real hotplug failure and can make later checks run
>>> against expectations for a transition that never happened. Move the
>>> write before the bookkeeping and return when it fails, so callers can
>>> observe the hotplug error and the test does not record a CPU as offline
>>> unless the offline operation actually succeeded.
>>>
>>> Fixes: a8c52eba880a ("kselftest/cgroup: Add cpuset v2 partition root state test")
>>> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
>> I am aware that on x86-64, cpu0/online may not exist. Other than that,
>> under what condition will the write fail as only root is allowed to run
>> the script?
>>
>> Cheers,
>> Longman
> Hi Longman,
>
> Right, the failure I wanted to preserve is not the normal permission
> case. Running as root only gets us past the sysfs file permission check;
> the write can still fail in the CPU hotplug path.
>
> For example, cpu_down() can return -EBUSY when trying to offline the last
> online CPU, when CPU hotplug is temporarily disabled, or when offlining
> would leave no housekeeping CPU. cpu_up() can also fail if the CPU is not
> possible/present, if hotplug is disabled, or if the CPU is not bootable
> under the current SMT control state. Arch or registered cpuhp callbacks
> may also reject the transition and return an error.
>
> The current helper hides all of those cases because it does the write and
> then calls pause(), so the function returns the status of pause rather
> than the failed sysfs write. It also updates OFFLINE_CPUS before knowing
> whether the offline request succeeded. So this change is mainly to avoid
> continuing the test with bookkeeping that says a CPU was offlined when the
> kernel actually rejected the hotplug operation.

Thank for the clarification. You are right that cpu_down() can be 
blocked in the hotplug path. That should rarely happen, but I agree that 
we catch that special case too.

Cheers,
Longman

>
> Thanks,
> Rui
>
>>> ---
>>>    tools/testing/selftests/cgroup/test_cpuset_prs.sh | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/cgroup/test_cpuset_prs.sh b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>>> index 16c38eb11e63..a2f7208633bc 100755
>>> --- a/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>>> +++ b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>>> @@ -412,6 +412,7 @@ write_cpu_online()
>>>    	CPU=${1%=*}
>>>    	VAL=${1#*=}
>>>    	CPUFILE=//sys/devices/system/cpu/cpu${CPU}/online
>>> +	echo $VAL > $CPUFILE || return 1
>>>    	if [[ $VAL -eq 0 ]]
>>>    	then
>>>    		OFFLINE_CPUS="$OFFLINE_CPUS $CPU"
>>> @@ -421,7 +422,6 @@ write_cpu_online()
>>>    					sort | uniq -u)
>>>    		}
>>>    	fi
>>> -	echo $VAL > $CPUFILE
>>>    	pause 0.05
>>>    }
>>>    
>>> @@ -493,6 +493,7 @@ set_ctrl_state()
>>>    			eval $COMM $REDIRECT
>>>    			;;
>>>    		    O*) VAL=${CMD#?}
>>> +			COMM="write_cpu_online $VAL"
>>>    			write_cpu_online $VAL
>>>    			;;
>>>    		    T*) COMM="echo 0 > $TFILE"