On Mon, 1 Aug 2022 00:42:04 +0000
cgel.zte@xxxxxxxxx wrote:
> From: cgel <cgel@xxxxxxxxxx>
>
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@xxxxxxxxxx>
> ---
> 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;
> }
