Re: [PATCH 07/24] sched/fair: Re-organize dequeue_task_fair()

Valentin Schneider
2024-08-12T10:02:45Z

On 11/08/24 00:17, Peter Zijlstra wrote:
> On Fri, Aug 09, 2024 at 06:53:30PM +0200, Valentin Schneider wrote:
>> On 27/07/24 12:27, Peter Zijlstra wrote:
>> > -static bool dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>> > +static int dequeue_entities(struct rq *rq, struct sched_entity *se, int flags)
>> > {
>> > - struct cfs_rq *cfs_rq;
>> > - struct sched_entity *se = &p->se;
>> > - int task_sleep = flags & DEQUEUE_SLEEP;
>> > - int idle_h_nr_running = task_has_idle_policy(p);
>> > bool was_sched_idle = sched_idle_rq(rq);
>> > int rq_h_nr_running = rq->cfs.h_nr_running;
>> > + bool task_sleep = flags & DEQUEUE_SLEEP;
>> > + struct task_struct *p = NULL;
>> > + int idle_h_nr_running = 0;
>> > + int h_nr_running = 0;
>> > + struct cfs_rq *cfs_rq;
>> >
>> > - util_est_dequeue(&rq->cfs, p);
>> > + if (entity_is_task(se)) {
>> > + p = task_of(se);
>> > + h_nr_running = 1;
>> > + idle_h_nr_running = task_has_idle_policy(p);
>> > + }
>> >
>>
>> This leaves the *h_nr_running to 0 for non-task entities. IIUC this makes
>> sense for ->sched_delayed entities (they should be empty of tasks), not so
>> sure for the other case. However, this only ends up being used for non-task
>> entities in:
>> - pick_next_entity(), if se->sched_delayed
>> - unregister_fair_sched_group()
>>
>> IIRC unregister_fair_sched_group() can only happen after the group has been
>> drained, so it would then indeed be empty of tasks, but I reckon this could
>> do with a comment/assert in dequeue_entities(), no? Or did I get too
>> confused by cgroups again?
>>
>
> Yeah, so I did have me a patch that made all this work for cfs bandwidth
> control as well. And then we need all this for throttled cgroup entries
> as well.
>
> Anyway... I had the patch, it worked, but then I remembered you were
> going to rewrite all that anyway and I was making a terrible mess of
> things, so I made it go away again.

Heh, sounds like someone needs to get back to it then :-)