Messages in this thread | | | Date | Wed, 1 May 2024 11:39:11 -0400 | From | Phil Auld <> | Subject | Re: [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 >
--
| |