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 08:27:58AM -0700 Yury Norov wrote:
> 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.
>

It's a specific edge case. I'd prefer to push for a forward solution
than revert.

> > 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.

As I said above let's try to go forward if we can. I'd argue that relying
on the old first cpu selection is not really an API, or documented so I
don't think a revert is needed.

I think a static key at the one or two places _distribute() is used
in the scheduler (and workqueue?) code would have the same effect as
this and be a better fit.


Cheers,
Phil

>
> Let's see what the folks will say. Please keep me in CC.
>
> Thanks,
> Yury
>

--


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