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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEivzxdMG-Hfq9muv7LrCJmp-qD-kqt06CqyMZ0ENFRkKmu5XQ@mail.gmail.com>
Date: Fri, 8 Nov 2024 14:17:48 +0100
From: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: mingo@...hat.com, James Morse <james.morse@....com>, 
	Jonathan Cameron <Jonathan.Cameron@...wei.com>, Gavin Shan <gshan@...hat.com>, 
	"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>, Thomas Gleixner <tglx@...utronix.de>, 
	Simon Deziel <simon.deziel@...onical.com>, Juri Lelli <juri.lelli@...hat.com>, 
	Vincent Guittot <vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>, 
	Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/cpuacct: show only present CPUs to userspace

On Mon, Oct 28, 2024 at 2:23 PM Peter Zijlstra <peterz@...radead.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@....com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > Cc: Gavin Shan <gshan@...hat.com>
> > Cc: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Reported-by: Simon Deziel <simon.deziel@...onical.com>
> > Closes: https://github.com/canonical/lxd/issues/13324
> > Co-developed-by: Simon Deziel <simon.deziel@...onical.com>
> > Signed-off-by: Simon Deziel <simon.deziel@...onical.com>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@...onical.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