[PATCH] x86/apic: Use div64_ul() instead of do_div()

[PATCH] x86/apic: Use div64_ul() instead of do_div()

[Thorsten Blum]

Fixes Coccinelle/coccicheck warnings reported by do_div.cocci.

Change deltapm to unsigned long and replace do_div() with div64_ul()
which doesn't implicitly cast the divisor and doesn't unnecessarily
calculate the remainder.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 arch/x86/kernel/apic/apic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..facfb03ef5c8 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 }
 
 static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(unsigned long deltapm, long *delta, long *deltatsc)
 {
 	const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
 	const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 	return -1;
 #endif
 
-	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %lu\n", deltapm);
 
 	/* Check, if the PM timer is available */
 	if (!deltapm)
@@ -724,14 +724,14 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 		return 0;
 	}
 
-	res = (((u64)deltapm) *  mult) >> 22;
+	res = (((u64)deltapm) * mult) >> 22;
 	do_div(res, 1000000);
 	pr_warn("APIC calibration not consistent "
 		"with PM-Timer: %ldms instead of 100ms\n", (long)res);
 
 	/* Correct the lapic counter value */
 	res = (((u64)(*delta)) * pm_100ms);
-	do_div(res, deltapm);
+	res = div64_ul(res, deltapm);
 	pr_info("APIC delta adjusted to PM-Timer: "
 		"%lu (%ld)\n", (unsigned long)res, *delta);
 	*delta = (long)res;
@@ -739,7 +739,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 	/* Correct the tsc counter value */
 	if (boot_cpu_has(X86_FEATURE_TSC)) {
 		res = (((u64)(*deltatsc)) * pm_100ms);
-		do_div(res, deltapm);
+		res = div64_ul(res, deltapm);
 		apic_printk(APIC_VERBOSE, "TSC delta adjusted to "
 					  "PM-Timer: %lu (%ld)\n",
 					(unsigned long)res, *deltatsc);
-- 
2.43.2

RE: [PATCH] x86/apic: Use div64_ul() instead of do_div()

[David Laight]

From: Thorsten Blum
> Sent: 27 February 2024 11:44
> 
> Fixes Coccinelle/coccicheck warnings reported by do_div.cocci.
> 
> Change deltapm to unsigned long and replace do_div() with div64_ul()
> which doesn't implicitly cast the divisor and doesn't unnecessarily
> calculate the remainder.

Eh? they are entirely different beasts.

do_div() does a 64 by 32 divide that gives a 32bit quotient.
div64_ul() does a much more expensive 64 by 64 divide that
can generate a 64bit quotient.

The remainder is pretty much free in both cases.
If a cpu has a divide instruction it will almost certainly
put the result in one register and the quotient in another.

64 by 64 divides are horribly expensive on 32bit - they typically
have to be done in software even if the cpu has a divide instruction
which will (typically) do a 64 by 32 divide.

Even on Intel 64 bit x86 the 128 by 64 divide (needed to get
the 64 bit quotient) takes twice as long as the 64 by 32 one.
Even when the values are small.

The entire reason that the 'libc' divide function isn't in
the kernel is because it is slow and you really don't want
to be doing it unless the VALUES not the types require it.


	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] x86/apic: Use div64_ul() instead of do_div()

[H. Peter Anvin]

>> 
>> Change deltapm to unsigned long and replace do_div() with div64_ul()
>> which doesn't implicitly cast the divisor and doesn't unnecessarily
>> calculate the remainder.
>
>Eh? they are entirely different beasts.
>
>do_div() does a 64 by 32 divide that gives a 32bit quotient.
>div64_ul() does a much more expensive 64 by 64 divide that
>can generate a 64bit quotient.
>
>The remainder is pretty much free in both cases.
>If a cpu has a divide instruction it will almost certainly
>put the result in one register and the quotient in another.
>

Not on e.g. RISC-V.

RE: [PATCH] x86/apic: Use div64_ul() instead of do_div()

[David Laight]

From: H. Peter Anvin
> Sent: 01 March 2024 01:02
> 
> >>
> >> Change deltapm to unsigned long and replace do_div() with div64_ul()
> >> which doesn't implicitly cast the divisor and doesn't unnecessarily
> >> calculate the remainder.
> >
> >Eh? they are entirely different beasts.
> >
> >do_div() does a 64 by 32 divide that gives a 32bit quotient.
> >div64_ul() does a much more expensive 64 by 64 divide that
> >can generate a 64bit quotient.
> >
> >The remainder is pretty much free in both cases.
> >If a cpu has a divide instruction it will almost certainly
> >put the result in one register and the quotient in another.
> >
> 
> Not on e.g. RISC-V.

