Re: [Devel] [PATCH hci-8.1 3/4] sched: account sleep and block time of a task group entity

[Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>]

On 10/14/26 1:28 PM, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] sched: group iowait/idle classification uses non-hierarchical cfs_rq->nr_iowait
> --
>
> commit b026bcc4d4882c5ba1716b8736a28e5f90a38798
> Author: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
>
> sched: account sleep and block time of a task group entity
>
> This commit makes a group sched_entity start a sleep or block period when
> it is dequeued with DEQUEUE_SLEEP. It chooses block if the group's cfs_rq
> has iowait sleepers, and sleep otherwise. It also starts the period from
> tg_unthrottle_up() when an unthrottled group has nothing to run.
>
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-147823
>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
> [ ... ]
>
>> +/* The group goes idle, blocked if it has iowait sleepers */
>> +static inline void
>> +update_stats_group_sleep_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
>> +{
>> +	struct sched_statistics *stats;
>> +
>> +	stats = __schedstats_from_se(se);
>> +	if (group_cfs_rq(se)->nr_iowait)
>> +		__schedstat_set(stats->block_start, rq_clock(rq_of(cfs_rq)));
>> +	else
>> +		__schedstat_set(stats->sleep_start, rq_clock(rq_of(cfs_rq)));
>> +}
> [Severity: Medium]
> Is the classification here hierarchical? group_cfs_rq(se)->nr_iowait is
> the counter of that group's own cfs_rq only.
>
> dequeue_entity() increments nr_iowait only on the task's immediate cfs_rq:
>
>      if ((flags & DEQUEUE_SLEEP) && entity_is_task(se)) {
>          struct task_struct *tsk = task_of(se);
>
>          if (tsk->in_iowait)
>              cfs_rq->nr_iowait++;
>
> Consider a task with in_iowait set that sleeps in a sub-cgroup S of a
> container CT:
>
>      dequeue_entity(task)
>          S->cfs_rq->nr_iowait++
>      dequeue_entity(S group entity, DEQUEUE_SLEEP)
>          update_stats_group_sleep_start()
>              S->nr_iowait != 0, so block_start is set
>      dequeue_entity(CT group entity, DEQUEUE_SLEEP)
>          update_stats_group_sleep_start()
>              CT->nr_iowait == 0, so sleep_start is set
>
> cpu_cgroup_update_stat() then reads iowait_sum and sum_sleep_runtime from
> the CT group entity, so the time appears as idle and not as iowait.
>
> The follow-up patch "sched: switch a sleeping task group entity between
> idle and iowait" (5de73903cb61) adds nr_iowait_update_group(), called from
> nr_iowait_inc_fair() and nr_iowait_dec_fair(). It re-evaluates only
> p->se.parent, the immediate parent group entity, and the increments and
> decrements still stay on the task's direct cfs_rq.
>
> So even with the whole series applied, it looks like the CT entity stays
> misclassified when the iowait sleepers are in nested cgroups, and the
> stated goal is met only for a flat hierarchy. The same non-hierarchical
> counter is read per task_group in cpuacct.c (tg_nr_iowait).
>
> Should the iowait state be propagated to, or evaluated across, all
> ancestor group entities?
Yes, it is not hierarchical and I have already written about it in the 
cover letter. cfs_rq:nr_iowait counts only the tasks sleeping on that 
cfs_rq, so with nested cpu cgroups inside the CT the CT group entity is 
accounted as idle instead of iowait.

Its the same with vz7. This series restores that behavior, it doesn't 
change the previous model. If we want to make it hierarchical I would 
rather do it in a separate ticket if there is a real need for it.

-- 
Best regards, Vasileios Almpanis
Software Developer, Virtuozzo.


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel