[PATCH] sched/cpuacct: show only present CPUs to userspace

[PATCH] sched/cpuacct: show only present CPUs to userspace

[Alexander Mikhalitsyn]

After commit b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES")
changed which CPUs are shown in /sys/devices/system/cpu/ (only "present" ones)
it also makes sense to change cpuacct cgroupv1 code not to report CPUs
which are not present in the system as it confuses userspace.
Let's make it consistent.

A configuration when #(present CPUs) < #(possible CPUs) is easy to get with:
qemu-system-x86_64
	-smp 3,maxcpus=12 \
	...

Cc: James Morse <james.morse@arm.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Gavin Shan <gshan@redhat.com>
Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Reported-by: Simon Deziel <simon.deziel@canonical.com>
Closes: https://github.com/canonical/lxd/issues/13324
Co-developed-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 kernel/sched/cpuacct.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
index 0de9dda09949..0f07fbfdb20e 100644
--- a/kernel/sched/cpuacct.c
+++ b/kernel/sched/cpuacct.c
@@ -213,7 +213,7 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
 	u64 percpu;
 	int i;
 
-	for_each_possible_cpu(i) {
+	for_each_present_cpu(i) {
 		percpu = cpuacct_cpuusage_read(ca, i, index);
 		seq_printf(m, "%llu ", (unsigned long long) percpu);
 	}
@@ -247,7 +247,7 @@ static int cpuacct_all_seq_show(struct seq_file *m, void *V)
 		seq_printf(m, " %s", cpuacct_stat_desc[index]);
 	seq_puts(m, "\n");
 
-	for_each_possible_cpu(cpu) {
+	for_each_present_cpu(cpu) {
 		seq_printf(m, "%d", cpu);
 		for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
 			seq_printf(m, " %llu",
-- 
2.34.1

Re: [PATCH] sched/cpuacct: show only present CPUs to userspace

[Aleksandr Mikhalitsyn]

Gentle ping.

On Thu, Oct 17, 2024 at 12:22 PM Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> After commit b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES")
> changed which CPUs are shown in /sys/devices/system/cpu/ (only "present" ones)
> it also makes sense to change cpuacct cgroupv1 code not to report CPUs
> which are not present in the system as it confuses userspace.
> Let's make it consistent.
>
> A configuration when #(present CPUs) < #(possible CPUs) is easy to get with:
> qemu-system-x86_64
>         -smp 3,maxcpus=12 \
>         ...
>
> Cc: James Morse <james.morse@arm.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Gavin Shan <gshan@redhat.com>
> Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Reported-by: Simon Deziel <simon.deziel@canonical.com>
> Closes: https://github.com/canonical/lxd/issues/13324
> Co-developed-by: Simon Deziel <simon.deziel@canonical.com>
> Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>  kernel/sched/cpuacct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> index 0de9dda09949..0f07fbfdb20e 100644
> --- a/kernel/sched/cpuacct.c
> +++ b/kernel/sched/cpuacct.c
> @@ -213,7 +213,7 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
>         u64 percpu;
>         int i;
>
> -       for_each_possible_cpu(i) {
> +       for_each_present_cpu(i) {
>                 percpu = cpuacct_cpuusage_read(ca, i, index);
>                 seq_printf(m, "%llu ", (unsigned long long) percpu);
>         }
> @@ -247,7 +247,7 @@ static int cpuacct_all_seq_show(struct seq_file *m, void *V)
>                 seq_printf(m, " %s", cpuacct_stat_desc[index]);
>         seq_puts(m, "\n");
>
> -       for_each_possible_cpu(cpu) {
> +       for_each_present_cpu(cpu) {
>                 seq_printf(m, "%d", cpu);
>                 for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
>                         seq_printf(m, " %llu",
> --
> 2.34.1
>

Re: [PATCH] sched/cpuacct: show only present CPUs to userspace

[Jonathan Cameron]

On Fri, 25 Oct 2024 17:35:56 +0200
Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote:

> Gentle ping.
> 
> On Thu, Oct 17, 2024 at 12:22 PM Alexander Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
> >
> > After commit b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES")
> > changed which CPUs are shown in /sys/devices/system/cpu/ (only "present" ones)
> > it also makes sense to change cpuacct cgroupv1 code not to report CPUs
> > which are not present in the system as it confuses userspace.
> > Let's make it consistent.
> >
> > A configuration when #(present CPUs) < #(possible CPUs) is easy to get with:
> > qemu-system-x86_64
> >         -smp 3,maxcpus=12 \
> >         ...
> >

On a general basis, we definitely want these to line up, but I'm not familiar
enough with this code to give more specific review.

Other than that, I'm curious as to what userspace is tripping over this?

Jonathan
 
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Gavin Shan <gshan@redhat.com>
> > Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Reported-by: Simon Deziel <simon.deziel@canonical.com>
> > Closes: https://github.com/canonical/lxd/issues/13324
> > Co-developed-by: Simon Deziel <simon.deziel@canonical.com>
> > Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> >  kernel/sched/cpuacct.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> > index 0de9dda09949..0f07fbfdb20e 100644
> > --- a/kernel/sched/cpuacct.c
> > +++ b/kernel/sched/cpuacct.c
> > @@ -213,7 +213,7 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
> >         u64 percpu;
> >         int i;
> >
> > -       for_each_possible_cpu(i) {
> > +       for_each_present_cpu(i) {
> >                 percpu = cpuacct_cpuusage_read(ca, i, index);
> >                 seq_printf(m, "%llu ", (unsigned long long) percpu);
> >         }
> > @@ -247,7 +247,7 @@ static int cpuacct_all_seq_show(struct seq_file *m, void *V)
> >                 seq_printf(m, " %s", cpuacct_stat_desc[index]);
> >         seq_puts(m, "\n");
> >
> > -       for_each_possible_cpu(cpu) {
> > +       for_each_present_cpu(cpu) {
> >                 seq_printf(m, "%d", cpu);
> >                 for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
> >                         seq_printf(m, " %llu",
> > --
> > 2.34.1
> >  

Re: [PATCH] sched/cpuacct: show only present CPUs to userspace

[Aleksandr Mikhalitsyn]

On Mon, Oct 28, 2024 at 12:07 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 25 Oct 2024 17:35:56 +0200
> Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote:
>
> > Gentle ping.
> >
> > On Thu, Oct 17, 2024 at 12:22 PM Alexander Mikhalitsyn
> > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > >
> > > After commit b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES")
> > > changed which CPUs are shown in /sys/devices/system/cpu/ (only "present" ones)
> > > it also makes sense to change cpuacct cgroupv1 code not to report CPUs
> > > which are not present in the system as it confuses userspace.
> > > Let's make it consistent.
> > >
> > > A configuration when #(present CPUs) < #(possible CPUs) is easy to get with:
> > > qemu-system-x86_64
> > >         -smp 3,maxcpus=12 \
> > >         ...
> > >
>

Dear Jonathan,

> On a general basis, we definitely want these to line up, but I'm not familiar
> enough with this code to give more specific review.
>
> Other than that, I'm curious as to what userspace is tripping over this?

Originally it was reported me with regard to LXD
https://github.com/canonical/lxd/issues/13324

But I can imagine that any userspace can be confused there as what you
see in /proc/cpuinfo (and even /sys/devices/system/cpu/)
is not aligned with that you see in cpuacct.usage_all file). That's
extremely unexpected to see more CPUs in a cgroup-related file
than in /sys/devices/system/cpu/, IMHO.

Kind regards,
Alex

>
> Jonathan
>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Gavin Shan <gshan@redhat.com>
> > > Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Reported-by: Simon Deziel <simon.deziel@canonical.com>
> > > Closes: https://github.com/canonical/lxd/issues/13324
> > > Co-developed-by: Simon Deziel <simon.deziel@canonical.com>
> > > Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > ---
> > >  kernel/sched/cpuacct.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> > > index 0de9dda09949..0f07fbfdb20e 100644
> > > --- a/kernel/sched/cpuacct.c
> > > +++ b/kernel/sched/cpuacct.c
> > > @@ -213,7 +213,7 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
> > >         u64 percpu;
> > >         int i;
> > >
> > > -       for_each_possible_cpu(i) {
> > > +       for_each_present_cpu(i) {
> > >                 percpu = cpuacct_cpuusage_read(ca, i, index);
> > >                 seq_printf(m, "%llu ", (unsigned long long) percpu);
> > >         }
> > > @@ -247,7 +247,7 @@ static int cpuacct_all_seq_show(struct seq_file *m, void *V)
> > >                 seq_printf(m, " %s", cpuacct_stat_desc[index]);
> > >         seq_puts(m, "\n");
> > >
> > > -       for_each_possible_cpu(cpu) {
> > > +       for_each_present_cpu(cpu) {
> > >                 seq_printf(m, "%d", cpu);
> > >                 for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
> > >                         seq_printf(m, " %llu",
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH] sched/cpuacct: show only present CPUs to userspace

[Peter Zijlstra]

On Thu, Oct 17, 2024 at 12:21:38PM +0200, Alexander Mikhalitsyn wrote:
> After commit b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES")
> changed which CPUs are shown in /sys/devices/system/cpu/ (only "present" ones)
> it also makes sense to change cpuacct cgroupv1 code not to report CPUs
> which are not present in the system as it confuses userspace.
> Let's make it consistent.
> 
> A configuration when #(present CPUs) < #(possible CPUs) is easy to get with:
> qemu-system-x86_64
> 	-smp 3,maxcpus=12 \
> 	...
> 
> Cc: James Morse <james.morse@arm.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Gavin Shan <gshan@redhat.com>
> Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Reported-by: Simon Deziel <simon.deziel@canonical.com>
> Closes: https://github.com/canonical/lxd/issues/13324
> Co-developed-by: Simon Deziel <simon.deziel@canonical.com>
> Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>  kernel/sched/cpuacct.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> index 0de9dda09949..0f07fbfdb20e 100644
> --- a/kernel/sched/cpuacct.c
> +++ b/kernel/sched/cpuacct.c
> @@ -213,7 +213,7 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
>  	u64 percpu;
>  	int i;
>  
> -	for_each_possible_cpu(i) {
> +	for_each_present_cpu(i) {
>  		percpu = cpuacct_cpuusage_read(ca, i, index);
>  		seq_printf(m, "%llu ", (unsigned long long) percpu);
>  	}
> @@ -247,7 +247,7 @@ static int cpuacct_all_seq_show(struct seq_file *m, void *V)
>  		seq_printf(m, " %s", cpuacct_stat_desc[index]);
>  	seq_puts(m, "\n");
>  
> -	for_each_possible_cpu(cpu) {
> +	for_each_present_cpu(cpu) {
>  		seq_printf(m, "%d", cpu);
>  		for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
>  			seq_printf(m, " %llu",


Doesn't this create problems for machines that support actual physical
hotplug?

Then all of a sudden, when a CPU with non-zero stats goes from present
to !present, the stats also go away, and any userspace looking at the
sum of stats over CPUs sees an unexplained dis-continuity.

Re: [PATCH] sched/cpuacct: show only present CPUs to userspace

[Aleksandr Mikhalitsyn]

On Mon, Oct 28, 2024 at 2:23 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Oct 17, 2024 at 12:21:38PM +0200, Alexander Mikhalitsyn wrote:
> > After commit b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES")
> > changed which CPUs are shown in /sys/devices/system/cpu/ (only "present" ones)
> > it also makes sense to change cpuacct cgroupv1 code not to report CPUs
> > which are not present in the system as it confuses userspace.
> > Let's make it consistent.
> >
> > A configuration when #(present CPUs) < #(possible CPUs) is easy to get with:
> > qemu-system-x86_64
> >       -smp 3,maxcpus=12 \
> >       ...
> >
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Gavin Shan <gshan@redhat.com>
> > Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Reported-by: Simon Deziel <simon.deziel@canonical.com>
> > Closes: https://github.com/canonical/lxd/issues/13324
> > Co-developed-by: Simon Deziel <simon.deziel@canonical.com>
> > Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> >  kernel/sched/cpuacct.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/cpuacct.c b/kernel/sched/cpuacct.c
> > index 0de9dda09949..0f07fbfdb20e 100644
> > --- a/kernel/sched/cpuacct.c
> > +++ b/kernel/sched/cpuacct.c
> > @@ -213,7 +213,7 @@ static int __cpuacct_percpu_seq_show(struct seq_file *m,
> >       u64 percpu;
> >       int i;
> >
> > -     for_each_possible_cpu(i) {
> > +     for_each_present_cpu(i) {
> >               percpu = cpuacct_cpuusage_read(ca, i, index);
> >               seq_printf(m, "%llu ", (unsigned long long) percpu);
> >       }
> > @@ -247,7 +247,7 @@ static int cpuacct_all_seq_show(struct seq_file *m, void *V)
> >               seq_printf(m, " %s", cpuacct_stat_desc[index]);
> >       seq_puts(m, "\n");
> >
> > -     for_each_possible_cpu(cpu) {
> > +     for_each_present_cpu(cpu) {
> >               seq_printf(m, "%d", cpu);
> >               for (index = 0; index < CPUACCT_STAT_NSTATS; index++)
> >                       seq_printf(m, " %llu",
>

Dear Peter,

>
> Doesn't this create problems for machines that support actual physical
> hotplug?
>
> Then all of a sudden, when a CPU with non-zero stats goes from present
> to !present, the stats also go away, and any userspace looking at the
> sum of stats over CPUs sees an unexplained dis-continuity.

Yep, that's what I thought about too. At the same time, if userspace
sum of stat over CPUs,
we already have /sys/fs/cgroup/cpuacct/cpuacct.usage (which uses
for_each_possible_cpu() and I did not
change that on purpose in this patch).

I agree that it's a bit tricky and not obvious which is correct. But
you might agree that it's a bit weird that
cpuacct.usage_all file shows more CPUs than you can see in sysfs
/sys/devices/system/cpu/.

I just wanted to point that problem out so we can discuss and decide.
But of course we should follow a Do-No-Harm principle here.

Kind regards,
Alex