If the remainder isn't used the compiler should optimise
away any code used to generate it.

gcc is also generating rather sub-optimal code.
On x86 it only does one divide for code that uses 'a / b' and
'a % b', but for riscv it does separate divide and remainder
instructions.
clang does a multiply and subtract for the remainder.

Compared to any form of divide, the extra multiply is noise.

gcc also pessimises attempts to calculate the remainder:
https://godbolt.org/z/Tojh1qcvs

Are the instruction weights set correctly for divide/remainder?
It is almost as though gcc thinks remainder is fast.

Actually I suspect even the 64 by 32 divide is a software loop
on riscv (32bit).
Not checked but I suspect the implementations (esp fpga ones) won't
allow 3 inputs to the ALU.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/apic: Use div64_ul() instead of do_div()

[Thorsten Blum]

> On Feb 29, 2024, at 23:13, David Laight <David.Laight@ACULAB.COM> wrote:
> 
> do_div() does a 64 by 32 divide that gives a 32bit quotient.
> div64_ul() does a much more expensive 64 by 64 divide that
> can generate a 64bit quotient.

Since the dividend and the divisor could (according to the types at least) both
be 64-bit values and do_div() does a 64-by-32 division, the quotient could
potentially be wrong.

However, if the values don't require a 64-by-64 division (not even on a 64-bit
system) and the divisor (deltapm) is guaranteed to fit into 32 bits, wouldn't it
make sense to change its type from long to int?

Given that acpi_pm_read_early() returns a u32 and is then assigned to unsigned
long pm, this might be the source of the issue. Changing pm and deltapm from
long to u32 and keeping do_div() as is, would improve the types and remove the
Coccinelle warnings...but maybe I'm missing something?

Thanks,
Thorsten

[PATCH v2] x86/apic: Use u32 instead of unsigned long

[Thorsten Blum]

Use u32 to fix Coccinelle/coccicheck warnings reported by do_div.cocci.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
Changes in v2:
- Revert changing do_div() to div64_ul() after feedback from David Laight
- Use u32 instead of unsigned long to fix Coccinelle warnings
- Update patch title and description
---
 arch/x86/kernel/apic/apic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..184e1843620d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -664,7 +664,7 @@ void lapic_update_tsc_freq(void)
 static __initdata int lapic_cal_loops = -1;
 static __initdata long lapic_cal_t1, lapic_cal_t2;
 static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2;
-static __initdata unsigned long lapic_cal_pm1, lapic_cal_pm2;
+static __initdata u32 lapic_cal_pm1, lapic_cal_pm2;
 static __initdata unsigned long lapic_cal_j1, lapic_cal_j2;
 
 /*
@@ -674,7 +674,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 {
 	unsigned long long tsc = 0;
 	long tapic = apic_read(APIC_TMCCT);
-	unsigned long pm = acpi_pm_read_early();
+	u32 pm = acpi_pm_read_early();
 
 	if (boot_cpu_has(X86_FEATURE_TSC))
 		tsc = rdtsc();
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 }
 
 static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(u32 deltapm, long *delta, long *deltatsc)
 {
 	const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
 	const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 	return -1;
 #endif
 
-	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %u\n", deltapm);
 
 	/* Check, if the PM timer is available */
 	if (!deltapm)
-- 
2.44.0

[RESEND PATCH v2] x86/apic: Use u32 instead of unsigned long

[Thorsten Blum]

Improve types by using u32 instead of unsigned long. Fixes two
Coccinelle/coccicheck warnings reported by do_div.cocci.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
Changes in v2:
- Revert changing do_div() to div64_ul() after feedback from David Laight
- Use u32 instead of unsigned long to fix Coccinelle warnings
- Update patch title and description
---
 arch/x86/kernel/apic/apic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..184e1843620d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -664,7 +664,7 @@ void lapic_update_tsc_freq(void)
 static __initdata int lapic_cal_loops = -1;
 static __initdata long lapic_cal_t1, lapic_cal_t2;
 static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2;
