Re: [PATCH v4 1/2] arch, x86, tsc deadline clockevent dev: eliminate frequency roundoff error

[Nicolai Stange <nicstange@gmail.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 14/07/2016 17:22, Nicolai Stange wrote:
>> In setup_APIC_timer(), the registered clockevent device's frequency
>> is calculated by first dividing tsc_khz by TSC_DIVISOR and multiplying
>> it with 1000 afterwards:
>> 
>>   (tsc_khz / TSC_DIVISOR) * 1000
>> 
>> The multiplication with 1000 is done for converting from kHz to Hz and the
>> division by TSC_DIVISOR is carried out in order to make sure that the final
>> result fits into an u32.
>> 
>> However, with the order given in this calculation, the roundoff error
>> introduced by the division gets magnified by a factor of 1000 by the
>> following multiplication. Thus, reversing the order of the division and
>> the multiplication a la
>> 
>>   (tsc_khz * 1000) / TSC_DIVISOR
>> 
>> reduces the roundoff error already.
>> 
>> Furthermore, if TSC_DIVISOR divides 1000, associativity holds
>> 
>>   (tsc_khz * 1000) / TSC_DIVISOR = tsc_khz * (1000 / TSC_DIVISOR)
>> 
>> and thus, the roundoff error even vanishes and the whole operation can be
>> carried out within 32 bits.
>> 
>> The powers of two that divide 1000 are 2, 4 and 8. A value of 8 for
>> TSC_DIVISOR still allows for TSC frequencies up to
>> 2^32 / 10^9ns * 8 = 34.4GHz which is way larger than anything to expect
>> in the next years.
>> 
>> Thus, replace the current TSC_DIVISOR value of 32 by 8. Reverse the
>> order of the divison and the multiplication in the calculation of the
>> registered clockevent device's frequency.
>> 
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>>  arch/x86/kernel/apic/apic.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
>> index 89a5bce..39517c0 100644
>> --- a/arch/x86/kernel/apic/apic.c
>> +++ b/arch/x86/kernel/apic/apic.c
>> @@ -311,7 +311,7 @@ int lapic_get_maxlvt(void)
>>  
>>  /* Clock divisor */
>>  #define APIC_DIVISOR 16
>> -#define TSC_DIVISOR  32
>> +#define TSC_DIVISOR  8
>>  
>>  /*
>>   * This function sets up the local APIC timer, with a timeout of
>> @@ -563,7 +563,7 @@ static void setup_APIC_timer(void)
>>  				    CLOCK_EVT_FEAT_DUMMY);
>>  		levt->set_next_event = lapic_next_deadline;
>>  		clockevents_config_and_register(levt,
>> -						(tsc_khz / TSC_DIVISOR) * 1000,
>> +						tsc_khz * (1000 / TSC_DIVISOR),
>>  						0xF, ~0UL);
>>  	} else
>>  		clockevents_register_device(levt);
>> 
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thank you very much!


> and I suggest even CCing this to stable as it's pretty safe and an
> improvement.

I've just collected both, the "coarse" TSC frequencies obtained through
the PIT calibration as well as the "refined" TSC frequencies across 1000
boots (CPU: Intel i7-4800MQ, Haswell).

My interpretation of the outcome is that the application of this patch
alone (i.e. w/o [2/2] "arch, x86, tsc: inform TSC deadline clockevent
device about recalibration") is not only worthless but has the potential
for making things worse.

1. Worthlessness
The "coarse" TSC frequency's mean is 2693.801 MHz with a standard
deviation of 122 kHz. Given that this patch changes the outcome by at
most 31kHz (15.5 kHz on average), the error corrected is relatively
small when compared to the standard deviation.

2. Potential for making things worse
Note that this patch never decreases frequency values. The roundoff
error fixed by this patch turns the "coarse" frequency mean of 2693.801
MHz into 2693.785 MHz. This errorneous value is significantly closer to
the "refined" frequency's mean of 2693.773 MHz (sdev: 5.8 kHz). Thus,
the two errors addressed by this series' two patches seem to be of
opposite direction on average and cancel each other to some extent
-> picking only one of the two makes the frequency bias worse on
average.


So, since it is unlikely that [2/2] ("arch, x86, tsc: inform TSC
deadline clockevent device about recalibration") is considered safe
enough for inclusion into -stable, I'd recommend not to send this
one (i.e. [1/2]) to stable either.


Thanks,

Nicolai