[Devel] [PATCH vz10 14/24] sched/ve: read task ve locklessly in update_sched_lat() under rq->lock

[Pavel Tikhomirov]

On 7/6/26 12:59, Konstantin Khorenko wrote:
> update_sched_lat() runs from the schedstats hot path
> (__update_stats_wait_end(), reached via set_next_entity() ->
> pick_next_task_fair() -> __schedule()) with rq->lock held - a raw
> spinlock. It used get_task_ve(), which takes task_lock() (a non-raw
> spinlock) plus a ve reference. Acquiring a non-raw spinlock under a raw
> spinlock is an invalid lock-nesting context; on a PROVE_RAW_LOCK_NESTING
> kernel lockdep reports it under load:
> 
>   BUG: Invalid wait context
>   stress-ng-cache/... is trying to lock: &p->alloc_lock ... {3:3}
>   1 lock held: &rq->__lock ... {2:2}
>     __update_stats_wait_end <- set_next_entity <- pick_next_task_fair
>       <- __pick_next_task <- __schedule
> 
> update_sched_lat() only needs the task's ve to bump a per-CPU latency
> counter, and the task is on this rq so its ve cannot be freed under us.
> Read it locklessly with task_ve() instead of taking task_lock() and a ve
> reference via get_task_ve(); this also drops the refcount churn from the
> scheduler hot path.
> 
> Fixes: 68bbb887a36b ("ve: Switch from ->task_ve to *task_ve() helpers")
> Feature: ve: ve generic structures
> https://virtuozzo.atlassian.net/browse/VSTOR-137234
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  kernel/sched/stats.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/stats.c b/kernel/sched/stats.c
> index 08ed7c8a9c4a..27feb9c8b880 100644
> --- a/kernel/sched/stats.c
> +++ b/kernel/sched/stats.c
> @@ -23,7 +23,16 @@ void __update_stats_wait_start(struct rq *rq, struct task_struct *p,
>  static inline void update_sched_lat(struct task_struct *t, u64 delta, u64 now)
>  {
>  #ifdef CONFIG_VE
> -	struct ve_struct *ve __free(put_ve) = get_task_ve(t);
> +	/*
> +	 * Called from the schedstats hot path (__update_stats_wait_end())
> +	 * with rq->lock held, i.e. under a raw spinlock. get_task_ve() takes
> +	 * task_lock() (a non-raw spinlock), which lockdep flags as an invalid
> +	 * wait context (BUG: Invalid wait context) under PROVE_RAW_LOCK_NESTING.
> +	 * We only need @t's ve to bump a per-CPU latency counter, and @t is
> +	 * on this rq (its ve cannot go away under us), so read it locklessly
> +	 * with task_ve() instead of taking task_lock() and a ve reference.
> +	 */

This comment violates https://docs.kernel.org/process/coding-style.html#commenting
We should not explain the history e.g. "It failed before with BUG:..." We should
only say what is important now.

E.g. something like:

Get task's ve lockles without reference. We can do it because we are in
scheduler and it's guaranteed that the task is not running at the moment and
ve is a stable pointer. We need to do it because we are in the raw spinlock
context and can't take non-raw tasklist lock here.

> +	struct ve_struct *ve = task_ve(t);
>  
>  	KSTAT_LAT_PCPU_ADD(&kstat_glob.sched_lat, delta);
>  	KSTAT_LAT_PCPU_ADD(&ve->sched_lat_ve, delta);

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.