-static __initdata unsigned long lapic_cal_pm1, lapic_cal_pm2;
+static __initdata u32 lapic_cal_pm1, lapic_cal_pm2;
 static __initdata unsigned long lapic_cal_j1, lapic_cal_j2;
 
 /*
@@ -674,7 +674,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 {
 	unsigned long long tsc = 0;
 	long tapic = apic_read(APIC_TMCCT);
-	unsigned long pm = acpi_pm_read_early();
+	u32 pm = acpi_pm_read_early();
 
 	if (boot_cpu_has(X86_FEATURE_TSC))
 		tsc = rdtsc();
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 }
 
 static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(u32 deltapm, long *delta, long *deltatsc)
 {
 	const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
 	const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 	return -1;
 #endif
 
-	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %u\n", deltapm);
 
 	/* Check, if the PM timer is available */
 	if (!deltapm)
-- 
2.44.0

Re: [RESEND PATCH v2] x86/apic: Use u32 instead of unsigned long

[Dave Hansen]

On 3/8/24 04:43, Thorsten Blum wrote:
> Improve types by using u32 instead of unsigned long. Fixes two
> Coccinelle/coccicheck warnings reported by do_div.cocci.

This seems simple enough, but the changelog and subject are really
lacking in substantive information.

The _patch_ literally does a few s/unsigned long/u32/ operations, and
that's just repeated pretty much verbatim in the changelog and subject.
So it just tells me two more times what I already know.

Without going and running do_div.cocci I can't tell whether the warnings
are good checks or nonsense.  I also don't know _which_ do_div()s the
warnings even refer to.

Could we get a _little_ more meat in here, please?

Re: [RESEND PATCH v2] x86/apic: Use u32 instead of unsigned long

[Thorsten Blum]

On Mar 8, 2024, at 17:12, Dave Hansen <dave.hansen@intel.com> wrote:
> 
> On 3/8/24 04:43, Thorsten Blum wrote:
>> Improve types by using u32 instead of unsigned long. Fixes two
>> Coccinelle/coccicheck warnings reported by do_div.cocci.
> 
> This seems simple enough, but the changelog and subject are really
> lacking in substantive information.
> 
> The _patch_ literally does a few s/unsigned long/u32/ operations, and
> that's just repeated pretty much verbatim in the changelog and subject.
> So it just tells me two more times what I already know.
> 
> Without going and running do_div.cocci I can't tell whether the warnings
> are good checks or nonsense.  I also don't know _which_ do_div()s the
> warnings even refer to.
> 
> Could we get a _little_ more meat in here, please?

Yes, of course.

Coccinelle issues the following two warnings:

arch/x86/kernel/apic/apic.c:734:1-7: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

arch/x86/kernel/apic/apic.c:742:2-8: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

These occur because the divisor (deltapm) is of type long and the do_div() macro
casts the divisor to uint32_t, which could potentially lead to an incorrect
quotient.

Version 1 of my patch replaced both do_div() macro calls with div64_ul()
function calls, thereby removing the Coccinelle warnings. I used div64_ul()
instead of div64_long() because I also changed deltapm from long to unsigned
long, given that deltapm cannot be negative.

However, David Laight noted that div64_ul() is a 64-by-64 division and 
significantly slower (especially on 32-bit machines).

David's feedback led me to trace the source of deltapm, which is
acpi_pm_read_early() in include/linux/acpi_pmtmr.h. Since acpi_pm_read_early()
returns a u32 (which is additionally masked to 24 bits), there is no reason for
deltapm or any of the other intermediate variables (pm, lapic_cal_pm1, and
lapic_cal_pm2) to be of type long or unsigned long. They can all be u32, which
more accurately reflects their possible values and which removes both
Coccinelle warnings.

Maybe I'm missing something, but hopefully this clarifies my changes in v2.

Thanks,
Thorsten

[PATCH v3] x86/apic: Improve data types to fix Coccinelle warnings

[Thorsten Blum]

Given that acpi_pm_read_early() returns a u32 (masked to 24 bits), several
variables that store its return value are improved by adjusting their data
types from unsigned long to u32. Specifically, change deltapm's type from
long to u32 because its value fits into 32 bits and it cannot be negative.

These data type improvements resolve the following two Coccinelle/
coccicheck warnings reported by do_div.cocci:

arch/x86/kernel/apic/apic.c:734:1-7: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

arch/x86/kernel/apic/apic.c:742:2-8: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
Changes in v2:
- Revert changing do_div() to div64_ul() after feedback from David Laight
- Use u32 instead of unsigned long to fix Coccinelle warnings
- Update patch title and description

