[tip:perf/core] perf/x86: Clean up cap_user_time* setting

[tip:perf/core] perf/x86: Clean up cap_user_time* setting

[tip-bot for Peter Zijlstra]

Commit-ID:  d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
Gitweb:     http://git.kernel.org/tip/d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 3 Oct 2013 16:00:14 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 4 Oct 2013 09:58:55 +0200

perf/x86: Clean up cap_user_time* setting

Currently the cap_user_time_zero capability has different tests than
cap_user_time; even though they expose the exact same data.

Switch from CONSTANT && NONSTOP to sched_clock_stable to also deal
with multi cabinet machines and drop the tsc_disabled() check.. non of
this will work sanely without tsc anyway.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-nmgn0j0muo1r4c94vlfh23xy@git.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 897783b..9d84491 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1888,10 +1888,7 @@ void arch_perf_update_userpage(struct perf_event_mmap_page *userpg, u64 now)
 	userpg->cap_user_rdpmc = x86_pmu.attr_rdpmc;
 	userpg->pmc_width = x86_pmu.cntval_bits;
 
-	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
-		return;
-
-	if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
+	if (!sched_clock_stable)
 		return;
 
 	userpg->cap_user_time = 1;
@@ -1899,10 +1896,8 @@ void arch_perf_update_userpage(struct perf_event_mmap_page *userpg, u64 now)
 	userpg->time_shift = CYC2NS_SCALE_FACTOR;
 	userpg->time_offset = this_cpu_read(cyc2ns_offset) - now;
 
