| Message ID | 1447870505-19319-1-git-send-email-yang.shi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Wed, Nov 18, 2015 at 10:15:05AM -0800, Yang Shi wrote: > As what Pavel Machek reported [1], some userspace applications depend on > bogomips showed by /proc/cpuinfo. > > Although there is much less legacy impact on aarch64 than arm, but it does > break libvirt. > > Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with > some tweak due to context change. > > [1] https://lkml.org/lkml/2015/1/4/132 I lost this argument last time around, so I won't re-tread that path this time around. I do, however, have some comments on the patch. > > Signed-off-by: Yang Shi <yang.shi@linaro.org> > --- > arch/arm64/kernel/cpuinfo.c | 5 +++++ > arch/arm64/kernel/smp.c | 7 ++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > index 706679d..8d4ba77 100644 > --- a/arch/arm64/kernel/cpuinfo.c > +++ b/arch/arm64/kernel/cpuinfo.c > @@ -30,6 +30,7 @@ > #include <linux/seq_file.h> > #include <linux/sched.h> > #include <linux/smp.h> > +#include <linux/delay.h> > > /* > * In case the boot CPU is hotpluggable, we record its initial state and > @@ -112,6 +113,10 @@ static int c_show(struct seq_file *m, void *v) > */ > seq_printf(m, "processor\t: %d\n", i); > > + seq_printf(m, "BogoMIPS\t: %lu.%02lu\n\n", This double newline makes /proc/cpuinfo looks really odd. Can we just have one, please? > + loops_per_jiffy / (500000UL/HZ), > + loops_per_jiffy / (5000UL/HZ) % 100); > + > /* > * Dump out the common processor features in a single line. > * Userspace should read the hwcaps with getauxval(AT_HWCAP) > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index b1adc51..1bed772 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -326,7 +326,12 @@ static void __init hyp_mode_check(void) > > void __init smp_cpus_done(unsigned int max_cpus) > { > - pr_info("SMP: Total of %d processors activated.\n", num_online_cpus()); > + unsigned long bogosum = loops_per_jiffy * num_online_cpus(); > + > + pr_info("SMP: Total of %d processors activated (%lu.%02lu BogoMIPS).\n", > + num_online_cpus(), bogosum / (500000/HZ), > + (bogosum / (5000/HZ)) % 100); Can we drop this hunk? I don't see a pressing need to print this in dmesg. With those two changes: Acked-by: Will Deacon <will.deacon@arm.com> I guess this needs Cc'ing to stable, too. Thanks, Will
On 11/18/2015 10:47 AM, Will Deacon wrote: > Hello, > > On Wed, Nov 18, 2015 at 10:15:05AM -0800, Yang Shi wrote: >> As what Pavel Machek reported [1], some userspace applications depend on >> bogomips showed by /proc/cpuinfo. >> >> Although there is much less legacy impact on aarch64 than arm, but it does >> break libvirt. >> >> Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 >> ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with >> some tweak due to context change. >> >> [1] https://lkml.org/lkml/2015/1/4/132 > > I lost this argument last time around, so I won't re-tread that path this > time around. I do, however, have some comments on the patch. > >> >> Signed-off-by: Yang Shi <yang.shi@linaro.org> >> --- >> arch/arm64/kernel/cpuinfo.c | 5 +++++ >> arch/arm64/kernel/smp.c | 7 ++++++- >> 2 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c >> index 706679d..8d4ba77 100644 >> --- a/arch/arm64/kernel/cpuinfo.c >> +++ b/arch/arm64/kernel/cpuinfo.c >> @@ -30,6 +30,7 @@ >> #include <linux/seq_file.h> >> #include <linux/sched.h> >> #include <linux/smp.h> >> +#include <linux/delay.h> >> >> /* >> * In case the boot CPU is hotpluggable, we record its initial state and >> @@ -112,6 +113,10 @@ static int c_show(struct seq_file *m, void *v) >> */ >> seq_printf(m, "processor\t: %d\n", i); >> >> + seq_printf(m, "BogoMIPS\t: %lu.%02lu\n\n", > > This double newline makes /proc/cpuinfo looks really odd. Can we just > have one, please? > >> + loops_per_jiffy / (500000UL/HZ), >> + loops_per_jiffy / (5000UL/HZ) % 100); >> + >> /* >> * Dump out the common processor features in a single line. >> * Userspace should read the hwcaps with getauxval(AT_HWCAP) >> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c >> index b1adc51..1bed772 100644 >> --- a/arch/arm64/kernel/smp.c >> +++ b/arch/arm64/kernel/smp.c >> @@ -326,7 +326,12 @@ static void __init hyp_mode_check(void) >> >> void __init smp_cpus_done(unsigned int max_cpus) >> { >> - pr_info("SMP: Total of %d processors activated.\n", num_online_cpus()); >> + unsigned long bogosum = loops_per_jiffy * num_online_cpus(); >> + >> + pr_info("SMP: Total of %d processors activated (%lu.%02lu BogoMIPS).\n", >> + num_online_cpus(), bogosum / (500000/HZ), >> + (bogosum / (5000/HZ)) % 100); > > Can we drop this hunk? I don't see a pressing need to print this in > dmesg. Sure, will solve them in v2. Thanks, Yang > > With those two changes: > > Acked-by: Will Deacon <will.deacon@arm.com> > > I guess this needs Cc'ing to stable, too. > > Thanks, > > Will >
On Wed, 18 Nov 2015, Will Deacon wrote: > Hello, > > On Wed, Nov 18, 2015 at 10:15:05AM -0800, Yang Shi wrote: > > As what Pavel Machek reported [1], some userspace applications depend on > > bogomips showed by /proc/cpuinfo. > > > > Although there is much less legacy impact on aarch64 than arm, but it does > > break libvirt. > > > > Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > > ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with > > some tweak due to context change. > > > > [1] https://lkml.org/lkml/2015/1/4/132 > > I lost this argument last time around, so I won't re-tread that path this > time around. No kidding. ;-) > I do, however, have some comments on the patch. > > > > > Signed-off-by: Yang Shi <yang.shi@linaro.org> > > --- > > arch/arm64/kernel/cpuinfo.c | 5 +++++ > > arch/arm64/kernel/smp.c | 7 ++++++- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > > index 706679d..8d4ba77 100644 > > --- a/arch/arm64/kernel/cpuinfo.c > > +++ b/arch/arm64/kernel/cpuinfo.c > > @@ -30,6 +30,7 @@ > > #include <linux/seq_file.h> > > #include <linux/sched.h> > > #include <linux/smp.h> > > +#include <linux/delay.h> > > > > /* > > * In case the boot CPU is hotpluggable, we record its initial state and > > @@ -112,6 +113,10 @@ static int c_show(struct seq_file *m, void *v) > > */ > > seq_printf(m, "processor\t: %d\n", i); > > > > + seq_printf(m, "BogoMIPS\t: %lu.%02lu\n\n", > > This double newline makes /proc/cpuinfo looks really odd. Can we just > have one, please? > > > + loops_per_jiffy / (500000UL/HZ), > > + loops_per_jiffy / (5000UL/HZ) % 100); Also, given nobody ever relied on any prior value here, can we at least print something here with some semblance of a meaning i.e. something related to the actual CPU speed and not some separate useless constant timer clock please? Nicolas
On Wed, Nov 18, 2015 at 04:55:10PM -0500, Nicolas Pitre wrote: > On Wed, 18 Nov 2015, Will Deacon wrote: > > On Wed, Nov 18, 2015 at 10:15:05AM -0800, Yang Shi wrote: > > > + loops_per_jiffy / (500000UL/HZ), > > > + loops_per_jiffy / (5000UL/HZ) % 100); > > Also, given nobody ever relied on any prior value here, can we at least > print something here with some semblance of a meaning i.e. something > related to the actual CPU speed and not some separate useless constant > timer clock please? There's also an argument that a 32-bit application should see the same BogoMIPS value under an arm64 kernel as an arm kernel running on the same machine. But I was never clear about what this value was intended to mean anyway, despite all the shouting :) Right now, I'm happy with anything that fixes libvirt and backports easily. Will
On 11/18/15, 1:15 PM, Yang Shi wrote: > As what Pavel Machek reported [1], some userspace applications depend on > bogomips showed by /proc/cpuinfo. > > Although there is much less legacy impact on aarch64 than arm, but it does > break libvirt. > > Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with > some tweak due to context change. On a total tangent, it would be ideal to (eventually) have something reported in /proc/cpuinfo or dmesg during boot that does "accurately" map back to the underlying core frequency (as opposed to the generic timer frequency). I have seen almost countless silly situations in the industry (external to my own organization) in which someone has taken a $VENDOR_X reference system that they're not supposed to run benchmarks on, and they've done it anyway. But usually on some silicon that's clocked multiples under what production would be. Then silly rumors about performance get around because nobody can do simple arithmetic and notice that they ought to have at least divided by some factor. Whether we do this through one of the platform tables or otherwise (multiple vendor EFI firmwares are being modified to make this much more glaringly obvious in the GUI view of system configuration so that when they do things they shouldn't, it's at least in the output) we should ultimately make sure that idiots at least have a fighting chance of noticing that they're actually running at 1GHz, and not 2 or 3GHz. Jon.
On Wed, Nov 25, 2015 at 12:45:13AM -0500, Jon Masters wrote: > On 11/18/15, 1:15 PM, Yang Shi wrote: > > >As what Pavel Machek reported [1], some userspace applications depend on > >bogomips showed by /proc/cpuinfo. > > > >Although there is much less legacy impact on aarch64 than arm, but it does > >break libvirt. > > > >Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > >("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with > >some tweak due to context change. > > On a total tangent, it would be ideal to (eventually) have something > reported in /proc/cpuinfo or dmesg during boot that does "accurately" map > back to the underlying core frequency (as opposed to the generic timer > frequency). I'm fine with this if someone proposes a (sane) patch. But it wouldn't be for stable.
On Wed, Nov 25, 2015 at 12:45:13AM -0500, Jon Masters wrote: > On 11/18/15, 1:15 PM, Yang Shi wrote: > > >As what Pavel Machek reported [1], some userspace applications depend on > >bogomips showed by /proc/cpuinfo. > > > >Although there is much less legacy impact on aarch64 than arm, but it does > >break libvirt. > > > >Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > >("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with > >some tweak due to context change. > > On a total tangent, it would be ideal to (eventually) have something > reported in /proc/cpuinfo or dmesg during boot that does "accurately" map > back to the underlying core frequency (as opposed to the generic timer > frequency). Absolutely not. You need to read the discussion that we had with Linus when someone reported that ARMs /proc/cpuinfo no longer reported it. Linus says that Bogomips reports the calibration value for udelay(), period. Whether that's a software loop, or a hardware timer is neither here nor there. It's nothing to do with the CPU clock rate. So, if your timer runs slowly, but your CPU is at the top end, a report of 6 Bogomips is (as far as Linus is concerned) absolutely correct, and we have no business at all as kernel developers coming up with some fake or real value to make it report as the CPU clock rate.
On 25 November 2015 at 07:45, Jon Masters <jcm@jonmasters.org> wrote: > On a total tangent, it would be ideal to (eventually) have something > reported in /proc/cpuinfo or dmesg during boot that does "accurately" map > back to the underlying core frequency (as opposed to the generic timer > frequency). Try: grep . /sys/devices/system/cpu/cpu?/cpufreq/scaling_cur_freq
On Wed, 25 Nov 2015, Jon Masters wrote: > On 11/18/15, 1:15 PM, Yang Shi wrote: > > > As what Pavel Machek reported [1], some userspace applications depend on > > bogomips showed by /proc/cpuinfo. > > > > Although there is much less legacy impact on aarch64 than arm, but it does > > break libvirt. > > > > Basically, this patch reverts commit > > 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > > ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with > > some tweak due to context change. > > On a total tangent, it would be ideal to (eventually) have something reported > in /proc/cpuinfo or dmesg during boot that does "accurately" map back to the > underlying core frequency (as opposed to the generic timer frequency). I have > seen almost countless silly situations in the industry (external to my own > organization) in which someone has taken a $VENDOR_X reference system that > they're not supposed to run benchmarks on, and they've done it anyway. But > usually on some silicon that's clocked multiples under what production would > be. Then silly rumors about performance get around because nobody can do > simple arithmetic and notice that they ought to have at least divided by some > factor. Be my guest my friend. According to the common wisdom, the bogomips reporting is completely senseless at this point and no one should expect anything useful from it. Therefore I attempted to rehabilitate some meaning into it given that we just can't get rid of it either and it continues to cause dammage. You certainly saw where that has led me. Nicolas
On 11/25/2015 7:16 AM, Nicolas Pitre wrote: > On Wed, 25 Nov 2015, Jon Masters wrote: > >> On 11/18/15, 1:15 PM, Yang Shi wrote: >> >>> As what Pavel Machek reported [1], some userspace applications depend on >>> bogomips showed by /proc/cpuinfo. >>> >>> Although there is much less legacy impact on aarch64 than arm, but it does >>> break libvirt. >>> >>> Basically, this patch reverts commit >>> 326b16db9f69fd0d279be873c6c00f88c0a4aad5 >>> ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with >>> some tweak due to context change. >> >> On a total tangent, it would be ideal to (eventually) have something reported >> in /proc/cpuinfo or dmesg during boot that does "accurately" map back to the >> underlying core frequency (as opposed to the generic timer frequency). I have >> seen almost countless silly situations in the industry (external to my own >> organization) in which someone has taken a $VENDOR_X reference system that >> they're not supposed to run benchmarks on, and they've done it anyway. But >> usually on some silicon that's clocked multiples under what production would >> be. Then silly rumors about performance get around because nobody can do >> simple arithmetic and notice that they ought to have at least divided by some >> factor. > > Be my guest my friend. > > According to the common wisdom, the bogomips reporting is completely > senseless at this point and no one should expect anything useful from > it. Therefore I attempted to rehabilitate some meaning into it given > that we just can't get rid of it either and it continues to cause > dammage. You certainly saw where that has led me. Or we may create a new one, i.e. "cpu MHz" like x86? Then we keep both in cpuinfo so that the userspace could adopt it gradually? Thanks, Yang > > > Nicolas >
On Wed, 25 Nov 2015, Shi, Yang wrote: > On 11/25/2015 7:16 AM, Nicolas Pitre wrote: > > On Wed, 25 Nov 2015, Jon Masters wrote: > > > > > On 11/18/15, 1:15 PM, Yang Shi wrote: > > > > > > > As what Pavel Machek reported [1], some userspace applications depend on > > > > bogomips showed by /proc/cpuinfo. > > > > > > > > Although there is much less legacy impact on aarch64 than arm, but it > > > > does > > > > break libvirt. > > > > > > > > Basically, this patch reverts commit > > > > 326b16db9f69fd0d279be873c6c00f88c0a4aad5 > > > > ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but > > > > with > > > > some tweak due to context change. > > > > > > On a total tangent, it would be ideal to (eventually) have something > > > reported > > > in /proc/cpuinfo or dmesg during boot that does "accurately" map back to > > > the > > > underlying core frequency (as opposed to the generic timer frequency). I > > > have > > > seen almost countless silly situations in the industry (external to my own > > > organization) in which someone has taken a $VENDOR_X reference system that > > > they're not supposed to run benchmarks on, and they've done it anyway. But > > > usually on some silicon that's clocked multiples under what production > > > would > > > be. Then silly rumors about performance get around because nobody can do > > > simple arithmetic and notice that they ought to have at least divided by > > > some > > > factor. > > > > Be my guest my friend. > > > > According to the common wisdom, the bogomips reporting is completely > > senseless at this point and no one should expect anything useful from > > it. Therefore I attempted to rehabilitate some meaning into it given > > that we just can't get rid of it either and it continues to cause > > dammage. You certainly saw where that has led me. > > Or we may create a new one, i.e. "cpu MHz" like x86? Then we keep both in > cpuinfo so that the userspace could adopt it gradually? The problem is that CPU MHz is not known/discoverable on all platforms. The initial spirit behind bogomips was close to CPU clock with rough precision that could be determined at run time. But CPU MHz, when available, has the merit of not being open to interpretation. Nicolas
On Wed, 2015-11-25 at 13:09 -0500, Nicolas Pitre wrote: > But CPU MHz, when available, has the merit of not being open to > interpretation. Current MHz, maximum MHz or 'turbo' MHz? ;-)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index 706679d..8d4ba77 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -30,6 +30,7 @@ #include <linux/seq_file.h> #include <linux/sched.h> #include <linux/smp.h> +#include <linux/delay.h> /* * In case the boot CPU is hotpluggable, we record its initial state and @@ -112,6 +113,10 @@ static int c_show(struct seq_file *m, void *v) */ seq_printf(m, "processor\t: %d\n", i); + seq_printf(m, "BogoMIPS\t: %lu.%02lu\n\n", + loops_per_jiffy / (500000UL/HZ), + loops_per_jiffy / (5000UL/HZ) % 100); + /* * Dump out the common processor features in a single line. * Userspace should read the hwcaps with getauxval(AT_HWCAP) diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index b1adc51..1bed772 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -326,7 +326,12 @@ static void __init hyp_mode_check(void) void __init smp_cpus_done(unsigned int max_cpus) { - pr_info("SMP: Total of %d processors activated.\n", num_online_cpus()); + unsigned long bogosum = loops_per_jiffy * num_online_cpus(); + + pr_info("SMP: Total of %d processors activated (%lu.%02lu BogoMIPS).\n", + num_online_cpus(), bogosum / (500000/HZ), + (bogosum / (5000/HZ)) % 100); + setup_cpu_features(); hyp_mode_check(); apply_alternatives_all();
As what Pavel Machek reported [1], some userspace applications depend on bogomips showed by /proc/cpuinfo. Although there is much less legacy impact on aarch64 than arm, but it does break libvirt. Basically, this patch reverts commit 326b16db9f69fd0d279be873c6c00f88c0a4aad5 ("arm64: delay: don't bother reporting bogomips in /proc/cpuinfo"), but with some tweak due to context change. [1] https://lkml.org/lkml/2015/1/4/132 Signed-off-by: Yang Shi <yang.shi@linaro.org> --- arch/arm64/kernel/cpuinfo.c | 5 +++++ arch/arm64/kernel/smp.c | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-)