Changes in v3:
- Adjust patch summary and description after feedback from Dave Hansen
---
 arch/x86/kernel/apic/apic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..184e1843620d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -664,7 +664,7 @@ void lapic_update_tsc_freq(void)
 static __initdata int lapic_cal_loops = -1;
 static __initdata long lapic_cal_t1, lapic_cal_t2;
 static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2;
-static __initdata unsigned long lapic_cal_pm1, lapic_cal_pm2;
+static __initdata u32 lapic_cal_pm1, lapic_cal_pm2;
 static __initdata unsigned long lapic_cal_j1, lapic_cal_j2;
 
 /*
@@ -674,7 +674,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 {
 	unsigned long long tsc = 0;
 	long tapic = apic_read(APIC_TMCCT);
-	unsigned long pm = acpi_pm_read_early();
+	u32 pm = acpi_pm_read_early();
 
 	if (boot_cpu_has(X86_FEATURE_TSC))
 		tsc = rdtsc();
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 }
 
 static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(u32 deltapm, long *delta, long *deltatsc)
 {
 	const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
 	const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 	return -1;
 #endif
 
-	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %u\n", deltapm);
 
 	/* Check, if the PM timer is available */
 	if (!deltapm)
-- 
2.44.0

[RESEND PATCH v3] x86/apic: Improve data types to fix Coccinelle warnings

[Thorsten Blum]

Given that acpi_pm_read_early() returns a u32 (masked to 24 bits), several
variables that store its return value are improved by adjusting their data
types from unsigned long to u32. Specifically, change deltapm's type from
long to u32 because its value fits into 32 bits and it cannot be negative.

These data type improvements resolve the following two Coccinelle/
coccicheck warnings reported by do_div.cocci:

arch/x86/kernel/apic/apic.c:734:1-7: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

arch/x86/kernel/apic/apic.c:742:2-8: WARNING: do_div() does a 64-by-32
division, please consider using div64_long instead.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
Changes in v2:
- Revert changing do_div() to div64_ul() after feedback from David Laight
- Use u32 instead of unsigned long to fix Coccinelle warnings
- Update patch title and description

Changes in v3:
- Adjust patch summary and description after feedback from Dave Hansen
---
 arch/x86/kernel/apic/apic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4667bc4b00ab..184e1843620d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -664,7 +664,7 @@ void lapic_update_tsc_freq(void)
 static __initdata int lapic_cal_loops = -1;
 static __initdata long lapic_cal_t1, lapic_cal_t2;
 static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2;
-static __initdata unsigned long lapic_cal_pm1, lapic_cal_pm2;
+static __initdata u32 lapic_cal_pm1, lapic_cal_pm2;
 static __initdata unsigned long lapic_cal_j1, lapic_cal_j2;
 
 /*
@@ -674,7 +674,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 {
 	unsigned long long tsc = 0;
 	long tapic = apic_read(APIC_TMCCT);
-	unsigned long pm = acpi_pm_read_early();
+	u32 pm = acpi_pm_read_early();
 
 	if (boot_cpu_has(X86_FEATURE_TSC))
 		tsc = rdtsc();
@@ -699,7 +699,7 @@ static void __init lapic_cal_handler(struct clock_event_device *dev)
 }
 
 static int __init
-calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
+calibrate_by_pmtimer(u32 deltapm, long *delta, long *deltatsc)
 {
 	const long pm_100ms = PMTMR_TICKS_PER_SEC / 10;
 	const long pm_thresh = pm_100ms / 100;
@@ -710,7 +710,7 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 	return -1;
 #endif
 
-	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %ld\n", deltapm);
+	apic_printk(APIC_VERBOSE, "... PM-Timer delta = %u\n", deltapm);
 
 	/* Check, if the PM timer is available */
 	if (!deltapm)
-- 
2.44.0

Re: [RESEND PATCH v3] x86/apic: Improve data types to fix Coccinelle warnings

[Dave Hansen]

On 3/18/24 03:47, Thorsten Blum wrote:
> Given that acpi_pm_read_early() returns a u32 (masked to 24 bits), several
> variables that store its return value are improved by adjusting their data
> types from unsigned long to u32. Specifically, change deltapm's type from
> long to u32 because its value fits into 32 bits and it cannot be negative.

Looks, good, thanks for the changes:

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>

I'll put this in the queue.