[v1] sched/fair: Propagate load for throttled cfs_rq

[PATCH] sched/fair: Propagate load for throttled cfs_rq

Posted by Aaron Lu 3 months, 1 week ago

On Fri, Sep 05, 2025 at 02:53:28PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 05, 2025 at 07:37:19PM +0800, Aaron Lu wrote:
> > Hi Peter,
> > 
> > On Thu, Sep 04, 2025 at 03:04:07PM +0800, Aaron Lu wrote:
> > > On Thu, Sep 04, 2025 at 11:14:31AM +0530, K Prateek Nayak wrote:
> > > > On 9/4/2025 1:57 AM, Peter Zijlstra wrote:
> > > > > So this is mostly tasks leaving/joining the class/cgroup. And its
> > > > > purpose seems to be to remove/add the blocked load component.
> > > > > 
> > > > > Previously throttle/unthrottle would {de,en}queue the whole subtree from
> > > > > PELT, see how {en,de}queue would also stop at throttle.
> > > > > 
> > > > > But now none of that is done; PELT is fully managed by the tasks
> > > > > {de,en}queueing.
> > > > > 
> > > > > So I'm thinking that when a task joins fair (deboost from RT or
> > > > > whatever), we add the blocking load and fully propagate it. If the task
> > > > > is subject to throttling, that will then happen 'naturally' and it will
> > > > > dequeue itself again.
> > > > 
> > > > That seems like the correct thing to do yes. Those throttled_cfs_rq()
> > > > checks in propagate_entity_cfs_rq() can be removed then.
> > > >
> > > 
> > > Not sure if I understand correctly, I've come to the below code
> > > according to your discussion:
> > >
> > 
> > Does the below diff look sane to you? If so, shall I send a separate
> > patch on top or fold it in patch3 and send an updated patch3?
> 
> Yeah, I suppose that works. Please send a follow up patch. It would also
> be good to have a comment that explains why we need that list_add_leaf
> thing. I think I see, but I'm sure I'll have forgotten all next time I
> see this code.

Here it is.

From d88c2d2be31bb3970708f9b78e1725b0e25824be Mon Sep 17 00:00:00 2001
From: Aaron Lu <ziqianlu@bytedance.com>
Date: Fri, 5 Sep 2025 14:21:50 +0800
Subject: [PATCH] sched/fair: Propagate load for throttled cfs_rq

Before task based throttle model, propagating load will stop at a
throttled cfs_rq and that propagate will happen on unthrottle time by
update_load_avg().

Now that there is no update_load_avg() on unthrottle for throttled
cfs_rq and all load tracking is done by task related operations, let the
propagate happen immediately.

While at it, add a comment to explain why cfs_rqs that are not affected
by throttle have to be added to leaf cfs_rq list in
propagate_entity_cfs_rq() per my understanding of commit 0258bdfaff5b
("sched/fair: Fix unfairness caused by missing load decay").

Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
---
 kernel/sched/fair.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index df8dc389af8e1..03f16f70bff8a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5729,6 +5729,11 @@ static inline int cfs_rq_throttled(struct cfs_rq *cfs_rq)
 	return cfs_bandwidth_used() && cfs_rq->throttled;
 }
 
+static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
+{
+	return cfs_bandwidth_used() && cfs_rq->pelt_clock_throttled;
+}
+
 /* check whether cfs_rq, or any parent, is throttled */
 static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
 {
@@ -6721,6 +6726,11 @@ static inline int cfs_rq_throttled(struct cfs_rq *cfs_rq)
 	return 0;
 }
 
++static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
+{
+	return false;
+}
+
 static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
 {
 	return 0;
@@ -13151,10 +13161,13 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
 {
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 
-	if (cfs_rq_throttled(cfs_rq))
-		return;
-
-	if (!throttled_hierarchy(cfs_rq))
+	/*
+	 * If a task gets attached to this cfs_rq and before being queued,
+	 * it gets migrated to another CPU due to reasons like cpuset change,
+	 * we need to make sure this cfs_rq stays on leaf cfs_rq list to
+	 * have that removed load decayed or it can cause faireness problem.
+	 */
+	if(!cfs_rq_pelt_clock_throttled(cfs_rq))
 		list_add_leaf_cfs_rq(cfs_rq);
 
 	/* Start to propagate at parent */
@@ -13165,10 +13178,7 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
 
 		update_load_avg(cfs_rq, se, UPDATE_TG);
 
-		if (cfs_rq_throttled(cfs_rq))
-			break;
-
-		if (!throttled_hierarchy(cfs_rq))
+		if (!cfs_rq_pelt_clock_throttled(cfs_rq))
 			list_add_leaf_cfs_rq(cfs_rq);
 	}
 }
