In the cpuset_css_online(), clearing the CS_SCHED_LOAD_BALANCE bit
of cs->flags is guarded by callback_lock and cpuset_mutex. There is
no problem with itself, because it is consistent with the description
of there two global lock at the beginning of this file. However, since
the operation of checking, setting and clearing the flag bit is atomic,
protection of callback_lock is unnecessary here, see CS_SPREAD_*. so
to make it more consistent with the other code, move the operation
outside the critical section of callback_lock.
No functional changes intended.
Signed-off-by: Xiu Jianfeng <[email protected]>
---
kernel/cgroup/cpuset.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index f9d2a3487645..315f8cbd6d35 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -4038,6 +4038,12 @@ static int cpuset_css_online(struct cgroup_subsys_state *css)
set_bit(CS_SPREAD_PAGE, &cs->flags);
if (is_spread_slab(parent))
set_bit(CS_SPREAD_SLAB, &cs->flags);
+ /*
+ * For v2, clear CS_SCHED_LOAD_BALANCE if parent is isolated
+ */
+ if (cgroup_subsys_on_dfl(cpuset_cgrp_subsys) &&
+ !is_sched_load_balance(parent))
+ clear_bit(CS_SCHED_LOAD_BALANCE, &cs->flags);
cpuset_inc();
@@ -4048,14 +4054,6 @@ static int cpuset_css_online(struct cgroup_subsys_state *css)
cs->use_parent_ecpus = true;
parent->child_ecpus_count++;
}
-
- /*
- * For v2, clear CS_SCHED_LOAD_BALANCE if parent is isolated
- */
- if (cgroup_subsys_on_dfl(cpuset_cgrp_subsys) &&
- !is_sched_load_balance(parent))
- clear_bit(CS_SCHED_LOAD_BALANCE, &cs->flags);
-
spin_unlock_irq(&callback_lock);
if (!test_bit(CGRP_CPUSET_CLONE_CHILDREN, &css->cgroup->flags))
--
2.34.1
On Sat, May 25, 2024 at 09:46:48AM +0000, Xiu Jianfeng wrote:
> In the cpuset_css_online(), clearing the CS_SCHED_LOAD_BALANCE bit
> of cs->flags is guarded by callback_lock and cpuset_mutex. There is
> no problem with itself, because it is consistent with the description
> of there two global lock at the beginning of this file. However, since
> the operation of checking, setting and clearing the flag bit is atomic,
> protection of callback_lock is unnecessary here, see CS_SPREAD_*. so
> to make it more consistent with the other code, move the operation
> outside the critical section of callback_lock.
>
> No functional changes intended.
>
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> kernel/cgroup/cpuset.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index f9d2a3487645..315f8cbd6d35 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -4038,6 +4038,12 @@ static int cpuset_css_online(struct cgroup_subsys_state *css)
> set_bit(CS_SPREAD_PAGE, &cs->flags);
> if (is_spread_slab(parent))
> set_bit(CS_SPREAD_SLAB, &cs->flags);
> + /*
> + * For v2, clear CS_SCHED_LOAD_BALANCE if parent is isolated
> + */
> + if (cgroup_subsys_on_dfl(cpuset_cgrp_subsys) &&
> + !is_sched_load_balance(parent))
> + clear_bit(CS_SCHED_LOAD_BALANCE, &cs->flags);
The code looks weird to me. It's doing the same thing under the
is_in_v2_mode() block and the cgroup_subsys_on_dfl() block and the former is
also run when the latter condition is true. Looks like we can get rid of the
latter block? Waiman?
Thanks.
--
tejun
On Fri, May 31, 2024 at 04:07:32PM -0400, Waiman Long wrote:
>
> On 5/26/24 14:35, Tejun Heo wrote:
> > On Sat, May 25, 2024 at 09:46:48AM +0000, Xiu Jianfeng wrote:
> > > In the cpuset_css_online(), clearing the CS_SCHED_LOAD_BALANCE bit
> > > of cs->flags is guarded by callback_lock and cpuset_mutex. There is
> > > no problem with itself, because it is consistent with the description
> > > of there two global lock at the beginning of this file. However, since
> > > the operation of checking, setting and clearing the flag bit is atomic,
> > > protection of callback_lock is unnecessary here, see CS_SPREAD_*. so
> > > to make it more consistent with the other code, move the operation
> > > outside the critical section of callback_lock.
> > >
> > > No functional changes intended.
> > >
> > > Signed-off-by: Xiu Jianfeng<[email protected]>
> > > ---
> > > kernel/cgroup/cpuset.c | 14 ++++++--------
> > > 1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> > > index f9d2a3487645..315f8cbd6d35 100644
> > > --- a/kernel/cgroup/cpuset.c
> > > +++ b/kernel/cgroup/cpuset.c
> > > @@ -4038,6 +4038,12 @@ static int cpuset_css_online(struct cgroup_subsys_state *css)
> > > set_bit(CS_SPREAD_PAGE, &cs->flags);
> > > if (is_spread_slab(parent))
> > > set_bit(CS_SPREAD_SLAB, &cs->flags);
> > > + /*
> > > + * For v2, clear CS_SCHED_LOAD_BALANCE if parent is isolated
> > > + */
> > > + if (cgroup_subsys_on_dfl(cpuset_cgrp_subsys) &&
> > > + !is_sched_load_balance(parent))
> > > + clear_bit(CS_SCHED_LOAD_BALANCE, &cs->flags);
> > The code looks weird to me. It's doing the same thing under the
> > is_in_v2_mode() block and the cgroup_subsys_on_dfl() block and the former is
> > also run when the latter condition is true. Looks like we can get rid of the
> > latter block? Waiman?
>
> Sorry for the late reply.
>
> The handling of the CS_SCHED_LOAD_BALANCE flag is different between v1 and
> v2. For v1, CS_SCHED_LOAD_BALANCE is default to ON unless it is explicitly
> turned off by writing to cpuset.sched_load_balance. For v2, the state will
> follow its parent when a new cpuset is brought online. Since is_in_v2_mode()
> can be on with cgroup v1, we can't group the two together.
>
> I agree that we don't need to protect the clearing of CS_SCHED_LOAD_BALANCE
> with the callback lock. So I don't have objection to this patch.
>
> Acked-by: Waiman Long <[email protected]>
Applied to cgroup/for-6.11.
Thanks.
--
tejun