-	if (sched_clock_stable && !check_tsc_disabled()) {
-		userpg->cap_user_time_zero = 1;
-		userpg->time_zero = this_cpu_read(cyc2ns_offset);
-	}
+	userpg->cap_user_time_zero = 1;
+	userpg->time_zero = this_cpu_read(cyc2ns_offset);
 }
 
 /*

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[Adrian Hunter]

On 4/10/2013 8:31 p.m., tip-bot for Peter Zijlstra wrote:
> Commit-ID:  d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
> Gitweb:     http://git.kernel.org/tip/d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Thu, 3 Oct 2013 16:00:14 +0200
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 4 Oct 2013 09:58:55 +0200
>
> perf/x86: Clean up cap_user_time* setting
>
> Currently the cap_user_time_zero capability has different tests than
> cap_user_time; even though they expose the exact same data.
>
> Switch from CONSTANT && NONSTOP to sched_clock_stable to also deal
> with multi cabinet machines and drop the tsc_disabled() check.. non of
> this will work sanely without tsc anyway.

Unfortunately in the case that TSC is disabled, sched_clock is still
reported as stable, which means removing the tsc_disabled() check breaks
the capability bit. e.g.

$ dmesg | grep -i TSC
[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-3.11.0+ root=UUID=f1b4c71a-15aa-41a6-8898-cdde49966bce ro ignore_loglevel earlyprintk=ttyS0 console=ttyS0,115200n8 no_console_suspend notsc=1 sched_debug=1
[    0.000000] Kernel command line: BOOT_IMAGE=/boot/vmlinuz-3.11.0+ root=UUID=f1b4c71a-15aa-41a6-8898-cdde49966bce ro ignore_loglevel earlyprintk=ttyS0 console=ttyS0,115200n8 no_console_suspend notsc=1 sched_debug=1
[    0.000000] tsc: Kernel compiled with CONFIG_X86_TSC, cannot disable TSC completely
[    0.000000] tsc: Fast TSC calibration using PIT
[    0.004000] tsc: Detected 2594.166 MHz processor
[    0.224000] TSC deadline timer enabled
$ cat /proc/sched_debug | grep sched_clock_stable
sched_clock_stable                      : 1

>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Link: http://lkml.kernel.org/n/tip-nmgn0j0muo1r4c94vlfh23xy@git.kernel.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>   arch/x86/kernel/cpu/perf_event.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 897783b..9d84491 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1888,10 +1888,7 @@ void arch_perf_update_userpage(struct perf_event_mmap_page *userpg, u64 now)
>   	userpg->cap_user_rdpmc = x86_pmu.attr_rdpmc;
>   	userpg->pmc_width = x86_pmu.cntval_bits;
>
> -	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
> -		return;
> -
> -	if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
> +	if (!sched_clock_stable)
>   		return;
>
>   	userpg->cap_user_time = 1;
> @@ -1899,10 +1896,8 @@ void arch_perf_update_userpage(struct perf_event_mmap_page *userpg, u64 now)
>   	userpg->time_shift = CYC2NS_SCALE_FACTOR;
>   	userpg->time_offset = this_cpu_read(cyc2ns_offset) - now;
>
> -	if (sched_clock_stable && !check_tsc_disabled()) {
> -		userpg->cap_user_time_zero = 1;
> -		userpg->time_zero = this_cpu_read(cyc2ns_offset);
> -	}
> +	userpg->cap_user_time_zero = 1;
> +	userpg->time_zero = this_cpu_read(cyc2ns_offset);
>   }
>
>   /*
>

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[Peter Zijlstra]

On Fri, Oct 04, 2013 at 09:31:22PM +0300, Adrian Hunter wrote:
> On 4/10/2013 8:31 p.m., tip-bot for Peter Zijlstra wrote:
> >Commit-ID:  d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
> >Gitweb:     http://git.kernel.org/tip/d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
> >Author:     Peter Zijlstra <peterz@infradead.org>
> >AuthorDate: Thu, 3 Oct 2013 16:00:14 +0200
> >Committer:  Ingo Molnar <mingo@kernel.org>
> >CommitDate: Fri, 4 Oct 2013 09:58:55 +0200
> >
> >perf/x86: Clean up cap_user_time* setting
> >
> >Currently the cap_user_time_zero capability has different tests than
> >cap_user_time; even though they expose the exact same data.
> >
> >Switch from CONSTANT && NONSTOP to sched_clock_stable to also deal
> >with multi cabinet machines and drop the tsc_disabled() check.. non of
> >this will work sanely without tsc anyway.
> 
> Unfortunately in the case that TSC is disabled, sched_clock is still
> reported as stable, which means removing the tsc_disabled() check breaks
> the capability bit. e.g.

I'm wanting to hear from the x86 people on why we have this absurd knob
to begin with; but I'm tempted to simply disable all of perf if you
touch it.

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Oct 04, 2013 at 09:31:22PM +0300, Adrian Hunter wrote:
> > On 4/10/2013 8:31 p.m., tip-bot for Peter Zijlstra wrote:
> > >Commit-ID:  d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
> > >Gitweb:     http://git.kernel.org/tip/d8b11a0cbd1c66ce283eb9dabe0498dfa6483f32
> > >Author:     Peter Zijlstra <peterz@infradead.org>
> > >AuthorDate: Thu, 3 Oct 2013 16:00:14 +0200
> > >Committer:  Ingo Molnar <mingo@kernel.org>
> > >CommitDate: Fri, 4 Oct 2013 09:58:55 +0200
> > >
> > >perf/x86: Clean up cap_user_time* setting
> > >
> > >Currently the cap_user_time_zero capability has different tests than
> > >cap_user_time; even though they expose the exact same data.
> > >
> > >Switch from CONSTANT && NONSTOP to sched_clock_stable to also deal
> > >with multi cabinet machines and drop the tsc_disabled() check.. non of
> > >this will work sanely without tsc anyway.
> > 
> > Unfortunately in the case that TSC is disabled, sched_clock is still
> > reported as stable, which means removing the tsc_disabled() check breaks
> > the capability bit. e.g.
> 
> I'm wanting to hear from the x86 people on why we have this absurd knob 
> to begin with; but I'm tempted to simply disable all of perf if you 
> touch it.

I'm fully with you, please zap the 'notsc' boot option - it's an ancient 
relic, if any box is still broken with the TSC on we want to hear about it 
and fix it!

Thanks,

	Ingo

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[Peter Zijlstra]

On Sun, Oct 06, 2013 at 11:10:54AM +0200, Ingo Molnar wrote:
> I'm fully with you, please zap the 'notsc' boot option - it's an ancient 
> relic, if any box is still broken with the TSC on we want to hear about it 
> and fix it!

something like so?

---
Subject: x86: Remove 'notsc' option for X86_TSC=y kernels

The 'notsc' thing is an ancient relic, if there's still any hardware
that needs this we need to hear about it.

This only removes the option for X86_TSC=y kernels; X86_TSC=n kernels
can still use it to force remove the TSC capability flag.

Since this removes the tsc_disabled=1 assignment, also remove all
tsc_disabled>0 tests as those will never be true.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/kernel/tsc.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 930e5d48f560..693c0226b014 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -89,20 +89,7 @@ int check_tsc_unstable(void)
 }
 EXPORT_SYMBOL_GPL(check_tsc_unstable);
 
-int check_tsc_disabled(void)
-{
-	return tsc_disabled;
-}
-EXPORT_SYMBOL_GPL(check_tsc_disabled);
-
-#ifdef CONFIG_X86_TSC
-int __init notsc_setup(char *str)
-{
-	pr_warn("Kernel compiled with CONFIG_X86_TSC, cannot disable TSC completely\n");
-	tsc_disabled = 1;
-	return 1;
-}
-#else
+#ifndef CONFIG_X86_TSC
 /*
  * disable flag for tsc. Takes effect by clearing the TSC cpu flag
  * in cpu/common.c
@@ -112,9 +99,9 @@ int __init notsc_setup(char *str)
 	setup_clear_cpu_cap(X86_FEATURE_TSC);
 	return 1;
 }
-#endif
 
 __setup("notsc", notsc_setup);
+#endif
 
 static int no_sched_irq_time;
 
@@ -935,7 +922,7 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 
 static int __init init_tsc_clocksource(void)
 {
-	if (!cpu_has_tsc || tsc_disabled > 0 || !tsc_khz)
+	if (!cpu_has_tsc || !tsc_khz)
 		return 0;
 
 	if (tsc_clocksource_reliable)
@@ -998,9 +985,6 @@ void __init tsc_init(void)
 	for_each_possible_cpu(cpu)
 		set_cyc2ns_scale(cpu_khz, cpu);
 
-	if (tsc_disabled > 0)
-		return;
-
 	/* now allow native_sched_clock() to use rdtsc */
 	tsc_disabled = 0;
 

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[Peter Zijlstra]