-- 
2.39.5

Re: [PATCH] sched/fair: Propagate load for throttled cfs_rq

Posted by kernel test robot 3 months, 1 week ago

Hi Aaron,

kernel test robot noticed the following build errors:



url:    https://github.com/intel-lab-lkp/linux/commits/UPDATE-20250908-190724/Aaron-Lu/sched-fair-Add-related-data-structure-for-task-based-throttle/20250829-161501
base:   the 3th patch of https://lore.kernel.org/r/20250829081120.806-4-ziqianlu%40bytedance.com
patch link:    https://lore.kernel.org/r/20250908110548.GA35%40bytedance
patch subject: [PATCH] sched/fair: Propagate load for throttled cfs_rq
config: i386-buildonly-randconfig-001-20250909 (https://download.01.org/0day-ci/archive/20250909/202509091233.f6nP3BVh-lkp@intel.com/config)
compiler: gcc-13 (Debian 13.3.0-16) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250909/202509091233.f6nP3BVh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509091233.f6nP3BVh-lkp@intel.com/

All errors (new ones prefixed by >>):

>> kernel/sched/fair.c:6747:1: error: expected identifier or '(' before '+' token
    6747 | +static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
         | ^


vim +6747 kernel/sched/fair.c

  6746	
> 6747	+static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
  6748	{
  6749		return false;
  6750	}
  6751	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] sched/fair: Propagate load for throttled cfs_rq

Posted by Aaron Lu 3 months, 1 week ago

