Re: [RFC PATCH 1/2] psi: introduce memory.pressure.stat

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon,  1 Aug 2022 00:42:04 +0000
cgel.zte@gmail.com wrote:

> From: cgel <cgel@zte.com.cn>
> 
Trivial things noticed in passing (not a full review!)

Please spell check patch descriptions.

> For now psi memory pressure account for all the mem stall in the
> system, And didnot provide a detailed information why the stall
did not
> happens. This patch introduce a cgroupu knob memory.pressure.stat,
> it tells the detailed stall information of all memory events and it
> format and the corresponding proc interface.
> 
> for the cgroup, add memory.pressure.stat and it shows:
> kswapd: avg10=0.00 avg60=0.00 avg300=0.00 total=0
> direct reclaim: avg10=0.00 avg60=0.00 avg300=0.12 total=42356
> kcompacted: avg10=0.00 avg60=0.00 avg300=0.00 total=0
> direct compact: avg10=0.00 avg60=0.00 avg300=0.00 total=0
> cgroup reclaim: avg10=0.00 avg60=0.00 avg300=0.00 total=0
> workingset thrashing:   avg10=0.00 avg60=0.00 avg300=0.00 total=0
> 
> for the system wide, a proc file introduced as pressure/memory_stat
> and the format is the same as the cgroup interface.
> 
> With this detaled information, for example, if the system is stalled

detailed

> because of kcompacted, compaction_proactiveness can be promoted so
> pro-compaction can be involved earlier.
> 
> Signed-off-by: cgel <cgel@zte.com.cn>
> ---
>  include/linux/psi.h       |   7 +--
>  include/linux/psi_types.h |  34 +++++++++++++
>  kernel/cgroup/cgroup.c    |  11 ++++
>  kernel/sched/psi.c        | 126 +++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 168 insertions(+), 10 deletions(-)
> 
...

> diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
> index 07aaf9b..194ea78 100644
> --- a/include/linux/psi_types.h
> +++ b/include/linux/psi_types.h
> @@ -9,6 +9,8 @@
>  
>  #ifdef CONFIG_PSI
>  
> +#define PSI_MASK(x)	((1UL << (x))-1)
> +
>  /* Tracked task states */
>  enum psi_task_count {
>  	NR_IOWAIT,
> @@ -22,6 +24,10 @@ enum psi_task_count {
>  #define TSK_MEMSTALL	(1 << NR_MEMSTALL)
>  #define TSK_RUNNING	(1 << NR_RUNNING)
>  
> +#define TSK_COUNT_MASK	PSI_MASK(NR_PSI_TASK_COUNTS)
> +#define TSK_COUNT_SHIFT	8
> +
> +
One blank line is probably enough.

>  /* Resources that workloads could be stalled on */
>  enum psi_res {
>  	PSI_IO,
> @@ -53,6 +59,27 @@ enum psi_aggregators {
>  	NR_PSI_AGGREGATORS,
>  };


> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 806fc9d..b50ab92 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -3613,6 +3613,13 @@ static int cgroup_memory_pressure_show(struct seq_file *seq, void *v)
>  
>  	return psi_show(seq, psi, PSI_MEM);
>  }

Almost certainly what blank lines around this.  I'm not sure what reasoning behind
not having it for the other functions in this block is though...

> +static int cgroup_memory_pressure_stat_show(struct seq_file *seq, void *v)
> +{
> +	struct cgroup *cgroup = seq_css(seq)->cgroup;
> +	struct psi_group *psi = cgroup->id == 1 ? &psi_system : &cgroup->psi;
> +
> +	return psi_mem_pressure_stat_show(seq, psi);
> +}
>  static int cgroup_cpu_pressure_show(struct seq_file *seq, void *v)
>  {
>  	struct cgroup *cgroup = seq_css(seq)->cgroup;
> @@ -4930,6 +4937,10 @@ static struct cftype cgroup_base_files[] = {
>  		.poll = cgroup_pressure_poll,
>  		.release = cgroup_pressure_release,
>  	},
> +	{
> +		.name = "memory.pressure.stat",
> +		.seq_show = cgroup_memory_pressure_stat_show,
> +	},
>  	{
>  		.name = "cpu.pressure",
>  		.seq_show = cgroup_cpu_pressure_show,
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 9154e74..072d535 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -279,6 +279,35 @@ static void get_recent_times(struct psi_group *group, int cpu,
..

>  	/*
> @@ -714,7 +770,7 @@ static u32 psi_group_change(struct psi_group *group, int cpu,
>  			state_mask |= (1 << s);
>  	}
>  	groupc->state_mask = state_mask;
> -

Probably better to leave the white space unless there is a strong reason to drop
the blank line.

> +	groupc->state_memstall = state_memstall;
>  	write_seqcount_end(&groupc->seq);
>  
>  	return state_mask;
..

>  static int psi_io_show(struct seq_file *m, void *v)
>  {
>  	return psi_show(m, &psi_system, PSI_IO);
> @@ -998,7 +1101,10 @@ static int psi_memory_open(struct inode *inode, struct file *file)
>  {
>  	return single_open(file, psi_memory_show, NULL);
>  }
> -

Fix this.  It's noise in this patch and the white space is probably desirable anyway.

> +static int psi_memory_stat_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, psi_mem_pressure_stat_show, NULL);
> +}
>  static int psi_cpu_open(struct inode *inode, struct file *file)
>  {
>  	return single_open(file, psi_cpu_show, NULL);
> @@ -1271,7 +1377,12 @@ static const struct file_operations psi_memory_fops = {
>  	.poll           = psi_fop_poll,
>  	.release        = psi_fop_release,
>  };
> -

And this

> +static const struct file_operations psi_memory_stat_fops = {
> +	.open           = psi_memory_stat_open,
> +	.read           = seq_read,
> +	.llseek         = seq_lseek,
> +	.release        = psi_fop_release,
> +};
>  static const struct file_operations psi_cpu_fops = {
>  	.open           = psi_cpu_open,
>  	.read           = seq_read,
> @@ -1286,6 +1397,7 @@ static int __init psi_proc_init(void)
>  	proc_mkdir("pressure", NULL);
>  	proc_create("pressure/io", 0, NULL, &psi_io_fops);
>  	proc_create("pressure/memory", 0, NULL, &psi_memory_fops);
> +	proc_create("pressure/memory_stat", 0, NULL, &psi_memory_stat_fops);
>  	proc_create("pressure/cpu", 0, NULL, &psi_cpu_fops);
>  	return 0;
>  }