[PATCH 0/3] sched/psi: psi bug fixes and cleanups

[PATCH 0/3] sched/psi: psi bug fixes and cleanups

[Hao Jia]

These three patches are about PSI.
patch 1: Fixed PSI statistics error caused by unzeroed memory
in struct psi_group.
patch 2 and patch 3 are to clean up some unused functions
and parameters.

Hao Jia (3):
  sched/psi: Zero the memory of struct psi_group
  sched/psi: Remove unused parameter nbytes of psi_trigger_create()
  sched/psi: Remove redundant cgroup_psi() when !CONFIG_CGROUPS

 include/linux/cgroup.h |  5 -----
 include/linux/psi.h    |  2 +-
 kernel/cgroup/cgroup.c |  2 +-
 kernel/sched/psi.c     | 10 +++-------
 4 files changed, 5 insertions(+), 14 deletions(-)

-- 
2.32.0

[PATCH 1/3] sched/psi: Zero the memory of struct psi_group

[Hao Jia]

After commit 5f69a6577bc3 ("psi: dont alloc memory for psi by default"),
the memory used by struct psi_group is no longer allocated and zeroed
in cgroup_create().

Since the memory of struct psi_group is not zeroed, the data in this
memory is random, which will lead to inaccurate psi statistics when
creating a new cgroup.

So we use kzlloc() to allocate and zero the struct psi_group and
remove the redundant zeroing in group_init().

Steps to reproduce:
1. Use cgroup v2 and enable CONFIG_PSI
2. Create a new cgroup, and query psi statistics
mkdir /sys/fs/cgroup/test
cat /sys/fs/cgroup/test/cpu.pressure
some avg10=0.00 avg60=0.00 avg300=47927752200.00 total=12884901
full avg10=561815124.00 avg60=125835394188.00 avg300=1077090462000.00 total=10273561772

cat /sys/fs/cgroup/test/io.pressure
some avg10=1040093132823.95 avg60=1203770351379.21 avg300=3862252669559.46 total=4294967296
full avg10=921884564601.39 avg60=0.00 avg300=1984507298.35 total=442381631

cat /sys/fs/cgroup/test/memory.pressure
some avg10=232476085778.11 avg60=0.00 avg300=0.00 total=0
full avg10=0.00 avg60=0.00 avg300=2585658472280.57 total=12884901

Fixes: commit 5f69a6577bc3 ("psi: dont alloc memory for psi by default")

Signed-off-by: Hao Jia <jiahao.os@bytedance.com>
---
 kernel/sched/psi.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index ec66b40bdd40..5ee615a59fe1 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -190,12 +190,8 @@ static void group_init(struct psi_group *group)
 	/* Init trigger-related members */
 	mutex_init(&group->trigger_lock);
 	INIT_LIST_HEAD(&group->triggers);
-	memset(group->nr_triggers, 0, sizeof(group->nr_triggers));
-	group->poll_states = 0;
 	group->poll_min_period = U32_MAX;
-	memset(group->polling_total, 0, sizeof(group->polling_total));
 	group->polling_next_update = ULLONG_MAX;
-	group->polling_until = 0;
 	init_waitqueue_head(&group->poll_wait);
 	timer_setup(&group->poll_timer, poll_timer_fn, 0);
 	rcu_assign_pointer(group->poll_task, NULL);
@@ -957,7 +953,7 @@ int psi_cgroup_alloc(struct cgroup *cgroup)
 	if (static_branch_likely(&psi_disabled))
 		return 0;
 
-	cgroup->psi = kmalloc(sizeof(struct psi_group), GFP_KERNEL);
+	cgroup->psi = kzalloc(sizeof(struct psi_group), GFP_KERNEL);
 	if (!cgroup->psi)
 		return -ENOMEM;
 
-- 
2.32.0

Re: [PATCH 1/3] sched/psi: Zero the memory of struct psi_group

[Johannes Weiner]

