Re: [PATCH 0/4] Task based throttle follow ups

[Benjamin Segall <bsegall@google.com>]

Aaron Lu <ziqianlu@bytedance.com> writes:

> Peter noticed the inconsistency in load propagation for throttled cfs_rq
> and Ben pointed out several other places regarding throttled cfs_rq that
> could be no longer needed after task based throttle model.
>
> To ease discussing and reviewing, I've come up with this follow up
> series which implements the individual changes.
>
> Patch1 deals with load propagation. According to Peter and Prateek's
> discussion, previously, load propagation for throttled cfs_rq happened
> on unthrottle time but now with per-task throttle, it's no longer the
> case so load propagation should happen immediately or we could lose this
> propagated part.
>
> Patch2 made update_cfs_group() to continue function for cfs_rqs in
> throttled hierarchy so that cfs_rq's entity can get an up2date weight. I
> think this is mostly useful when a cfs_rq in throttled hierarchy still
> has tasks running and on tick/enqueue/dequeue, update_cfs_group() can
> update this cfs_rq's entity weight.
>
> Patch3 removed special treatment of tasks in throttled hierarchy,
> including: dequeue_entities(), check_preempt_wakeup_fair() and
> yield_task_to_fair().
>
> Patch4 inhibited load balancing to a throttled cfs_rq to make hackbench
> happy.
>
> I think patch1 is needed for correctness, patch2-4 is open for
> discussion as there are pros/cons doing things either way. Comments are
> welcome, thanks.
>
> BTW, I also noticed there is the task_is_throttled sched class callback
> and in fair, it is task_is_throttled_fair(). IIUC, it is used by core
> scheduling to find a matching cookie task to run on the sibling SMT CPU.
> For this reason, it doesn't seem very useful if we find it a task that
> is to be throttled so I kept the current implementation; but I guess
> this is also two folded if that to be throttled task is holding some
> kernel resources. Anyway, I didn't write a patch to change it in this
> series, but feel free to let me know if it should be changed.
>
> Aaron Lu (4):
>   sched/fair: Propagate load for throttled cfs_rq
>   sched/fair: update_cfs_group() for throttled cfs_rqs
>   sched/fair: Do not special case tasks in throttled hierarchy
>   sched/fair: Do not balance task to a throttled cfs_rq
>
>  kernel/sched/fair.c | 50 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
>
>
> base-commit: 5b726e9bf9544a349090879a513a5e00da486c14

Yeah, these all make sense to me (with v2 for patch 4).

Reviewed-by: Ben Segall <bsegall@google.com>