lkml.org 
[lkml]   [2024]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] lib/cpumask: Boot option to disable tasks distribution within cpumask
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

\
 
 \ /
  Last update: 2024-05-27 18:11    [W:0.092 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site