On Sat, Aug 06, 2022 at 08:05:08PM +0800, Hao Jia wrote:
> After commit 5f69a6577bc3 ("psi: dont alloc memory for psi by default"),
> the memory used by struct psi_group is no longer allocated and zeroed
> in cgroup_create().
>
> Since the memory of struct psi_group is not zeroed, the data in this
> memory is random, which will lead to inaccurate psi statistics when
> creating a new cgroup.
> 
> So we use kzlloc() to allocate and zero the struct psi_group and
> remove the redundant zeroing in group_init().
> 
> Steps to reproduce:
> 1. Use cgroup v2 and enable CONFIG_PSI
> 2. Create a new cgroup, and query psi statistics
> mkdir /sys/fs/cgroup/test
> cat /sys/fs/cgroup/test/cpu.pressure
> some avg10=0.00 avg60=0.00 avg300=47927752200.00 total=12884901
> full avg10=561815124.00 avg60=125835394188.00 avg300=1077090462000.00 total=10273561772
> 
> cat /sys/fs/cgroup/test/io.pressure
> some avg10=1040093132823.95 avg60=1203770351379.21 avg300=3862252669559.46 total=4294967296
> full avg10=921884564601.39 avg60=0.00 avg300=1984507298.35 total=442381631
> 
> cat /sys/fs/cgroup/test/memory.pressure
> some avg10=232476085778.11 avg60=0.00 avg300=0.00 total=0
> full avg10=0.00 avg60=0.00 avg300=2585658472280.57 total=12884901
> 
> Fixes: commit 5f69a6577bc3 ("psi: dont alloc memory for psi by default")
> 
> Signed-off-by: Hao Jia <jiahao.os@bytedance.com>

Yikes! Yes, we relied on the embedding cgroup being kzalloc'd, or, in
the case of psi_system, on the psi_group being in NULLed static mem.
The partial zeroing in group_init() obscured that. Thanks for the fix.

Cc: stable@vger.kernel.org # 5.19
Acked-by: Johannes Weiner <hannes@cmpxchg.org>

[PATCH 2/3] sched/psi: Remove unused parameter nbytes of psi_trigger_create()

[Hao Jia]

psi_trigger_create()'s 'nbytes' parameter is not used, so we can remove it.

Signed-off-by: Hao Jia <jiahao.os@bytedance.com>
---
 include/linux/psi.h    | 2 +-
 kernel/cgroup/cgroup.c | 2 +-
 kernel/sched/psi.c     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/psi.h b/include/linux/psi.h
index 89784763d19e..dd74411ac21d 100644
--- a/include/linux/psi.h
+++ b/include/linux/psi.h
@@ -27,7 +27,7 @@ void psi_memstall_leave(unsigned long *flags);
 
 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, size_t nbytes, enum psi_res res);
+			char *buf, enum psi_res res);
 void psi_trigger_destroy(struct psi_trigger *t);
 
 __poll_t psi_trigger_poll(void **trigger_ptr, struct file *file,
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index ffaccd6373f1..df7df5843b4f 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3698,7 +3698,7 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
 	}
 
 	psi = cgroup_ino(cgrp) == 1 ? &psi_system : cgrp->psi;
-	new = psi_trigger_create(psi, buf, nbytes, res);
+	new = psi_trigger_create(psi, buf, res);
 	if (IS_ERR(new)) {
 		cgroup_put(cgrp);
 		return PTR_ERR(new);
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 5ee615a59fe1..ecb4b4ff4ce0 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -1087,7 +1087,7 @@ int psi_show(struct seq_file *m, struct psi_group *group, enum psi_res res)
 }
 
 struct psi_trigger *psi_trigger_create(struct psi_group *group,
-			char *buf, size_t nbytes, enum psi_res res)
+			char *buf, enum psi_res res)
 {
 	struct psi_trigger *t;
 	enum psi_states state;
@@ -1316,7 +1316,7 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
 		return -EBUSY;
 	}
 
-	new = psi_trigger_create(&psi_system, buf, nbytes, res);
+	new = psi_trigger_create(&psi_system, buf, res);
 	if (IS_ERR(new)) {
 		mutex_unlock(&seq->lock);
 		return PTR_ERR(new);
-- 
2.32.0

Re: [PATCH 2/3] sched/psi: Remove unused parameter nbytes of psi_trigger_create()

[Johannes Weiner]

On Sat, Aug 06, 2022 at 08:05:09PM +0800, Hao Jia wrote:
> psi_trigger_create()'s 'nbytes' parameter is not used, so we can remove it.
> 
> Signed-off-by: Hao Jia <jiahao.os@bytedance.com>

Right, it relies on \0-termination which the callers psi_write() and
kernfs_fop_write_iter() guarantee.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

[PATCH 3/3] sched/psi: Remove redundant cgroup_psi() when !CONFIG_CGROUPS

[Hao Jia]

cgroup_psi() is only called under CONFIG_CGROUPS.
We don't need cgroup_psi() when !CONFIG_CGROUPS,
so we can remove it in this case.

Signed-off-by: Hao Jia <jiahao.os@bytedance.com>
---
 include/linux/cgroup.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index ed53bfe7c46c..ac5d0515680e 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -734,11 +734,6 @@ static inline struct cgroup *cgroup_parent(struct cgroup *cgrp)
 	return NULL;
 }
 
