[PATCH 0/3] sched_clock: Fixes for frequency reporting

[PATCH 0/3] sched_clock: Fixes for frequency reporting

[Maciej W. Rozycki]

Hi,

 Hmm, kernel/time/sched_clock.c lacks a MAINTAINERS entry, but I guess it 
goes along with either high-resolution timers or clocksource core.  Please 
advise otherwise.

 In the course of reviewing the MIPS part of the `random_get_entropy' 
patch set recently posted I have noticed the odd rounding mode used for 
clock source frequency reporting, and then upon inspecting code involved 
I've come across a couple of further small issues.

 This patch set addresses these issues.  Please see individual change 
descriptions for details.

  Maciej

[PATCH 1/3] sched_clock: Round the frequency reported to nearest rather than down

[Maciej W. Rozycki]

We currently round the frequency reported for clock sources down, which 
gives misleading figures, e.g.:

I/O ASIC clock frequency 24999480Hz
clocksource: dec-ioasic: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 76452008078 ns
sched_clock: 32 bits at 24MHz, resolution 40ns, wraps every 85901132779ns
MIPS counter frequency 59998512Hz
clocksource: MIPS: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 31855130776 ns
sched_clock: 32 bits at 59MHz, resolution 16ns, wraps every 35792281591ns

Rounding to nearest seems more adequate:

I/O ASIC clock frequency 24999664Hz
clocksource: dec-ioasic: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 76451445358 ns
sched_clock: 32 bits at 25MHz, resolution 40ns, wraps every 85900499947ns
MIPS counter frequency 59999728Hz
clocksource: MIPS: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 31854485176 ns
sched_clock: 32 bits at 60MHz, resolution 16ns, wraps every 35791556599ns

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 112f38a4a316 ("ARM: sched_clock: provide common infrastructure for sched_clock()")
---
 kernel/time/sched_clock.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

linux-sched-clock-rate-round.diff
Index: linux-macro/kernel/time/sched_clock.c
===================================================================
--- linux-macro.orig/kernel/time/sched_clock.c
+++ linux-macro/kernel/time/sched_clock.c
@@ -8,6 +8,7 @@
 #include <linux/jiffies.h>
 #include <linux/ktime.h>
 #include <linux/kernel.h>
+#include <linux/math.h>
 #include <linux/moduleparam.h>
 #include <linux/sched.h>
 #include <linux/sched/clock.h>
@@ -199,11 +200,11 @@ sched_clock_register(u64 (*read)(void),
 
 	r = rate;
 	if (r >= 4000000) {
-		r /= 1000000;
+		r = DIV_ROUND_CLOSEST(r, 1000000);
 		r_unit = 'M';
 	} else {
 		if (r >= 1000) {
-			r /= 1000;
+			r = DIV_ROUND_CLOSEST(r, 1000);
 			r_unit = 'k';
 		} else {
 			r_unit = ' ';

Re: [PATCH 1/3] sched_clock: Round the frequency reported to nearest rather than down

[John Stultz]

On Sun, Apr 24, 2022 at 4:47 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> We currently round the frequency reported for clock sources down, which
> gives misleading figures, e.g.:
>
> I/O ASIC clock frequency 24999480Hz
> clocksource: dec-ioasic: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 76452008078 ns
> sched_clock: 32 bits at 24MHz, resolution 40ns, wraps every 85901132779ns
> MIPS counter frequency 59998512Hz
> clocksource: MIPS: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 31855130776 ns
> sched_clock: 32 bits at 59MHz, resolution 16ns, wraps every 35792281591ns
>
> Rounding to nearest seems more adequate:
>
> I/O ASIC clock frequency 24999664Hz
> clocksource: dec-ioasic: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 76451445358 ns
> sched_clock: 32 bits at 25MHz, resolution 40ns, wraps every 85900499947ns
> MIPS counter frequency 59999728Hz
> clocksource: MIPS: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 31854485176 ns
> sched_clock: 32 bits at 60MHz, resolution 16ns, wraps every 35791556599ns
>
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Fixes: 112f38a4a316 ("ARM: sched_clock: provide common infrastructure for sched_clock()")
> ---

This seems sane to me.

Acked-by: John Stultz <jstultz@google.com>

thanks
-john

[tip: timers/core] time/sched_clock: Round the frequency reported to nearest rather than down

[tip-bot2 for Maciej W. Rozycki]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     92067440f1311dfa4d77b57a9da6b3706f5da32e
Gitweb:        https://git.kernel.org/tip/92067440f1311dfa4d77b57a9da6b3706f5da32e
Author:        Maciej W. Rozycki <macro@orcam.me.uk>
AuthorDate:    Sun, 24 Apr 2022 12:47:20 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 02 May 2022 14:29:04 +02:00

time/sched_clock: Round the frequency reported to nearest rather than down

The frequency reported for clock sources are rounded down, which gives
misleading figures, e.g.:

 I/O ASIC clock frequency 24999480Hz
 sched_clock: 32 bits at 24MHz, resolution 40ns, wraps every 85901132779ns
 MIPS counter frequency 59998512Hz
 sched_clock: 32 bits at 59MHz, resolution 16ns, wraps every 35792281591ns

Rounding to nearest is more adequate:

 I/O ASIC clock frequency 24999664Hz
 sched_clock: 32 bits at 25MHz, resolution 40ns, wraps every 85900499947ns
 MIPS counter frequency 59999728Hz
 sched_clock: 32 bits at 60MHz, resolution 16ns, wraps every 35791556599ns

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: John Stultz <jstultz@google.com>
Link: https://lore.kernel.org/r/alpine.DEB.2.21.2204240055590.9383@angie.orcam.me.uk

---
 kernel/time/sched_clock.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index b1b9b12..ee07f3a 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -8,6 +8,7 @@
 #include <linux/jiffies.h>
 #include <linux/ktime.h>
 #include <linux/kernel.h>
+#include <linux/math.h>
 #include <linux/moduleparam.h>
 #include <linux/sched.h>
 #include <linux/sched/clock.h>
@@ -199,11 +200,11 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 
 	r = rate;
 	if (r >= 4000000) {
-		r /= 1000000;
+		r = DIV_ROUND_CLOSEST(r, 1000000);
 		r_unit = 'M';
 	} else {
 		if (r >= 1000) {
-			r /= 1000;
+			r = DIV_ROUND_CLOSEST(r, 1000);
 			r_unit = 'k';
 		} else {
 			r_unit = ' ';

[PATCH 2/3] sched_clock: Use Hz as the unit for clock rate reporting below 4kHz

[Maciej W. Rozycki]

We have always used kHz as the unit for clock rates reported between 
1MHz (inclusive) and 4MHz (exclusive), e.g.:

riscv_timer_init_dt: Registering clocksource cpuid [0] hartid [1]
clocksource: riscv_clocksource: mask: 0xffffffffffffffff max_cycles: 0x1d854df40, max_idle_ns: 3526361616960 ns
sched_clock: 64 bits at 1000kHz, resolution 1000ns, wraps every 2199023255500ns

This reduces the amount of data lost due to rounding, but hasn't been 
replicated for the kHz range when support was added for proper reporting 
of sub-kHz clock rates.  Take the same approach then for rates between 
1kHz (inclusive) and 4kHz (exclusive), also for consistency.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 2f0778afac79 ("ARM: 7205/2: sched_clock: allow sched_clock to be selected at runtime")
---
Russell,

 Please correct me if I am wrong with my guess for the origin of the 4MHz 
boundary as there's no mention of it with your commit 112f38a4a316 ("ARM: 
sched_clock: provide common infrastructure for sched_clock()"), which is 
where this code has come from.

  Maciej
---
 kernel/time/sched_clock.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

linux-sched-clock-rate-4khz.diff
Index: linux-macro/kernel/time/sched_clock.c
===================================================================
--- linux-macro.orig/kernel/time/sched_clock.c
+++ linux-macro/kernel/time/sched_clock.c
@@ -203,7 +203,7 @@ sched_clock_register(u64 (*read)(void),
 		r = DIV_ROUND_CLOSEST(r, 1000000);
 		r_unit = 'M';
 	} else {
-		if (r >= 1000) {
+		if (r >= 4000) {
 			r = DIV_ROUND_CLOSEST(r, 1000);
 			r_unit = 'k';
 		} else {

[tip: timers/core] time/sched_clock: Use Hz as the unit for clock rate reporting below 4kHz

[tip-bot2 for Maciej W. Rozycki]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     cc1b923a4e378c943386e7fe6205918d43e5fede
Gitweb:        https://git.kernel.org/tip/cc1b923a4e378c943386e7fe6205918d43e5fede
Author:        Maciej W. Rozycki <macro@orcam.me.uk>
AuthorDate:    Sun, 24 Apr 2022 12:47:26 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 02 May 2022 14:29:04 +02:00

time/sched_clock: Use Hz as the unit for clock rate reporting below 4kHz

The kernel uses kHz as the unit for clock rates reported between 1MHz
(inclusive) and 4MHz (exclusive), e.g.:

 sched_clock: 64 bits at 1000kHz, resolution 1000ns, wraps every 2199023255500ns

This reduces the amount of data lost due to rounding, but hasn't been
replicated for the kHz range when support was added for proper reporting of
sub-kHz clock rates.  Take the same approach for rates between 1kHz
(inclusive) and 4kHz (exclusive), which makes it consistent.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/alpine.DEB.2.21.2204240106380.9383@angie.orcam.me.uk

---
 kernel/time/sched_clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index ee07f3a..4a95c0b 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -203,7 +203,7 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 		r = DIV_ROUND_CLOSEST(r, 1000000);
 		r_unit = 'M';
 	} else {
-		if (r >= 1000) {
+		if (r >= 4000) {
 			r = DIV_ROUND_CLOSEST(r, 1000);
 			r_unit = 'k';
 		} else {

[PATCH 3/3] sched_clock: Fix formatting of frequency reporting code

[Maciej W. Rozycki]

Use flat rather than nested indentation for chained else/if clauses as 
per coding-style.rst:

	if (x == y) {
		..
	} else if (x > y) {
		...
	} else {
		....
	}

This also improves readability.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 32fea568aec5b ("timers, sched/clock: Clean up the code a bit")
---
Hi,

 I guess this got broken with 32fea568aec5b by mistake.

  Maciej
---
 kernel/time/sched_clock.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

linux-sched-clock-rate-cond.diff
Index: linux-macro/kernel/time/sched_clock.c
===================================================================
--- linux-macro.orig/kernel/time/sched_clock.c
+++ linux-macro/kernel/time/sched_clock.c
@@ -202,13 +202,11 @@ sched_clock_register(u64 (*read)(void),
 	if (r >= 4000000) {
 		r = DIV_ROUND_CLOSEST(r, 1000000);
 		r_unit = 'M';
+	} else if (r >= 4000) {
+		r = DIV_ROUND_CLOSEST(r, 1000);
+		r_unit = 'k';
 	} else {
-		if (r >= 4000) {
-			r = DIV_ROUND_CLOSEST(r, 1000);
-			r_unit = 'k';
-		} else {
-			r_unit = ' ';
-		}
+		r_unit = ' ';
 	}
 
 	/* Calculate the ns resolution of this counter */

Re: [PATCH 3/3] sched_clock: Fix formatting of frequency reporting code

[John Stultz]

On Sun, Apr 24, 2022 at 4:47 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> Use flat rather than nested indentation for chained else/if clauses as
> per coding-style.rst:
>
>         if (x == y) {
>                 ..
>         } else if (x > y) {
>                 ...
>         } else {
>                 ....
>         }
>
> This also improves readability.
>
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Fixes: 32fea568aec5b ("timers, sched/clock: Clean up the code a bit")

This patch seems fine to me. Though as Ingo was the one to introduce
the change, his style preference may override in this case.

Acked-by: John Stultz <jstultz@google.com>

thanks
-john

[tip: timers/core] time/sched_clock: Fix formatting of frequency reporting code

[tip-bot2 for Maciej W. Rozycki]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     f4b62e1e1137507268c2c63dc4e6da279dc58e9f
Gitweb:        https://git.kernel.org/tip/f4b62e1e1137507268c2c63dc4e6da279dc58e9f
Author:        Maciej W. Rozycki <macro@orcam.me.uk>
AuthorDate:    Sun, 24 Apr 2022 12:47:30 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 02 May 2022 14:29:04 +02:00

time/sched_clock: Fix formatting of frequency reporting code

Use flat rather than nested indentation for chained else/if clauses as 
per coding-style.rst:

	if (x == y) {
		..
	} else if (x > y) {
		...
	} else {
		....
	}

This also improves readability.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: John Stultz <jstultz@google.com>
Link: https://lore.kernel.org/r/alpine.DEB.2.21.2204240148220.9383@angie.orcam.me.uk

---
 kernel/time/sched_clock.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 4a95c0b..8464c5a 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -202,13 +202,11 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 	if (r >= 4000000) {
 		r = DIV_ROUND_CLOSEST(r, 1000000);
 		r_unit = 'M';
+	} else if (r >= 4000) {
+		r = DIV_ROUND_CLOSEST(r, 1000);
+		r_unit = 'k';
 	} else {
-		if (r >= 4000) {
-			r = DIV_ROUND_CLOSEST(r, 1000);
-			r_unit = 'k';
-		} else {
-			r_unit = ' ';
-		}
+		r_unit = ' ';
 	}
 
 	/* Calculate the ns resolution of this counter */