Re: [PATCH -next] cgroup/cpuset: Do not clear xcpus when clearing cpus

[Saket Kumar Bhaskar <skb99@linux.ibm.com>]

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