-static inline struct psi_group *cgroup_psi(struct cgroup *cgrp)
-{
-	return NULL;
-}
-
 static inline bool cgroup_psi_enabled(void)
 {
 	return false;
-- 
2.32.0

Re: [PATCH 3/3] sched/psi: Remove redundant cgroup_psi() when !CONFIG_CGROUPS

[Johannes Weiner]

On Sat, Aug 06, 2022 at 08:05:10PM +0800, Hao Jia wrote:
> cgroup_psi() is only called under CONFIG_CGROUPS.
> We don't need cgroup_psi() when !CONFIG_CGROUPS,
> so we can remove it in this case.
> 
> Signed-off-by: Hao Jia <jiahao.os@bytedance.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH 0/3] sched/psi: psi bug fixes and cleanups

[Ingo Molnar]

* Hao Jia <jiahao.os@bytedance.com> wrote:

> These three patches are about PSI.
> patch 1: Fixed PSI statistics error caused by unzeroed memory
> in struct psi_group.
> patch 2 and patch 3 are to clean up some unused functions
> and parameters.
> 
> Hao Jia (3):
>   sched/psi: Zero the memory of struct psi_group
>   sched/psi: Remove unused parameter nbytes of psi_trigger_create()
>   sched/psi: Remove redundant cgroup_psi() when !CONFIG_CGROUPS
> 
>  include/linux/cgroup.h |  5 -----
>  include/linux/psi.h    |  2 +-
>  kernel/cgroup/cgroup.c |  2 +-
>  kernel/sched/psi.c     | 10 +++-------
>  4 files changed, 5 insertions(+), 14 deletions(-)

LGTM, and I suspect the scheduler fix wants to go upstream via the tree 
that introduced the bug, the cgroup tree?

For the series:

   Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 0/3] sched/psi: psi bug fixes and cleanups

[Tejun Heo]

(cc'ing Johannes)

On Sat, Aug 06, 2022 at 08:05:07PM +0800, Hao Jia wrote:
> These three patches are about PSI.
> patch 1: Fixed PSI statistics error caused by unzeroed memory
> in struct psi_group.
> patch 2 and patch 3 are to clean up some unused functions
> and parameters.
> 
> Hao Jia (3):
>   sched/psi: Zero the memory of struct psi_group
>   sched/psi: Remove unused parameter nbytes of psi_trigger_create()
>   sched/psi: Remove redundant cgroup_psi() when !CONFIG_CGROUPS
> 
>  include/linux/cgroup.h |  5 -----
>  include/linux/psi.h    |  2 +-
>  kernel/cgroup/cgroup.c |  2 +-
>  kernel/sched/psi.c     | 10 +++-------
>  4 files changed, 5 insertions(+), 14 deletions(-)

Johannes, care to review these patches?

Thanks.

-- 
tejun

Re: [PATCH 0/3] sched/psi: psi bug fixes and cleanups

[Johannes Weiner]

On Tue, Aug 09, 2022 at 07:37:21AM -1000, Tejun Heo wrote:
> (cc'ing Johannes)
> 
> On Sat, Aug 06, 2022 at 08:05:07PM +0800, Hao Jia wrote:
> > These three patches are about PSI.
> > patch 1: Fixed PSI statistics error caused by unzeroed memory
> > in struct psi_group.
> > patch 2 and patch 3 are to clean up some unused functions
> > and parameters.
> > 
> > Hao Jia (3):
> >   sched/psi: Zero the memory of struct psi_group
> >   sched/psi: Remove unused parameter nbytes of psi_trigger_create()
> >   sched/psi: Remove redundant cgroup_psi() when !CONFIG_CGROUPS
> > 
> >  include/linux/cgroup.h |  5 -----
> >  include/linux/psi.h    |  2 +-
> >  kernel/cgroup/cgroup.c |  2 +-
> >  kernel/sched/psi.c     | 10 +++-------
> >  4 files changed, 5 insertions(+), 14 deletions(-)
> 
> Johannes, care to review these patches?

They look good to me.

Thanks!

Re: [PATCH 0/3] sched/psi: psi bug fixes and cleanups

[Tejun Heo]

On Sat, Aug 06, 2022 at 08:05:07PM +0800, Hao Jia wrote:
> These three patches are about PSI.
> patch 1: Fixed PSI statistics error caused by unzeroed memory
> in struct psi_group.
> patch 2 and patch 3 are to clean up some unused functions
> and parameters.

Applied 1-3 to cgroup/for-6.0-fixes.

Thanks.

-- 
tejun