2017-06-08 11:43:17

by Zhenzhong Duan

Subject: [PATCH] Calling check_system_tsc_reliable before unsynchronized_tsc

unsynchronized_tsc() checks value of tsc_clocksource_reliable which is set by
check_system_tsc_reliable(). It's better to move check_system_tsc_reliable() at
front, or else this check makes no sense.

Though X86_FEATURE_CONSTANT_TSC is usually set for TSC reliable system, just in
case.

Signed-off-by: Zhenzhong Duan <[email protected]>
---
arch/x86/kernel/tsc.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 714dfba..a316bdd 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1412,11 +1412,11 @@ void __init tsc_init(void)

use_tsc_delay();

+ check_system_tsc_reliable();
+
if (unsynchronized_tsc())
mark_tsc_unstable("TSCs unsynchronized");

- check_system_tsc_reliable();
-
detect_art();
}

--
1.7.3

2017-06-12 16:04:11

by Thomas Gleixner

Subject: Re: [PATCH] Calling check_system_tsc_reliable before unsynchronized_tsc

On Thu, 8 Jun 2017, Zhenzhong Duan wrote:

> unsynchronized_tsc() checks value of tsc_clocksource_reliable which is set by
> check_system_tsc_reliable(). It's better to move check_system_tsc_reliable()
> at
> front, or else this check makes no sense.
>
> Though X86_FEATURE_CONSTANT_TSC is usually set for TSC reliable system, just
> in
> case.
>
> Signed-off-by: Zhenzhong Duan <[email protected]>
> ---
> arch/x86/kernel/tsc.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 714dfba..a316bdd 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1412,11 +1412,11 @@ void __init tsc_init(void)
> use_tsc_delay();
> + check_system_tsc_reliable();
> +
> if (unsynchronized_tsc())
> mark_tsc_unstable("TSCs unsynchronized");
> - check_system_tsc_reliable();
> -

What kind of patch is that? Definitely nothing which can be applied.

Thanks,

tglx

2017-06-13 02:47:49

by Zhenzhong Duan

Subject: Re: [PATCH] Calling check_system_tsc_reliable before unsynchronized_tsc

?? 2017/6/13 0:03, Thomas Gleixner д??:
> On Thu, 8 Jun 2017, Zhenzhong Duan wrote:
>
>> unsynchronized_tsc() checks value of tsc_clocksource_reliable which is set by
>> check_system_tsc_reliable(). It's better to move check_system_tsc_reliable()
>> at
>> front, or else this check makes no sense.
>>
>> Though X86_FEATURE_CONSTANT_TSC is usually set for TSC reliable system, just
>> in
>> case.
>>
>> Signed-off-by: Zhenzhong Duan <[email protected]>
>> ---
>> arch/x86/kernel/tsc.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index 714dfba..a316bdd 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -1412,11 +1412,11 @@ void __init tsc_init(void)
>> use_tsc_delay();
>> + check_system_tsc_reliable();
>> +
>> if (unsynchronized_tsc())
>> mark_tsc_unstable("TSCs unsynchronized");
>> - check_system_tsc_reliable();
>> -
> What kind of patch is that? Definitely nothing which can be applied.
To avoid using unassigned variable tsc_clocksource_reliable.
Or what's the purpose of if (tsc_clocksource_reliable) check in
unsynchronized_tsc()?

--
thanks
zduan

2017-06-20 13:14:38

by Thomas Gleixner

Subject: Re: [PATCH] Calling check_system_tsc_reliable before unsynchronized_tsc

On Tue, 13 Jun 2017, Zhenzhong Duan wrote:
> ?? 2017/6/13 0:03, Thomas Gleixner д??:
> > On Thu, 8 Jun 2017, Zhenzhong Duan wrote:
> >
> > > unsynchronized_tsc() checks value of tsc_clocksource_reliable which is set
> > > by
> > > check_system_tsc_reliable(). It's better to move
> > > check_system_tsc_reliable()
> > > at
> > > front, or else this check makes no sense.
> > >
> > > Though X86_FEATURE_CONSTANT_TSC is usually set for TSC reliable system,
> > > just
> > > in
> > > case.
> > >
> > > Signed-off-by: Zhenzhong Duan <[email protected]>
> > > ---
> > > arch/x86/kernel/tsc.c | 4 ++--
> > > 1 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > index 714dfba..a316bdd 100644
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -1412,11 +1412,11 @@ void __init tsc_init(void)
> > > use_tsc_delay();
> > > + check_system_tsc_reliable();
> > > +
> > > if (unsynchronized_tsc())
> > > mark_tsc_unstable("TSCs unsynchronized");
> > > - check_system_tsc_reliable();
> > > -
> > What kind of patch is that? Definitely nothing which can be applied.
> To avoid using unassigned variable tsc_clocksource_reliable.
> Or what's the purpose of if (tsc_clocksource_reliable) check in
> unsynchronized_tsc()?

That's not the question. The patch format itself is broken:

@@ -1412,11 +1412,11 @@ void __init tsc_init(void)
use_tsc_delay();
+ check_system_tsc_reliable();
+
if (unsynchronized_tsc())
mark_tsc_unstable("TSCs unsynchronized");
- check_system_tsc_reliable();
-

That is not a patch created with diff -u or any other mechanism
which creates properly formatted patches.

Thanks,

tglx