Re: [PATCH] delayacct: Account blkio start on the correct task

[Christian Loehle <christian.loehle@arm.com>]

On 10/14/26 09:17, Zhan Xusheng wrote:
> __block_task(rq, p) tests p->in_iowait and bumps rq->nr_iowait, but the
> third statement in that block takes neither argument:
> 
> 	if (p->in_iowait) {
> 		atomic_inc(&rq->nr_iowait);
> 		delayacct_blkio_start();
> 	}
> 
> delayacct_blkio_start() stamps current->delays->blkio_start.
> 
> That was fine when commit e8901061ca0c ("sched: Split DEQUEUE_SLEEP from
> deactivate_task()") introduced __block_task(), because its only caller
> was block_task() out of __schedule(), where prev == current.  Commit
> 152e11f6df29 ("sched/fair: Implement delayed dequeue") added a second
> call, at the point the delayed entity is finally dequeued:
> 
> 	/*
> 	 * Fix-up what block_task() skipped.
> 	 *
> 	 * Must be last, @p might not be valid after this.
> 	 */
> 	__block_task(rq, p);
> 
> The paths that reach it do not run as @p:
> 
>   - pick_next_entity() -> __dequeue_task(DEQUEUE_DELAYED), inside the
>     __schedule() of whatever task is prev on that CPU
>   - wait_task_inactive(), in the task doing the waiting
>   - switching_from_fair(), via sched_setscheduler() or rt_mutex_setprio(),
>     which may or may not be @p itself
> 
> So the stamp lands on an unrelated task, and @p keeps whatever
> blkio_start it had.  When the I/O completes, delayacct_blkio_end(p)
> computes local_clock() - p->delays->blkio_start and adds that to @p's
> blkio_delay and blkio_count, and may overwrite blkio_delay_max and
> blkio_delay_max_ts.  The error is however long it has been since @p's
> blkio_start was last written, so it is unbounded.  ->delays is
> kmem_cache_zalloc()'d, so if it was never written the delta is the time
> since boot.
> 
> Measured on an 8-vCPU guest, 2.5 GB file on f2fs over virtio, one task
> alternating a 1 MiB O_DIRECT read with two seconds of pure CPU work,
> pinned to CPU 0 against three CPU hogs so that the reader gets
> delay-dequeued.  Reading delayacct_blkio_ticks from /proc/self/stat
> around the second read:
> 
>   DELAY_DEQUEUE    18 runs, 4 wrong: 206, 20316, 20729, 21968 ticks
>                    reported for reads that took 25-50 ms
>   NO_DELAY_DEQUEUE 12 runs, 0 wrong, all 1-3 ticks
> 
> The 206 case is the two seconds of CPU work swept in; the 20000+ cases
> are blkio_start still at zero.  In a separate run under the same
> conditions, a kprobe on __delayacct_blkio_start() caught it being called
> with current == "sh" and current == "kworker/0:2", neither of which
> issues any I/O.
> 
> Consumers are delayacct_blkio_ticks in /proc/pid/stat, taskstats
> (getdelays, iotop, delaytop) and the per-task blkio_delay_max peak.  The
> error also goes the other way: a task woken while still delayed goes
> through ttwu_runnable() and has that sleep dropped entirely.
> 
> Pass the task to delayacct_blkio_start(), the same way commit
> c96f5471ce7d ("delayacct: Account blkio completion on the correct task")
> did for the end side when commit e33a9bba85a8 ("sched/core: move IO
> scheduling accounting from io_schedule_timeout() into scheduler") moved
> that call into ttwu().
> 
> Note this only makes blkio_start agree with the existing deferral: both
> the stamp and rq->nr_iowait are still taken at __block_task() time, so
> the interval a task spends delayed is counted by neither.  Whether that
> interval should count as iowait is a separate question.
> 
> Fixes: 152e11f6df29 ("sched/fair: Implement delayed dequeue")
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>

2 iowait patches in one day? Wow
For the task-pointer fix:
Reviewed-by: Christian Loehle <christian.loehle@arm.com>

BUT FWIW I don't think this is correct in the first place, why would delayacct_blkio
only start after delayed dequeue? This means a task waking up within
delayed dequeue is invisible to the accounting (as you note).
Let me try to move this to the actual block/wakeup...


> ---
>  include/linux/delayacct.h | 10 +++++-----
>  kernel/delayacct.c        |  4 ++--
>  kernel/sched/sched.h      |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
> index ecb06f16d22c5..e595d51699377 100644
> --- a/include/linux/delayacct.h
> +++ b/include/linux/delayacct.h
> @@ -92,7 +92,7 @@ extern void delayacct_init(void);
>  
>  extern void __delayacct_tsk_init(struct task_struct *);
>  extern void __delayacct_tsk_exit(struct task_struct *);
> -extern void __delayacct_blkio_start(void);
> +extern void __delayacct_blkio_start(struct task_struct *p);
>  extern void __delayacct_blkio_end(struct task_struct *);
>  extern int delayacct_add_tsk(struct taskstats *, struct task_struct *);
>  extern __u64 __delayacct_blkio_ticks(struct task_struct *);
> @@ -126,13 +126,13 @@ static inline void delayacct_tsk_free(struct task_struct *tsk)
>  	tsk->delays = NULL;
>  }
>  
> -static inline void delayacct_blkio_start(void)
> +static inline void delayacct_blkio_start(struct task_struct *p)
>  {
>  	if (!static_branch_unlikely(&delayacct_key))
>  		return;
>  
> -	if (current->delays)
> -		__delayacct_blkio_start();
> +	if (p->delays)
> +		__delayacct_blkio_start(p);
>  }
>  
>  static inline void delayacct_blkio_end(struct task_struct *p)
> @@ -257,7 +257,7 @@ static inline void delayacct_tsk_init(struct task_struct *tsk)
>  {}
>  static inline void delayacct_tsk_free(struct task_struct *tsk)
>  {}
> -static inline void delayacct_blkio_start(void)
> +static inline void delayacct_blkio_start(struct task_struct *p)
>  {}
>  static inline void delayacct_blkio_end(struct task_struct *p)
>  {}
> diff --git a/kernel/delayacct.c b/kernel/delayacct.c
> index 479e860aff705..d6158ecbe0be5 100644
> --- a/kernel/delayacct.c
> +++ b/kernel/delayacct.c
> @@ -126,9 +126,9 @@ static void delayacct_end(raw_spinlock_t *lock, u64 *start, u64 *total, u32 *cou
>  	}
>  }
>  
> -void __delayacct_blkio_start(void)
> +void __delayacct_blkio_start(struct task_struct *p)
>  {
> -	current->delays->blkio_start = local_clock();
> +	p->delays->blkio_start = local_clock();
>  }
>  
>  /*
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 7d2ec527b8a25..5bba703160e58 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3106,7 +3106,7 @@ static inline void __block_task(struct rq *rq, struct task_struct *p)
>  
>  	if (p->in_iowait) {
>  		atomic_inc(&rq->nr_iowait);
> -		delayacct_blkio_start();
> +		delayacct_blkio_start(p);
>  	}
>  
>  	ASSERT_EXCLUSIVE_WRITER(p->on_rq);
> 
> base-commit: 4a3b51aab6e25244d97936aa65e6d5425adf98e1