Re: [PATCH -next 1/4] cpuset: remove redundant CS_ONLINE flag

[Waiman Long <llong@redhat.com>]

On 8/12/25 9:20 PM, Chen Ridong wrote:
>
> On 2025/8/13 9:00, Waiman Long wrote:
>> On 8/12/25 8:54 PM, Chen Ridong wrote:
>>> On 2025/8/12 22:44, Waiman Long wrote:
>>>> On 8/8/25 5:25 AM, Chen Ridong wrote:
>>>>> From: Chen Ridong <chenridong@huawei.com>
>>>>>
>>>>> The CS_ONLINE flag was introduced prior to the CSS_ONLINE flag in the
>>>>> cpuset subsystem. Currently, the flag setting sequence is as follows:
>>>>>
>>>>> 1. cpuset_css_online() sets CS_ONLINE
>>>>> 2. css->flags gets CSS_ONLINE set
>>>>> ...
>>>>> 3. cgroup->kill_css sets CSS_DYING
>>>>> 4. cpuset_css_offline() clears CS_ONLINE
>>>>> 5. css->flags clears CSS_ONLINE
>>>>>
>>>>> The is_cpuset_online() check currently occurs between steps 1 and 3.
>>>>> However, it would be equally safe to perform this check between steps 2
>>>>> and 3, as CSS_ONLINE provides the same synchronization guarantee as
>>>>> CS_ONLINE.
>>>>>
>>>>> Since CS_ONLINE is redundant with CSS_ONLINE and provides no additional
>>>>> synchronization benefits, we can safely remove it to simplify the code.
>>>>>
>>>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>>>>> ---
>>>>>     include/linux/cgroup.h          | 5 +++++
>>>>>     kernel/cgroup/cpuset-internal.h | 3 +--
>>>>>     kernel/cgroup/cpuset.c          | 4 +---
>>>>>     3 files changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>>>>> index b18fb5fcb38e..ae73dbb19165 100644
>>>>> --- a/include/linux/cgroup.h
>>>>> +++ b/include/linux/cgroup.h
>>>>> @@ -354,6 +354,11 @@ static inline bool css_is_dying(struct cgroup_subsys_state *css)
>>>>>         return css->flags & CSS_DYING;
>>>>>     }
>>>>>     +static inline bool css_is_online(struct cgroup_subsys_state *css)
>>>>> +{
>>>>> +    return css->flags & CSS_ONLINE;
>>>>> +}
>>>>> +
>>>>>     static inline bool css_is_self(struct cgroup_subsys_state *css)
>>>>>     {
>>>>>         if (css == &css->cgroup->self) {
>>>>> diff --git a/kernel/cgroup/cpuset-internal.h b/kernel/cgroup/cpuset-internal.h
>>>>> index 383963e28ac6..75b3aef39231 100644
>>>>> --- a/kernel/cgroup/cpuset-internal.h
>>>>> +++ b/kernel/cgroup/cpuset-internal.h
>>>>> @@ -38,7 +38,6 @@ enum prs_errcode {
>>>>>       /* bits in struct cpuset flags field */
>>>>>     typedef enum {
>>>>> -    CS_ONLINE,
>>>>>         CS_CPU_EXCLUSIVE,
>>>>>         CS_MEM_EXCLUSIVE,
>>>>>         CS_MEM_HARDWALL,
>>>>> @@ -202,7 +201,7 @@ static inline struct cpuset *parent_cs(struct cpuset *cs)
>>>>>     /* convenient tests for these bits */
>>>>>     static inline bool is_cpuset_online(struct cpuset *cs)
>>>>>     {
>>>>> -    return test_bit(CS_ONLINE, &cs->flags) && !css_is_dying(&cs->css);
>>>>> +    return css_is_online(&cs->css) && !css_is_dying(&cs->css);
>>>>>     }
>>>>>       static inline int is_cpu_exclusive(const struct cpuset *cs)
>>>>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>>>>> index f74d04429a29..cf7cd2255265 100644
>>>>> --- a/kernel/cgroup/cpuset.c
>>>>> +++ b/kernel/cgroup/cpuset.c
>>>>> @@ -207,7 +207,7 @@ static inline void notify_partition_change(struct cpuset *cs, int old_prs)
>>>>>      * parallel, we may leave an offline CPU in cpu_allowed or some other masks.
>>>>>      */
>>>>>     static struct cpuset top_cpuset = {
>>>>> -    .flags = BIT(CS_ONLINE) | BIT(CS_CPU_EXCLUSIVE) |
>>>>> +    .flags = BIT(CS_CPU_EXCLUSIVE) |
>>>>>              BIT(CS_MEM_EXCLUSIVE) | BIT(CS_SCHED_LOAD_BALANCE),
>>>>>         .partition_root_state = PRS_ROOT,
>>>>>         .relax_domain_level = -1,
>>>> top_cpuset.css is not initialized like the one in the children. If you modify is_cpuset_online() to
>>>> test the css.flags, you will probably need to set the CSS_ONLINE flag in top_cpuset.css.flags. I do
>>>> doubt that we will apply the is_cpuset_online() test on top_cpuset. To be consistent, we should
>>>> support that.
>>>>
>>>> BTW, other statically allocated css'es in the cgroup root may have similar problem. If you make the
>>>> css_is_online() helper available to all other controllers, you will have to document that
>>>> limitation.
>>>>
>>>> Cheers,
>>>> Longman
>>> Hi, Longman, thank you for your response.
>>>
>>> If I understand correctly, the CSS_ONLINE flag should be set in top_cpuset.css during the following
>>> process:
>>>
>>> css_create
>>>     css = ss->css_alloc(parent_css);  // cgroup root is static, unlike children
>>>     online_css(css);
>>>        ret = ss->css_online(css);     // css root may differ from children
>>>        css->flags |= CSS_ONLINE;      // css.flags is set with CSS_ONLINE, including the root css
>>>
>>> I think css online must be successful, and it's CSS_ONLINE flag must be set. Do I missing anything?
>> I am talking about just the top_cpuset which is statically allocated. It is not created by the
>> css_create() call and so the CSS_ONLINE will not be set.
>>
>> Cheers,
>> Longman
> Hi Longman,
>
> Apologies for the call stack earlier. Thank you for your patience in clarifying this matter.
>
> The CSS root is brought online through the following initialization flow:
>
> cgroup_init_subsys
>    css = ss->css_alloc(NULL);       // css root is static, unlike children
>    online_css(css)
>      ret = ss->css_online(css);     // css root may differ from children
>      css->flags |= CSS_ONLINE;      // css.flags is set with CSS_ONLINE, including the root css
>
> My key point is that:
> - The root CSS should be online by design.
> - Root css CSS_ONLINE flag should be properly set during initialization.

Yes, you are right. I missed css_online() call for the root css for each 
controller. Thanks for the clarification.

With that, I am OK with this patch. Though the other ones are not good.

Acked-by: Waiman Long <longman@redhat.com>