linux-kernel - Re: [PATCH v8 6/6] x86/tsc: use tsc early

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6813baaf-909e-e32e-b074-6c466ceabcc6@cn.fujitsu.com>
Date:   Wed, 15 Nov 2017 11:53:07 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Pavel Tatashin <pasha.tatashin@...cle.com>
CC:     Steve Sistare <steven.sistare@...cle.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        <linux@...linux.org.uk>, <schwidefsky@...ibm.com>,
        <heiko.carstens@...ibm.com>, John Stultz <john.stultz@...aro.org>,
        <sboyd@...eaurora.org>, <x86@...nel.org>,
        <linux-kernel@...r.kernel.org>, <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>, <hpa@...or.com>
Subject: Re: [PATCH v8 6/6] x86/tsc: use tsc early

Hi Pavel,

At 11/15/2017 05:46 AM, Pavel Tatashin wrote:
> Hi Dou,
>
> Great comments, my replies below:
>
>>>  static inline unsigned long long paravirt_sched_clock(void)
>>>  {
>>> -       return PVOP_CALL0(unsigned long long, pv_time_ops.sched_clock);
>>> +       return PVOP_CALL0(unsigned long long,
>>> pv_time_ops.active_sched_clock);
>>>  }
>>>
>>
>> Should in the 5th patch
>
> Actually, it has to be in patch 6, because otherwise patch 5 without
> patch 6 would cause native_sched_clock() to be used even when a
> platform specific clock is set, thus may cause performance regressions
> where it is not optimal to use tsc for clock.
>
Indeed.

 >> +	tsc_early_disable();
 >> +	__sched_clock_offset = sched_clock_early() - sched_clock();
 >> +	pr_info("sched clock early is finished, offset[%lld.%09lds]\n", t, r);
 >> +}

s/sched clock early/early sched clock/

 >> +#endif /* CONFIG_X86_TSC */
>> Add definitions for the situation of X86_TSC = no :
>>
>> #else /* CONFIG_X86_TSC */
>> static inline void tsc_early_init(unsigned int khz) { }
>> static inline void tsc_early_fini(void) { }
>
> Excellent point, I totally forgot about  X86_TSC = no, however, a
> better fix is to simply remove #ifdef CONFIG_X86_TSC from my
> functions. Apparently, even with X86_TSC=no we can use TSC unless

Agree with removing #ifdef CONFIG_X86_TSC

> notsc kernel parameter is passed. This will be in the next patchset.
>
>>
>> According to tsc_early_delay_calibrate(), if (!boot_cpu_has(X86_FEATURE_TSC
>> || !tsc_khz ), tsc_early_init(tsc_khz)
>> will never be called, so here is  redundant.
>>
>>>                 return;
>>>         }
>>>
>>> @@ -1302,6 +1385,7 @@ void __init tsc_init(void)
>>>         if (!tsc_khz) {
>>>                 mark_tsc_unstable("could not calculate TSC khz");
>>>                 setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
>>> +               tsc_early_fini();
>>
>>
>> ditto
>
> Right, in both case we still want to call tsc_early_fini(). Because,
> it calls tsc_early_disable() even when tsc_early_init() was never
> called.  tsc_early_disable()  either sets static branch
> __tsc_early_static to false or changes active_sched_clock to be
> platform specific, depending on CONFIG_PARAVIRT.

Yes, the function names confused me, the actually purpose of
tsc_early_fini() is switching schedule clock to the final one. right?

How about replacing *return;* with *goto final_sched_clock;*

Thanks,
	dou.
>
>> BTW, seems you forgot to cc Peter Zijlstra <peterz@...radead.org> in both V7
>> and V8 patchsets.
>
> Thank you for noticing this! I will include Peter when I send out
> patchset version 9.
>
> Thank you,
> Pavel
>
>
>