| Message ID | 20240731092102.2369580-1-chenridong@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [-next] cgroup/cpuset: Do not clear xcpus when clearing cpus | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 7/31/24 05:21, Chen Ridong wrote: > After commit 737bb142a00d ("cgroup/cpuset: Make cpuset.cpus.exclusive > independent of cpuset.cpus"), cpuset.cpus.exclusive and cpuset.cpus > became independent. However we found that cpuset.cpus.exclusive.effective > is cleared when cpuset.cpus is clear. To fix this issue, just remove xcpus > clearing when cpuset.cpus is being cleared. > > It can be reproduced as below: > cd /sys/fs/cgroup/ > mkdir test > echo +cpuset > cgroup.subtree_control > cd test > echo 3 > cpuset.cpus.exclusive > cat cpuset.cpus.exclusive.effective > 3 > echo > cpuset.cpus > cat cpuset.cpus.exclusive.effective // was cleared > > Signed-off-by: Chen Ridong <chenridong@huawei.com> > --- > kernel/cgroup/cpuset.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c > index a9b6d56eeffa..248c39bebbe9 100644 > --- a/kernel/cgroup/cpuset.c > +++ b/kernel/cgroup/cpuset.c > @@ -2523,10 +2523,9 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs, > * that parsing. The validate_change() call ensures that cpusets > * with tasks have cpus. > */ > - if (!*buf) { > + if (!*buf) > cpumask_clear(trialcs->cpus_allowed); > - cpumask_clear(trialcs->effective_xcpus); > - } else { > + else { > retval = cpulist_parse(buf, trialcs->cpus_allowed); > if (retval < 0) > return retval; Yes, that is a corner case bug that has not been properly handled. Reviewed-by: Waiman Long <longman@redhat.com>
On 7/31/24 23:22, Waiman Long wrote: > On 7/31/24 05:21, Chen Ridong wrote: >> After commit 737bb142a00d ("cgroup/cpuset: Make cpuset.cpus.exclusive >> independent of cpuset.cpus"), cpuset.cpus.exclusive and cpuset.cpus >> became independent. However we found that >> cpuset.cpus.exclusive.effective >> is cleared when cpuset.cpus is clear. To fix this issue, just remove >> xcpus >> clearing when cpuset.cpus is being cleared. >> >> It can be reproduced as below: >> cd /sys/fs/cgroup/ >> mkdir test >> echo +cpuset > cgroup.subtree_control >> cd test >> echo 3 > cpuset.cpus.exclusive >> cat cpuset.cpus.exclusive.effective >> 3 >> echo > cpuset.cpus >> cat cpuset.cpus.exclusive.effective // was cleared >> >> Signed-off-by: Chen Ridong <chenridong@huawei.com> >> --- >> kernel/cgroup/cpuset.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c >> index a9b6d56eeffa..248c39bebbe9 100644 >> --- a/kernel/cgroup/cpuset.c >> +++ b/kernel/cgroup/cpuset.c >> @@ -2523,10 +2523,9 @@ static int update_cpumask(struct cpuset *cs, >> struct cpuset *trialcs, >> * that parsing. The validate_change() call ensures that cpusets >> * with tasks have cpus. >> */ >> - if (!*buf) { >> + if (!*buf) >> cpumask_clear(trialcs->cpus_allowed); >> - cpumask_clear(trialcs->effective_xcpus); >> - } else { >> + else { >> retval = cpulist_parse(buf, trialcs->cpus_allowed); >> if (retval < 0) >> return retval; > > Yes, that is a corner case bug that has not been properly handled. > > Reviewed-by: Waiman Long <longman@redhat.com> > With a second thought, I think we should keep the clearing of effective_xcpus if exclusive_cpus is empty. IOW diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index 6ba8313f1fc3..2023cd68d9bc 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -2516,7 +2516,8 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs, */ if (!*buf) { cpumask_clear(trialcs->cpus_allowed); - cpumask_clear(trialcs->effective_xcpus); + if (cpumask_empty(trialcs->exclusive_cpus)) + cpumask_clear(trialcs->effective_xcpus); } else { retval = cpulist_parse(buf, trialcs->cpus_allowed); if (retval < 0) Thanks, Longman
On Thu, Aug 01, 2024 at 12:31:44PM -0400, Waiman Long wrote: > > On 7/31/24 23:22, Waiman Long wrote: > > On 7/31/24 05:21, Chen Ridong wrote: > > > After commit 737bb142a00d ("cgroup/cpuset: Make cpuset.cpus.exclusive > > > independent of cpuset.cpus"), cpuset.cpus.exclusive and cpuset.cpus > > > became independent. However we found that > > > cpuset.cpus.exclusive.effective > > > is cleared when cpuset.cpus is clear. To fix this issue, just remove > > > xcpus > > > clearing when cpuset.cpus is being cleared. > > > > > > It can be reproduced as below: > > > cd /sys/fs/cgroup/ > > > mkdir test > > > echo +cpuset > cgroup.subtree_control > > > cd test > > > echo 3 > cpuset.cpus.exclusive > > > cat cpuset.cpus.exclusive.effective > > > 3 > > > echo > cpuset.cpus > > > cat cpuset.cpus.exclusive.effective // was cleared > > > > > > Signed-off-by: Chen Ridong <chenridong@huawei.com> > > > --- > > > kernel/cgroup/cpuset.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c > > > index a9b6d56eeffa..248c39bebbe9 100644 > > > --- a/kernel/cgroup/cpuset.c > > > +++ b/kernel/cgroup/cpuset.c > > > @@ -2523,10 +2523,9 @@ static int update_cpumask(struct cpuset *cs, > > > struct cpuset *trialcs, > > > * that parsing. The validate_change() call ensures that cpusets > > > * with tasks have cpus. > > > */ > > > - if (!*buf) { > > > + if (!*buf) > > > cpumask_clear(trialcs->cpus_allowed); > > > - cpumask_clear(trialcs->effective_xcpus); > > > - } else { > > > + else { > > > retval = cpulist_parse(buf, trialcs->cpus_allowed); > > > if (retval < 0) > > > return retval; > > > > Yes, that is a corner case bug that has not been properly handled. > > > > Reviewed-by: Waiman Long <longman@redhat.com> > > > With a second thought, I think we should keep the clearing of > effective_xcpus if exclusive_cpus is empty. IOW > > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c > index 6ba8313f1fc3..2023cd68d9bc 100644 > --- a/kernel/cgroup/cpuset.c > +++ b/kernel/cgroup/cpuset.c > @@ -2516,7 +2516,8 @@ static int update_cpumask(struct cpuset *cs, struct > cpuset *trialcs, > */ > if (!*buf) { > cpumask_clear(trialcs->cpus_allowed); > - cpumask_clear(trialcs->effective_xcpus); > + if (cpumask_empty(trialcs->exclusive_cpus)) > + cpumask_clear(trialcs->effective_xcpus); > } else { > retval = cpulist_parse(buf, trialcs->cpus_allowed); > if (retval < 0) > > Thanks, > Longman > Hi Longman, Is there any situation in which we could land here for or after clearing exclusive_cpus. AFAIK only way we could landup after clearing exclusive_cpus to update_exclusive_cpumask(), which anyway clears effective_xcpus. In that case, clearing effective_xcpus would be redundant in update_cpumask(). Also, is there any situation in which we could end up clearing exclusive_cpus without clearing effective_xcpus as we have a piece of code: static inline struct cpumask *fetch_xcpus(struct cpuset *cs) { return !cpumask_empty(cs->exclusive_cpus) ? cs->exclusive_cpus : cpumask_empty(cs->effective_xcpus) ? cs->cpus_allowed : cs->effective_xcpus; } Thanks, Saket
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index a9b6d56eeffa..248c39bebbe9 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -2523,10 +2523,9 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs, * that parsing. The validate_change() call ensures that cpusets * with tasks have cpus. */ - if (!*buf) { + if (!*buf) cpumask_clear(trialcs->cpus_allowed); - cpumask_clear(trialcs->effective_xcpus); - } else { + else { retval = cpulist_parse(buf, trialcs->cpus_allowed); if (retval < 0) return retval;
After commit 737bb142a00d ("cgroup/cpuset: Make cpuset.cpus.exclusive independent of cpuset.cpus"), cpuset.cpus.exclusive and cpuset.cpus became independent. However we found that cpuset.cpus.exclusive.effective is cleared when cpuset.cpus is clear. To fix this issue, just remove xcpus clearing when cpuset.cpus is being cleared. It can be reproduced as below: cd /sys/fs/cgroup/ mkdir test echo +cpuset > cgroup.subtree_control cd test echo 3 > cpuset.cpus.exclusive cat cpuset.cpus.exclusive.effective 3 echo > cpuset.cpus cat cpuset.cpus.exclusive.effective // was cleared Signed-off-by: Chen Ridong <chenridong@huawei.com> --- kernel/cgroup/cpuset.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)