Re: [PATCH v2 1/2] cgroup/cpuset: reset DL migration state on can_attach() failure

[Waiman Long <longman@redhat.com>]

On 5/7/26 10:14 PM, Chen Ridong wrote:
>
> On 2026/5/7 22:31, Waiman Long wrote:
>> On 5/7/26 6:33 AM, Guopeng Zhang wrote:
>>> cpuset_can_attach() accumulates temporary SCHED_DEADLINE migration
>>> state in the destination cpuset while walking the taskset.
>>>
>>> If a later task_can_attach() or security_task_setscheduler() check
>>> fails, cgroup_migrate_execute() treats cpuset as the failing subsystem
>>> and does not call cpuset_cancel_attach() for it. The partially
>>> accumulated state is then left behind and can be consumed by a later
>>> attach, corrupting cpuset DL task accounting and pending DL bandwidth
>>> accounting.
>>>
>>> Reset the pending DL migration state before returning from those
>>> per-task failure paths.
>>>
>>> Fixes: 2ef269ef1ac0 ("cgroup/cpuset: Free DL BW in case can_attach() fails")
>>> Signed-off-by: Guopeng Zhang <zhangguopeng@kylinos.cn>
>>> ---
>>>    kernel/cgroup/cpuset.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>>> index e3a081a07c6d..ae41736399a1 100644
>>> --- a/kernel/cgroup/cpuset.c
>>> +++ b/kernel/cgroup/cpuset.c
>>> @@ -3029,12 +3029,12 @@ static int cpuset_can_attach(struct cgroup_taskset *tset)
>>>        cgroup_taskset_for_each(task, css, tset) {
>>>            ret = task_can_attach(task);
>>>            if (ret)
>>> -            goto out_unlock;
>>> +            goto out_reset_dl_data;
>>>              if (setsched_check) {
>>>                ret = security_task_setscheduler(task);
>>>                if (ret)
>>> -                goto out_unlock;
>>> +                goto out_reset_dl_data;
>>>            }
>>>              if (dl_task(task)) {
>>> @@ -3070,6 +3070,10 @@ static int cpuset_can_attach(struct cgroup_taskset *tset)
>>>         * changes which zero cpus/mems_allowed.
>>>         */
>>>        cs->attach_in_progress++;
>>> +    goto out_unlock;
>>> +
>>> +out_reset_dl_data:
>>> +    reset_migrate_dl_data(cs);
>>>    out_unlock:
>>>        mutex_unlock(&cpuset_mutex);
>>>        return ret;
>> I would prefer the likely success path be a straight line instead of doing a
>> goto. IOW, move out_reset_dl_data below return. Other than that, this patch
>> looks good to me.
>>
> I've read the code and found several places that call reset_migrate_dl_data(cs).
>
> I think it would be better to call reset_migrate_dl_data(cs) only when we
> encounter an error, for example:
>
> ```
> static int cpuset_can_attach(struct cgroup_taskset *tset)
> {
> ...
> out_unlock:
> 	if (ret)
> 		reset_migrate_dl_data(cs);
> 	mutex_unlock(&cpuset_mutex);
> 	return ret;
> }
> ```
> After that, no other places would need to call reset_migrate_dl_data(cs), right?
>
Yes, that should work too.

Cheers,
Longman