| Message ID | 01baee92-9d7f-5a2c-d63f-1de390bc10e2@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: replace a few do_div() uses | expand |
On 12/01/2022 09:00, Jan Beulich wrote: > When the macro's "return value" is not used, the macro use can be > replaced by a simply division, avoiding some obfuscation. > > According to my observations, no change to generated code. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I like this change in principle, but see below. do_div() needs to be deleted, because it's far too easy screw up. At a bare minimum, it should be replaced with a static inline that takes it's first parameter by pointer, because then at least every callsite reads correctly in terms of the C language. > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency( > struct vcpu_time_info *info = &this_cpu(vcpu_info)->time; > uint64_t freq; > > - freq = 1000000000ULL << 32; > - do_div(freq, info->tsc_to_system_mul); > + freq = (1000000000ULL << 32) / info->tsc_to_system_mul; > if ( info->tsc_shift < 0 ) > freq <<= -info->tsc_shift; do_div()'s output is consumed here. I don't think this hunk is safe to convert. ~Andrew
On 12.01.2022 10:22, Andrew Cooper wrote: > On 12/01/2022 09:00, Jan Beulich wrote: >> When the macro's "return value" is not used, the macro use can be >> replaced by a simply division, avoiding some obfuscation. >> >> According to my observations, no change to generated code. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I like this change in principle, but see below. > > do_div() needs to be deleted, because it's far too easy screw up. At a > bare minimum, it should be replaced with a static inline that takes it's > first parameter by pointer, because then at least every callsite reads > correctly in terms of the C language. That ought to be a 2nd step, requiring agreement with Arm folks (and adjustments to their code). >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency( >> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time; >> uint64_t freq; >> >> - freq = 1000000000ULL << 32; >> - do_div(freq, info->tsc_to_system_mul); >> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul; >> if ( info->tsc_shift < 0 ) >> freq <<= -info->tsc_shift; > > do_div()'s output is consumed here. I don't think this hunk is safe to > convert. If by "output" you mean its "return value", then it clearly isn't consumed. And I continue to think that I did express correctly the effect do_div() did have on "freq". Jan
On 12.01.2022 10:28, Jan Beulich wrote: > On 12.01.2022 10:22, Andrew Cooper wrote: >> On 12/01/2022 09:00, Jan Beulich wrote: >>> When the macro's "return value" is not used, the macro use can be >>> replaced by a simply division, avoiding some obfuscation. >>> >>> According to my observations, no change to generated code. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> I like this change in principle, but see below. >> >> do_div() needs to be deleted, because it's far too easy screw up. At a >> bare minimum, it should be replaced with a static inline that takes it's >> first parameter by pointer, because then at least every callsite reads >> correctly in terms of the C language. > > That ought to be a 2nd step, requiring agreement with Arm folks (and > adjustments to their code). > >>> --- a/xen/arch/x86/time.c >>> +++ b/xen/arch/x86/time.c >>> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency( >>> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time; >>> uint64_t freq; >>> >>> - freq = 1000000000ULL << 32; >>> - do_div(freq, info->tsc_to_system_mul); >>> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul; >>> if ( info->tsc_shift < 0 ) >>> freq <<= -info->tsc_shift; >> >> do_div()'s output is consumed here. I don't think this hunk is safe to >> convert. > > If by "output" you mean its "return value", then it clearly isn't > consumed. And I continue to think that I did express correctly the > effect do_div() did have on "freq". I think I did address both points (the earlier one was actually more a remark imo anyway, not a request to change anything right in this patch), so may I please ask for an ack (or a response clarifying what I'm not understanding in what you have said)? Thanks, Jan
On 18/02/2022 08:39, Jan Beulich wrote: > On 12.01.2022 10:28, Jan Beulich wrote: >> On 12.01.2022 10:22, Andrew Cooper wrote: >>> On 12/01/2022 09:00, Jan Beulich wrote: >>>> When the macro's "return value" is not used, the macro use can be >>>> replaced by a simply division, avoiding some obfuscation. >>>> >>>> According to my observations, no change to generated code. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> I like this change in principle, but see below. >>> >>> do_div() needs to be deleted, because it's far too easy screw up. At a >>> bare minimum, it should be replaced with a static inline that takes it's >>> first parameter by pointer, because then at least every callsite reads >>> correctly in terms of the C language. >> That ought to be a 2nd step, requiring agreement with Arm folks (and >> adjustments to their code). >> >>>> --- a/xen/arch/x86/time.c >>>> +++ b/xen/arch/x86/time.c >>>> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency( >>>> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time; >>>> uint64_t freq; >>>> >>>> - freq = 1000000000ULL << 32; >>>> - do_div(freq, info->tsc_to_system_mul); >>>> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul; >>>> if ( info->tsc_shift < 0 ) >>>> freq <<= -info->tsc_shift; >>> do_div()'s output is consumed here. I don't think this hunk is safe to >>> convert. >> If by "output" you mean its "return value", then it clearly isn't >> consumed. And I continue to think that I did express correctly the >> effect do_div() did have on "freq". > I think I did address both points (the earlier one was actually more a > remark imo anyway, not a request to change anything right in this patch), > so may I please ask for an ack (or a response clarifying what I'm not > understanding in what you have said)? No - you're right. My mistake. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/cpu/intel.c +++ b/xen/arch/x86/cpu/intel.c @@ -392,9 +392,8 @@ static void intel_log_freq(const struct unsigned long long val = ecx; val *= ebx; - do_div(val, eax); printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n", - smp_processor_id(), ecx, ebx, eax, val); + smp_processor_id(), ecx, ebx, eax, val / eax); } else if ( ecx | eax | ebx ) { --- a/xen/arch/x86/hpet.c +++ b/xen/arch/x86/hpet.c @@ -105,10 +105,7 @@ custom_param("hpet", parse_hpet_param); static inline unsigned long div_sc(unsigned long ticks, unsigned long nsec, int shift) { - uint64_t tmp = ((uint64_t)ticks) << shift; - - do_div(tmp, nsec); - return (unsigned long) tmp; + return ((uint64_t)ticks << shift) / nsec; } /* --- a/xen/arch/x86/nmi.c +++ b/xen/arch/x86/nmi.c @@ -292,10 +292,9 @@ static void clear_msr_range(unsigned int static inline void write_watchdog_counter(const char *descr) { - u64 count = (u64)cpu_khz * 1000; + uint64_t count = cpu_khz * 1000ULL / nmi_hz; - do_div(count, nmi_hz); - if(descr) + if ( descr ) Dprintk("setting %s to -%#"PRIx64"\n", descr, count); wrmsrl(nmi_perfctr_msr, 0 - count); } --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency( struct vcpu_time_info *info = &this_cpu(vcpu_info)->time; uint64_t freq; - freq = 1000000000ULL << 32; - do_div(freq, info->tsc_to_system_mul); + freq = (1000000000ULL << 32) / info->tsc_to_system_mul; if ( info->tsc_shift < 0 ) freq <<= -info->tsc_shift; else @@ -2173,8 +2172,7 @@ void __init early_time_init(void) set_time_scale(&t->tsc_scale, tmp); t->stamp.local_tsc = boot_tsc_stamp; - do_div(tmp, 1000); - cpu_khz = (unsigned long)tmp; + cpu_khz = tmp / 1000; printk("Detected %lu.%03lu MHz processor.\n", cpu_khz / 1000, cpu_khz % 1000);
When the macro's "return value" is not used, the macro use can be replaced by a simply division, avoiding some obfuscation. According to my observations, no change to generated code. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Arguably the ULL suffix (in write_watchdog_counter()) or the cast to uint64_t (in div_sc()) aren't really needed in code which gets built for 64-bit only.