Re: [PATCH v2 1/4] psi: add psi_group_flush_stats() function

[Lorenzo Stoakes <ljs@kernel.org>]

On Fri, May 08, 2026 at 11:00:52PM +0800, Vernon Yang wrote:
> From: Vernon Yang <yanglincheng@kylinos.cn>
>
> Add psi_group_flush_stats() function to prepare for the subsequent
> mthp_ext ebpf program.

This isn't a great commit message, you're just saying you're adding a function
then what you plan to use it for, not anything about the why of adding it.

>
> no function changes.
>
> Signed-off-by: Vernon Yang <yanglincheng@kylinos.cn>
> ---
>  include/linux/psi.h |  1 +
>  kernel/sched/psi.c  | 34 ++++++++++++++++++++++++++--------
>  2 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/psi.h b/include/linux/psi.h
> index e0745873e3f2..7b4fd8190810 100644
> --- a/include/linux/psi.h
> +++ b/include/linux/psi.h
> @@ -22,6 +22,7 @@ void psi_init(void);
>  void psi_memstall_enter(unsigned long *flags);
>  void psi_memstall_leave(unsigned long *flags);
>
> +void psi_group_flush_stats(struct psi_group *group);

Feels a bit iffy, exporting an internal management function?

>  int psi_show(struct seq_file *s, struct psi_group *group, enum psi_res res);
>  struct psi_trigger *psi_trigger_create(struct psi_group *group, char *buf,
>  				       enum psi_res res, struct file *file,
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index d9c9d9480a45..76ffad90b0b5 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -1242,11 +1242,35 @@ void psi_cgroup_restart(struct psi_group *group)
>  }
>  #endif /* CONFIG_CGROUPS */
>
> +/*
> + * __psi_group_flush_stats - flush the total stall time of a psi group
> + * @group: psi group to flush
> + */
> +static void __psi_group_flush_stats(struct psi_group *group)
> +{
> +	u64 now;
> +
> +	/* Update averages before reporting them */
> +	mutex_lock(&group->avgs_lock);
> +	now = sched_clock();
> +	collect_percpu_times(group, PSI_AVGS, NULL);
> +	if (now >= group->avg_next_update)
> +		group->avg_next_update = update_averages(group, now);
> +	mutex_unlock(&group->avgs_lock);

If we do need to factor this out, maybe worth making the mutex lock/unlock a
guard(mutex)(&group->avgs_lock) instead?

> +}
> +
> +void psi_group_flush_stats(struct psi_group *group)
> +{
> +	if (static_branch_likely(&psi_disabled))
> +		return;

Is it actually likely if you're calling this function?

And the caller doesn't care even if PSI is disabled?

> +
> +	__psi_group_flush_stats(group);
> +}
> +
>  int psi_show(struct seq_file *m, struct psi_group *group, enum psi_res res)
>  {
>  	bool only_full = false;
>  	int full;
> -	u64 now;
>
>  	if (static_branch_likely(&psi_disabled))
>  		return -EOPNOTSUPP;
> @@ -1256,13 +1280,7 @@ int psi_show(struct seq_file *m, struct psi_group *group, enum psi_res res)
>  		return -EOPNOTSUPP;
>  #endif
>
> -	/* Update averages before reporting them */
> -	mutex_lock(&group->avgs_lock);
> -	now = sched_clock();
> -	collect_percpu_times(group, PSI_AVGS, NULL);
> -	if (now >= group->avg_next_update)
> -		group->avg_next_update = update_averages(group, now);
> -	mutex_unlock(&group->avgs_lock);
> +	__psi_group_flush_stats(group);
>
>  #ifdef CONFIG_IRQ_TIME_ACCOUNTING
>  	only_full = res == PSI_IRQ;
> --
> 2.53.0
>