On Mon, Oct 07, 2013 at 11:33:22AM +0200, Peter Zijlstra wrote:
> On Sun, Oct 06, 2013 at 11:10:54AM +0200, Ingo Molnar wrote:
> > I'm fully with you, please zap the 'notsc' boot option - it's an ancient 
> > relic, if any box is still broken with the TSC on we want to hear about it 
> > and fix it!
> 
> something like so?
> 
> ---
> Subject: x86: Remove 'notsc' option for X86_TSC=y kernels
> 
> The 'notsc' thing is an ancient relic, if there's still any hardware
> that needs this we need to hear about it.
> 
> This only removes the option for X86_TSC=y kernels; X86_TSC=n kernels
> can still use it to force remove the TSC capability flag.
> 
> Since this removes the tsc_disabled=1 assignment, also remove all
> tsc_disabled>0 tests as those will never be true.
> 
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>

OK, so I forgot to compile this this morning; but I now found
apicpmtimer somehow calls notsc_setup(). I've no idea why, but can
someone who knows this crap take a look?

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[H. Peter Anvin]

On 10/06/2013 02:10 AM, Ingo Molnar wrote:
>>
>> I'm wanting to hear from the x86 people on why we have this absurd knob 
>> to begin with; but I'm tempted to simply disable all of perf if you 
>> touch it.
> 
> I'm fully with you, please zap the 'notsc' boot option - it's an ancient 
> relic, if any box is still broken with the TSC on we want to hear about it 
> and fix it!
> 

