Re: [PATCH] lib/cpumask: Boot option to disable tasks distribution within cpumask

[Yury Norov <yury.norov@gmail.com>]

On Wed, May 01, 2024 at 09:36:08AM -0400, Phil Auld wrote:
> 
> Hi Yuri,

[...]
 
> > Not that I'm familiar to your setup, but this sounds like a userspace
> > configuration problems. Can you try to move your non-RT tasks into a
> > cgroup attached to non-RT CPUs, or something like that? 
> >
> 
> It's not really. In a container environment just logging in to the
> container could end up with the exec'd task landing on one of
> the polling or latency sensitive cores.
> 
> In a telco deployment the applications will run containers with
> isolated(pinned) cpus with load balacning disabled.  These
> containers typically use one of these cpus for its "housekeeping"
> with the remainder used for the latency sensitive workloads.
> 
> Also, this is a change in kernel behavior which is breaking
> userspace.

Alright, that's a different story.

> We are also hitting this and are interested in a way to get the
> old behavior back for some workloads.
> 
> > > With the introduction of kernel cmdline param 'sched_pick_firstcpu',
> > > there is an option provided for such usecases to disable the distribution
> > > of tasks within the cpumask logic and use the previous 'pick first cpu'
> > > approach for initial placement of tasks. Because many telco vendors
> > > configure the system in such a way that the first cpu within a cpuset
> > > of pod doesn't run any SCHED_FIFO or High priority tasks.
> > > 
> > > Co-developed-by: Alexey Makhalov <alexey.makhalov@broadcom.com>
> > > Signed-off-by: Alexey Makhalov <alexey.makhalov@broadcom.com>
> > > Signed-off-by: Ankit Jain <ankit-aj.jain@broadcom.com>
> > > ---
> > >  lib/cpumask.c | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/lib/cpumask.c b/lib/cpumask.c
> > > index e77ee9d46f71..3dea87d5ec1f 100644
> > > --- a/lib/cpumask.c
> > > +++ b/lib/cpumask.c
> > > @@ -154,6 +154,23 @@ unsigned int cpumask_local_spread(unsigned int i, int node)
> > >  }
> > >  EXPORT_SYMBOL(cpumask_local_spread);
> > >  
> > > +/*
> > > + * Task distribution within the cpumask feature disabled?
> > > + */
> > > +static bool cpumask_pick_firstcpu __read_mostly;
> > > +
> > > +/*
> > > + * Disable Tasks distribution within the cpumask feature
> > > + */
> > > +static int __init cpumask_pick_firstcpu_setup(char *str)
> > > +{
> > > +	cpumask_pick_firstcpu = 1;
> > > +	pr_info("cpumask: Tasks distribution within cpumask is disabled.");
> > > +	return 1;
> > > +}
> > > +
> > > +__setup("sched_pick_firstcpu", cpumask_pick_firstcpu_setup);
> > > +
> > >  static DEFINE_PER_CPU(int, distribute_cpu_mask_prev);
> > >  
> > >  /**
> > > @@ -171,6 +188,13 @@ unsigned int cpumask_any_and_distribute(const struct cpumask *src1p,
> > >  {
> > >  	unsigned int next, prev;
> > >  
> > > +	/*
> > > +	 * Don't distribute, if tasks distribution
> > > +	 * within cpumask feature is disabled
> > > +	 */
> > > +	if (cpumask_pick_firstcpu)
> > > +		return cpumask_any_and(src1p, src2p);
> > 
> > No, this is a wrong way.
> > 
> > To begin with, this parameter shouldn't control a single random
> > function. At least, the other cpumask_*_distribute() should be
> > consistent to the policy.
> > 
> > But in general... I don't think we should do things like that at all.
> > Cpumask API is a simple and plain wrapper around bitmaps. If you want
> > to modify a behavior of the scheduler, you could do that at scheduler
> > level, not in a random helper function.
> > 
> > Consider 2 cases:
> >  - Someone unrelated to scheduler would use the same helper and will
> >    be affected by this parameter inadvertently.
> >  - Scheduler will switch to using another function to distribute CPUs,
> >    and your setups will suddenly get broken again. This time deeply in
> >    production.
> >
> 
> Yeah, I think I agree with this part.  At the scheduler level, where this
> is called, makes more sense. 
> 
> Note, this is "deeply in production" now...

So, if we all agree that touching cpumasks is a bad idea, let's drop
this patch and try figuring out a better solution.

Now that you're saying the scheduler patches break userspace, I think
it would be legitimate to revert them, unless there's a simple fix for
that.

Let's see what the folks will say. Please keep me in CC.

Thanks,
Yury