linux-kernel - Re: [PATCH v12 09/11] x86/tsc: prepare for early sched

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]

Date:   Sat, 23 Jun 2018 14:49:41 -0400
From:   Pavel Tatashin <pasha.tatashin@...cle.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     steven.sistare@...cle.com, daniel.m.jordan@...cle.com,
        linux@...linux.org.uk, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, john.stultz@...aro.org,
        sboyd@...eaurora.org, x86@...nel.org, linux-kernel@...r.kernel.org,
        mingo@...hat.com, hpa@...or.com, douly.fnst@...fujitsu.com,
        peterz@...radead.org, prarit@...hat.com, feng.tang@...el.com,
        pmladek@...e.com, gnomes@...rguk.ukuu.org.uk,
        linux-s390@...r.kernel.org
Subject: Re: [PATCH v12 09/11] x86/tsc: prepare for early sched_clock

> Let me give you an example:
> 
>   When tsc_init() enables the usage of TSC for sched_clock() the
>   initialization of the tsc sched clock conversion data starts from zero
>   and not from the current jiffies based sched_clock() value. This makes
>   the timestamps jump backwards:
> 
>   [    0.010000] tsc: Detected 3192.137 MHz processor
>   [    0.011000] clocksource: tsc-early: mask:  ... 
>   [    0.002233] Calibrating delay loop (skipped), ....
> 
>   To address this, extend set_cyc2ns_scale() with an argument to base the
>   cyc2ns offset on the current sched_clock() value. During run time this
>   offset is 0 as the cyc2ns offset is based on the TSC sched_clock()
>   itself.
> 
> See? Precise and pure technical. No we/us/would/ and no irrelevant
> information.

Yes, thank you Thomas. I will update the changelog based on your suggestions, and no longer will impersonating my commit comments.

> 
>> Signed-off-by: Pavel Tatashin <pasha.tatashin@...cle.com>
>> ---
>>  arch/x86/kernel/tsc.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index 186395041725..654a01cc0358 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -133,7 +133,9 @@ static inline unsigned long long cycles_2_ns(unsigned long long cyc)
>>  	return ns;
>>  }
>>  
>> -static void set_cyc2ns_scale(unsigned long khz, int cpu, unsigned long long tsc_now)
>> +static void set_cyc2ns_scale(unsigned long khz, int cpu,
>> +			     unsigned long long tsc_now,
>> +			     unsigned long long sched_now)
> 
> sched_now is not a real good name for this as it's only used at
> initialization time. So the argument name should reflect this otherwise you
> wonder yourself when looking at that code 6 month from now, why it's 0 on
> all the run time call sites. init_offset or some other sensible name which
> makes the purpose entirely clear.
> 
>>  void __init tsc_init(void)
>>  {
>> -	u64 lpj, cyc;
>> +	u64 lpj, cyc, sch;
> 
> sch? what's wrong with sched_now or now? It's not that there is a 3 letter
> limit.

Sometimes I get caught into following the local style too much:

void __init tsc_init(void)
{
u64 lpj, cyc;
int cpu;

Hm, all the above are 3-letter variables, lets add another 3 letter one :)

I will change it to init_offset as you suggested above for set_cyc2ns_scale().

Also, I will address all the other comments that you provided in this series.

Thank you,
Pavel