Perhaps better would be to make the notsc option do what other feature
removal options do and just remove the CPU feature flag.

Early on we had a bunch of ad hoc behaviors for feature disabling.  They
are harmful and just wrong... "not present" and "disabled" should be the
same thing in 99% of all cases (in the case of the TSC one may wish to
set the CR4 bit which disables the TSC from userspace, but I don't think
"notsc" ever did that.)

However:

pr_warn("Kernel compiled with CONFIG_X86_TSC, cannot disable TSC
completely\n");

That is a total "say what"?

At one point it even said:

printk(KERN_WARNING "notsc: Kernel compiled with CONFIG_X86_TSC, "	
"cannot disable TSC.\n");

CONFIG_X86_TSC is a baseline control option; we shouldn't key
functionality off of it.  It's fine to say notsc -> no tracing, but
making it a compile-time key makes me a bit uphappy.  We cut off 386,
but cutting of 486 at this point makes me nervous.

	-hpa

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[Peter Zijlstra]

On Mon, Oct 07, 2013 at 10:22:06AM -0700, H. Peter Anvin wrote:
> CONFIG_X86_TSC is a baseline control option; we shouldn't key
> functionality off of it.  It's fine to say notsc -> no tracing, but
> making it a compile-time key makes me a bit uphappy.  We cut off 386,
> but cutting of 486 at this point makes me nervous.

The thing that annoys me about notsc is that it disables usage even if
its present.

I've no problem with 486 which simply doesn't have TSC and thus
cpu_has_tsc will be false and other stuff will happen -- and I don't
think my proposition would actually change anything there.

What is completely insane is people using notsc on say a haswell chip
and expecting something sane to happen.

So I'm not proposing we remove !cap_has_tsc support; all I'm proposing
is we remove the notsc knob that avoids using the TSC on perfectly good
hardware.

Re: [tip:perf/core] perf/x86: Clean up cap_user_time* setting

[H. Peter Anvin]

On 10/07/2013 10:40 AM, Peter Zijlstra wrote:
> On Mon, Oct 07, 2013 at 10:22:06AM -0700, H. Peter Anvin wrote:
>> CONFIG_X86_TSC is a baseline control option; we shouldn't key
>> functionality off of it.  It's fine to say notsc -> no tracing, but
>> making it a compile-time key makes me a bit uphappy.  We cut off 386,
>> but cutting of 486 at this point makes me nervous.
> 
> The thing that annoys me about notsc is that it disables usage even if
> its present.

That is *exactly* what it is supposed to do.  It seems to do so poorly
at this point.  It should behave exactly as if the TSC isn't there (and
thus any feature which depends on the TSC is not available, either.)

> I've no problem with 486 which simply doesn't have TSC and thus
> cpu_has_tsc will be false and other stuff will happen -- and I don't
> think my proposition would actually change anything there.

Except it will be a lot harder to test it.

> What is completely insane is people using notsc on say a haswell chip
> and expecting something sane to happen.
>
> So I'm not proposing we remove !cap_has_tsc support; all I'm proposing
> is we remove the notsc knob that avoids using the TSC on perfectly good
> hardware.

Uh, no, that's broken.

There seem to be a couple of issues here:

1. We have keyed functionality off CONFIG_X86_TSC, which is a baseline
control option -- this is basically a no-no.

2. With CONFIG_X86_TSC turned on, notsc is meaningless: it means running
in a configuration that the kernel compile doesn't support

3. Functionality that depends on the TSC should be disabled at runtime
if !CONFIG_X86_TSC.

4. Does CONFIG_X86_TSC even make sense?  It isn't listed in
required-features.h and is only present in a handful of places in the
kernel, and the trace-clock bits seem just plain wrong.  It isn't
valuable to unload TSC support (which isn't what CONFIG_X86_TSC does as
it is currently configured) even on embedded (non-TSC hardware is rare
today)... so why do we even bother?

	-hpa