Re: [PATCH v7 6/8] x86: tsc: Always Running Timer (ART) correlated clocksource

[Andy Lutomirski]

On Mon, Feb 22, 2016 at 6:38 PM, Christopher Hall
<christopher.s.hall@xxxxxxxxx> wrote:
> On Thu, 18 Feb 2016 13:11:33 -0800, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>>>
>>> +#define X86_FEATURE_INVARIANT_TSC (7*32+4) /* Intel Invariant TSC */
>
>
> This is removed. It was basically an alias for NONSTOP_TSC and not needed.
>
>>
>>> +/*
>>> + * Convert ART to TSC given numerator/denominator found in detect_art()
>>> + */
>>> +struct system_counterval_t convert_art_to_tsc(cycle_t art)
>>> +{
>>> +       u64 tmp, res, rem;
>>> +
>>> +       rem = do_div(art, art_to_tsc_denominator);
>>> +
>>> +       res = art * art_to_tsc_numerator;
>>> +       tmp = rem * art_to_tsc_numerator;
>>> +
>>> +       do_div(tmp, art_to_tsc_denominator);
>>> +       res += tmp;
>>> +
>>> +       return (struct system_counterval_t) {.cs =
>>> art_related_clocksource,
>>> +                       .cycles = res};
>>
>>
>> The SDM and the patch description both mention an offset "k".  Shouldn't
>> this code at least have a comment about how it deals with the k != 0 case?
>
>
> I don't deal with the k != 0 case. I assume that IA32 TSC adjust MSR is 0
> because it's almost always a *bad idea* to change it. I've discussed this
> with a few other developers and there is some consensus agreeing. From an
> earlier related thread Peter Zijlstra asserts that TSC adjust "had
> better" be 0.(http://lkml.iu.edu/hypermail/linux/kernel/1507.3/03734.html).

I'm having trouble finding that in the link you sent.

>
> Do we really need to accommodate BIOS's that do this?

There are three interesting cases that I can think of:

1. Crappy BIOS that sets TSC_ADJUST.  As the not-so-proud owner of a
piece of crap motherboard that actively messes with the TSC, I don't
trust BIOS.

2. Hypervisors.  What if we're running as a guest with an ART-using
NIC passed through?

3. Hypothetical improved future kernel that politely uses TSC_ADJUST
to keep the TSC from going backwards across suspend/resume.

--Andy