Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 8 Sep 2023 08:45:01 -0400
From:   Phil Auld <pauld@redhat.com>
To:     Hao Jia <jiahao.os@bytedance.com>
Cc:     mingo@redhat.com, peterz@infradead.org, mingo@kernel.org,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [External] Re: [PATCH 0/2] Fix nohz_full vs rt bandwidth
Message-ID: <20230908124501.GA471894@lorien.usersys.redhat.com>
References: <20230821094927.51079-1-jiahao.os@bytedance.com>
 <1823b6fd-037f-38dc-2d33-0879d77768c3@bytedance.com>
 <20230907141703.GA441901@lorien.usersys.redhat.com>
 <bd786914-4718-7a35-fcc1-9cdee58d2f06@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bd786914-4718-7a35-fcc1-9cdee58d2f06@bytedance.com>
Precedence: bulk

On Fri, Sep 08, 2023 at 10:57:26AM +0800 Hao Jia wrote:
> 
> 
> On 2023/9/7 Phil Auld wrote:
> > Hi Hao,
> > 
> > On Wed, Sep 06, 2023 at 02:45:39PM +0800 Hao Jia wrote:
> > > 
> > > Friendly ping...
> > > 
> > > On 2023/8/21 Hao Jia wrote:
> > > > Since the commit 88c56cfeaec4 ("sched/fair: Block nohz tick_stop
> > > > when cfs bandwidth in use") was merged, it handles conflicts between
> > > > NOHZ full and cfs_bandwidth well, and the scheduler feature HZ_BW
> > > > allows us to choose which one to prefer.
> > > > 
> > > > This conflict also exists between NOHZ full and rt_bandwidth,
> > > > these two patches try to handle it in a similar way.
> > > > 
> > 
> > Are you actually hitting this in the real world?
> > 
> > We, for example, no longer enable RT_GROUP_SCHED so this is a non-issue
> > for our use cases.  I'd recommend considering that. (Does it even
> > work with cgroup2?)
> > 
> 
> Yes, it has always been there. Regardless of whether RT_GROUP_SCHED is
> enabled or not, rt bandwidth is always enabled. If RT_GROUP_SCHED is not
> enabled, all rt tasks in the system are a group, and rt_runtime is 950000,
> and rt_period is 1000000.So rt bandwidth is always enabled by default.

Sure, there is that. But I think Daniel is actively trying to remove it.

Also I'm not sure you answered my question. Are you actually hitting this
in the real world?  I'd be tempted to think this is a mis-configuration or
mis-use of RT.  Plus you can disable that throttling and use stalld to catch
cases where the rt task goes out of control.

I'm not totally against doing this (for what my vote counts...), I just
wonder if it's really needed. It seem it may be over-engineering something
that is soon to be a non-problem. 


Cheers,
Phil

> 
> 
> Thanks,
> Hao
> 
> > In some ways what you have is a simplification of code, but it also
> > obfuscates the stop_tick conditions by hiding them all in the class
> > specific functions.  It was easier to see why the tick didn't stop
> > looking at the original code.
> > 
> > It would be better to do this only if it is really needed, in my opinion.
> > 
> > 
> > Cheers,
> > Phil
> > 
> > > > patch1: Extracts a can_stop_tick() callback function for each
> > > > sched_class from sched_can_stop_tick(), it will make things clearer
> > > > and also convenient to handle the conflict between NOHZ full
> > > > and rt_bandwidth.
> > > > 
> > > > patch2: If the HZ_BW scheduler feature is enabled, and the RT task
> > > > to be run is constrained by rt_bandwidth runtime. Then it will
> > > > prevent NO_HZ full from stopping tick.
> > > > 
> > > > Hao Jia (2):
> > > >     sched/core: Introduce sched_class::can_stop_tick()
> > > >     sched/rt: Block nohz tick_stop when rt bandwidth in use
> > > > 
> > > >    kernel/sched/core.c     | 67 +++++--------------------------
> > > >    kernel/sched/deadline.c | 16 ++++++++
> > > >    kernel/sched/fair.c     | 56 +++++++++++++++++++++++---
> > > >    kernel/sched/rt.c       | 89 ++++++++++++++++++++++++++++++++++++++++-
> > > >    kernel/sched/sched.h    |  5 ++-
> > > >    5 files changed, 168 insertions(+), 65 deletions(-)
> > > > 
> > > 
> > 
> 

--