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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> >
> 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
