Re: [PATCH v4 3/5] sched/fair: Switch to task based throttle model

[Aaron Lu]

On Mon, Sep 08, 2025 at 08:58:09PM -0700, Benjamin Segall wrote:
> Aaron Lu <ziqianlu@xxxxxxxxxxxxx> writes:
> 
> > On Thu, Sep 04, 2025 at 03:21:06PM +0530, K Prateek Nayak wrote:
> >> Hello Aaron,
> >> 
> >> On 9/4/2025 1:46 PM, Aaron Lu wrote:
> >> > @@ -8722,15 +8730,6 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
> >> >  	if (unlikely(se == pse))
> >> >  		return;
> >> >  
> >> > -	/*
> >> > -	 * This is possible from callers such as attach_tasks(), in which we
> >> > -	 * unconditionally wakeup_preempt() after an enqueue (which may have
> >> > -	 * lead to a throttle).  This both saves work and prevents false
> >> > -	 * next-buddy nomination below.
> >> > -	 */
> >> > -	if (unlikely(throttled_hierarchy(cfs_rq_of(pse))))
> >> > -		return;
> >> 
> >> I think we should have a:
> >> 
> >> 	if (task_is_throttled(p))
> >> 		return;
> >> 
> >> here. I can see at least one possibility via prio_changed_fair()
> >
> > Ah right. I didn't realize wakeup_preempt() can be called for a throttled
> > task, I think it is not expected. What about forbid that :)
> > (not tested in anyway, just to show the idea and get feedback)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index cb93e74a850e8..f1383aede764f 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -13135,7 +13135,11 @@ static void task_fork_fair(struct task_struct *p)
> >  static void
> >  prio_changed_fair(struct rq *rq, struct task_struct *p, int oldprio)
> >  {
> > -	if (!task_on_rq_queued(p))
> > +	/*
> > +	 * p->on_rq can be set for throttled task but there is no need to
> > +	 * check wakeup preempt for throttled task, so use p->se.on_rq instead.
> > +	 */
> > +	if (!p->se.on_rq)
> >  		return;
> >  
> >  	if (rq->cfs.nr_queued == 1)
> >
> >> where a throttled task might reach here. Rest looks good. I'll
> >> still wait on Ben for the update_cfs_group() bits :)
> 
> 
> Yeah, I think I agree with all of these (this patch and the previous
> patch); the preempt ones are subjective but I'd probably default to "no
> special case needed for throttle". Removing the check in
> update_cfs_group() I think is correct, unless we want to freeze
> everything, yeah. (And that seems dangerous in its own way)

Thanks for all these info, let me go ahead and send these changes for
review then :)