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

[no-reply@sashiko.virtuozzo.com]

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?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261014-ct-iowait-v1-0-9d65fe37cf89@virtuozzo.com?part=3

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