Received-SPF: pass (google.com: domain of linux-kernel+bounces-165501-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 1 May 2024 11:39:11 -0400
From: Phil Auld <pauld@redhat.com>
To: Yury Norov <yury.norov@gmail.com>
Cc: Ankit Jain <ankit-aj.jain@broadcom.com>, linux@rasmusvillemoes.dk,
	akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
	juri.lelli@redhat.com, ajay.kaher@broadcom.com,
	alexey.makhalov@broadcom.com, vasavi.sirnapalli@broadcom.com,
	Paul Turner <pjt@google.com>, Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Valentin Schneider <vschneid@redhat.com>
Subject: Re: [PATCH] lib/cpumask: Boot option to disable tasks distribution
 within cpumask
Message-ID: <20240501153911.GD39737@lorien.usersys.redhat.com>
References: <20240430090431.1619622-1-ankit-aj.jain@broadcom.com>
 <ZjE3C9UgeZR02Jyy@yury-ThinkPad>
 <20240501133608.GB39737@lorien.usersys.redhat.com>
 <ZjJfftWMbjT9r8iT@yury-ThinkPad>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjJfftWMbjT9r8iT@yury-ThinkPad>

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
> 

--