On Tue, Sep 09, 2025 at 12:20:47PM +0800, kernel test robot wrote:
> Hi Aaron,
> 
> kernel test robot noticed the following build errors:
> 
> 
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/UPDATE-20250908-190724/Aaron-Lu/sched-fair-Add-related-data-structure-for-task-based-throttle/20250829-161501
> base:   the 3th patch of https://lore.kernel.org/r/20250829081120.806-4-ziqianlu%40bytedance.com
> patch link:    https://lore.kernel.org/r/20250908110548.GA35%40bytedance
> patch subject: [PATCH] sched/fair: Propagate load for throttled cfs_rq
> config: i386-buildonly-randconfig-001-20250909 (https://download.01.org/0day-ci/archive/20250909/202509091233.f6nP3BVh-lkp@intel.com/config)
> compiler: gcc-13 (Debian 13.3.0-16) 13.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250909/202509091233.f6nP3BVh-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202509091233.f6nP3BVh-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
> >> kernel/sched/fair.c:6747:1: error: expected identifier or '(' before '+' token
>     6747 | +static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
>          | ^

Sigh, I remembered I did a build test with !CFS_BANDWIDTH and now I went
to check that build directory and noticed I didn't have CFS_BANDWIDTH
disabled...

Sorry for the trouble, will send an updated patch later.

And thanks for the report, lkp.

> 
> 
> vim +6747 kernel/sched/fair.c
> 
>   6746	
> > 6747	+static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
>   6748	{
>   6749		return false;
>   6750	}
>   6751	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Re: [PATCH] sched/fair: Propagate load for throttled cfs_rq

Posted by K Prateek Nayak 3 months, 1 week ago

Hello Aaron,

On 9/9/2025 11:47 AM, Aaron Lu wrote:
>>>> kernel/sched/fair.c:6747:1: error: expected identifier or '(' before '+' token
>>     6747 | +static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
>>          | ^
> 
> Sigh, I remembered I did a build test with !CFS_BANDWIDTH and now I went
> to check that build directory and noticed I didn't have CFS_BANDWIDTH
> disabled...
> 
> Sorry for the trouble, will send an updated patch later.

While at it, another nit.

On 9/8/2025 4:35 PM, Aaron Lu wrote:
> @@ -13151,10 +13161,13 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
>  {
>  	struct cfs_rq *cfs_rq = cfs_rq_of(se);
>  
> -	if (cfs_rq_throttled(cfs_rq))
> -		return;
> -
> -	if (!throttled_hierarchy(cfs_rq))
> +	/*
> +	 * If a task gets attached to this cfs_rq and before being queued,
> +	 * it gets migrated to another CPU due to reasons like cpuset change,
> +	 * we need to make sure this cfs_rq stays on leaf cfs_rq list to
> +	 * have that removed load decayed or it can cause faireness problem.
> +	 */
> +	if(!cfs_rq_pelt_clock_throttled(cfs_rq))

          ^ Can you also add a space after the "if" here.

>  		list_add_leaf_cfs_rq(cfs_rq);

-- 
Thanks and Regards,
Prateek

Re: [PATCH] sched/fair: Propagate load for throttled cfs_rq

Posted by Aaron Lu 3 months, 1 week ago

On Tue, Sep 09, 2025 at 11:52:55AM +0530, K Prateek Nayak wrote:
> Hello Aaron,
> 
> On 9/9/2025 11:47 AM, Aaron Lu wrote:
> >>>> kernel/sched/fair.c:6747:1: error: expected identifier or '(' before '+' token
> >>     6747 | +static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
> >>          | ^
> > 
> > Sigh, I remembered I did a build test with !CFS_BANDWIDTH and now I went
> > to check that build directory and noticed I didn't have CFS_BANDWIDTH
> > disabled...
> > 
> > Sorry for the trouble, will send an updated patch later.
> 
> While at it, another nit.
> 
> On 9/8/2025 4:35 PM, Aaron Lu wrote:
> > @@ -13151,10 +13161,13 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
> >  {
> >  	struct cfs_rq *cfs_rq = cfs_rq_of(se);
> >  
> > -	if (cfs_rq_throttled(cfs_rq))
> > -		return;
> > -
> > -	if (!throttled_hierarchy(cfs_rq))
> > +	/*
> > +	 * If a task gets attached to this cfs_rq and before being queued,
> > +	 * it gets migrated to another CPU due to reasons like cpuset change,
> > +	 * we need to make sure this cfs_rq stays on leaf cfs_rq list to
> > +	 * have that removed load decayed or it can cause faireness problem.
> > +	 */
> > +	if(!cfs_rq_pelt_clock_throttled(cfs_rq))
> 
>           ^ Can you also add a space after the "if" here.
>

Yeah, I definitely should do that, thanks for catching this.

> >  		list_add_leaf_cfs_rq(cfs_rq);
>

Re: [PATCH] sched/fair: Propagate load for throttled cfs_rq

Posted by Aaron Lu 3 months, 1 week ago

On Tue, Sep 09, 2025 at 02:27:15PM +0800, Aaron Lu wrote:
> On Tue, Sep 09, 2025 at 11:52:55AM +0530, K Prateek Nayak wrote:
> > Hello Aaron,
> > 
> > On 9/9/2025 11:47 AM, Aaron Lu wrote:
> > >>>> kernel/sched/fair.c:6747:1: error: expected identifier or '(' before '+' token
> > >>     6747 | +static inline bool cfs_rq_pelt_clock_throttled(struct cfs_rq *cfs_rq)
> > >>          | ^
> > > 
> > > Sigh, I remembered I did a build test with !CFS_BANDWIDTH and now I went
> > > to check that build directory and noticed I didn't have CFS_BANDWIDTH
> > > disabled...
> > > 
> > > Sorry for the trouble, will send an updated patch later.
> > 
> > While at it, another nit.
> > 
> > On 9/8/2025 4:35 PM, Aaron Lu wrote:
> > > @@ -13151,10 +13161,13 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
> > >  {
> > >  	struct cfs_rq *cfs_rq = cfs_rq_of(se);
> > >  
> > > -	if (cfs_rq_throttled(cfs_rq))
> > > -		return;
> > > -
> > > -	if (!throttled_hierarchy(cfs_rq))
> > > +	/*
> > > +	 * If a task gets attached to this cfs_rq and before being queued,
> > > +	 * it gets migrated to another CPU due to reasons like cpuset change,
> > > +	 * we need to make sure this cfs_rq stays on leaf cfs_rq list to
> > > +	 * have that removed load decayed or it can cause faireness problem.
> > > +	 */
> > > +	if(!cfs_rq_pelt_clock_throttled(cfs_rq))
> > 
> >           ^ Can you also add a space after the "if" here.
> >
> 
> Yeah, I definitely should do that, thanks for catching this.
> 
> > >  		list_add_leaf_cfs_rq(cfs_rq);
> > 

Just a note that the updated patch is sent here:
https://lore.kernel.org/lkml/20250910095044.278-2-ziqianlu@bytedance.com/