Re: [Xen-devel] [PATCH 2/4] x86/time: reduce rounding errors in calculations
On 16/03/2020 08:58, Jan Beulich wrote: > On 13.03.2020 16:14, Andrew Cooper wrote: >> On 13/03/2020 09:25, Jan Beulich wrote: >>> Plain (unsigned) integer division simply truncates the results. The >>> overall errors are smaller though if we use proper rounding. (Extend >>> this to the purely cosmetic aspect of time.c's freq_string(), which >>> before this change I've frequently observed to report e.g. NN.999MHz >>> HPET clock speeds.) >>> >>> Signed-off-by: Jan Beulich >>> --- >>> We could switch at least the first div/rem pair in calibrate_APIC_clock() >>> to use do_div(), but this would imply switching bus_freq (and then also >>> result) to unsigned int (as the divisor has to be 32-bit). While I think >>> there's pretty little risk for bus frequencies to go beyond 4GHz in the >>> next so many years, I still wasn't certain enough this would be a welcome >>> change. >> >> Honestly, do_div() is very easy to get wrong (and in security relevant >> ways in Linux). I'd advocate for phasing its use out, irrespective of >> this frequency concern. >> >> As for 4GHz, I think its unlikely, but better to be safe in the code. >> >>> >>> --- a/xen/arch/x86/apic.c >>> +++ b/xen/arch/x86/apic.c >>> @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v >>> /* set up multipliers for accurate timer code */ >>> bus_freq = result*HZ; >>> bus_cycle = (u32) (1LL/bus_freq); /* in pico seconds */ >>> +bus_cycle += (1UL % bus_freq) * 2 > bus_freq; >> >> These two differ in signedness of the numerator. GCC seems to cope with >> combining the two into a single div instruction, but I think we should >> be consistent with the constant nevertheless. > > IOW you'd like me to change the other line too, to have a UL > suffix? If so, at that point I'd drop the stray cast, too. That would be fine yes. ~Andrew > >> Otherwise, Acked-by: Andrew Cooper > > Thanks, but please let me know if the above is a correct > understanding of mine. > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] x86/time: reduce rounding errors in calculations
On 13.03.2020 16:14, Andrew Cooper wrote: > On 13/03/2020 09:25, Jan Beulich wrote: >> Plain (unsigned) integer division simply truncates the results. The >> overall errors are smaller though if we use proper rounding. (Extend >> this to the purely cosmetic aspect of time.c's freq_string(), which >> before this change I've frequently observed to report e.g. NN.999MHz >> HPET clock speeds.) >> >> Signed-off-by: Jan Beulich >> --- >> We could switch at least the first div/rem pair in calibrate_APIC_clock() >> to use do_div(), but this would imply switching bus_freq (and then also >> result) to unsigned int (as the divisor has to be 32-bit). While I think >> there's pretty little risk for bus frequencies to go beyond 4GHz in the >> next so many years, I still wasn't certain enough this would be a welcome >> change. > > Honestly, do_div() is very easy to get wrong (and in security relevant > ways in Linux). I'd advocate for phasing its use out, irrespective of > this frequency concern. > > As for 4GHz, I think its unlikely, but better to be safe in the code. > >> >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v >> /* set up multipliers for accurate timer code */ >> bus_freq = result*HZ; >> bus_cycle = (u32) (1LL/bus_freq); /* in pico seconds */ >> +bus_cycle += (1UL % bus_freq) * 2 > bus_freq; > > These two differ in signedness of the numerator. GCC seems to cope with > combining the two into a single div instruction, but I think we should > be consistent with the constant nevertheless. IOW you'd like me to change the other line too, to have a UL suffix? If so, at that point I'd drop the stray cast, too. > Otherwise, Acked-by: Andrew Cooper Thanks, but please let me know if the above is a correct understanding of mine. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] x86/time: reduce rounding errors in calculations
On 13/03/2020 09:25, Jan Beulich wrote: > Plain (unsigned) integer division simply truncates the results. The > overall errors are smaller though if we use proper rounding. (Extend > this to the purely cosmetic aspect of time.c's freq_string(), which > before this change I've frequently observed to report e.g. NN.999MHz > HPET clock speeds.) > > Signed-off-by: Jan Beulich > --- > We could switch at least the first div/rem pair in calibrate_APIC_clock() > to use do_div(), but this would imply switching bus_freq (and then also > result) to unsigned int (as the divisor has to be 32-bit). While I think > there's pretty little risk for bus frequencies to go beyond 4GHz in the > next so many years, I still wasn't certain enough this would be a welcome > change. Honestly, do_div() is very easy to get wrong (and in security relevant ways in Linux). I'd advocate for phasing its use out, irrespective of this frequency concern. As for 4GHz, I think its unlikely, but better to be safe in the code. > > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v > /* set up multipliers for accurate timer code */ > bus_freq = result*HZ; > bus_cycle = (u32) (1LL/bus_freq); /* in pico seconds */ > +bus_cycle += (1UL % bus_freq) * 2 > bus_freq; These two differ in signedness of the numerator. GCC seems to cope with combining the two into a single div instruction, but I think we should be consistent with the constant nevertheless. Otherwise, Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/4] x86/time: reduce rounding errors in calculations
Plain (unsigned) integer division simply truncates the results. The
overall errors are smaller though if we use proper rounding. (Extend
this to the purely cosmetic aspect of time.c's freq_string(), which
before this change I've frequently observed to report e.g. NN.999MHz
HPET clock speeds.)
Signed-off-by: Jan Beulich
---
We could switch at least the first div/rem pair in calibrate_APIC_clock()
to use do_div(), but this would imply switching bus_freq (and then also
result) to unsigned int (as the divisor has to be 32-bit). While I think
there's pretty little risk for bus frequencies to go beyond 4GHz in the
next so many years, I still wasn't certain enough this would be a welcome
change.
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v
/* set up multipliers for accurate timer code */
bus_freq = result*HZ;
bus_cycle = (u32) (1LL/bus_freq); /* in pico seconds */
+bus_cycle += (1UL % bus_freq) * 2 > bus_freq;
bus_scale = (1000*262144)/bus_cycle;
+bus_scale += ((1000 * 262144) % bus_cycle) * 2 > bus_cycle;
apic_printk(APIC_VERBOSE, ". bus_scale = %#x\n", bus_scale);
/* reset APIC to zero timeout value */
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -799,9 +799,9 @@ u64 __init hpet_setup(void)
hpet_resume(hpet_boot_cfg);
hpet_rate = 1000ULL; /* 10^15 */
-(void)do_div(hpet_rate, hpet_period);
+last = do_div(hpet_rate, hpet_period);
-return hpet_rate;
+return hpet_rate + (last * 2 > hpet_period);
}
void hpet_resume(u32 *boot_cfg)
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -275,7 +275,10 @@ static char *freq_string(u64 freq)
{
static char s[20];
unsigned int x, y;
-y = (unsigned int)do_div(freq, 100) / 1000;
+
+if ( do_div(freq, 1000) > 500 )
+++freq;
+y = (unsigned int)do_div(freq, 1000);
x = (unsigned int)freq;
snprintf(s, sizeof(s), "%u.%03uMHz", x, y);
return s;
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
