On Friday 14 Feb 2020 at 15:45:25 (+0000), Ionela Voinescu wrote:

Hi Thomas,

On Friday 14 Feb 2020 at 01:35:58 (+0100), Thomas Gleixner wrote:

quoted

Ionela Voinescu [off-list ref] writes:

quoted

From: Valentin Schneider <redacted>

Using an arch timer with a frequency of less than 1MHz can result in an
incorrect functionality of the system which assumes a reasonable rate.

One example is the use of activity monitors for frequency invariance
which uses the rate of the arch timer as the known rate of the constant
cycle counter in computing its ratio compared to the maximum frequency
of a CPU. For arch timer frequencies less than 1MHz this ratio could
end up being 0 which is an invalid value for its use.

Therefore, warn if the arch timer rate is below 1MHz which contravenes
the recommended architecture interval of 1 to 50MHz.

Signed-off-by: Ionela Voinescu <redacted>

So this patch is from Valentin. Where is his Signed-off-by?

Yes, sorry about this. This was based on a diff that Valentin provided
in v2. I'll change the author as agreed at:
https://lore.kernel.org/lkml/20200212103249.GA19041@arm.com/ (local)

quoted

quoted

 
+static int validate_timer_rate(void)
+{
+	if (!arch_timer_rate)
+		return -EINVAL;
+
+	/* Arch timer frequency < 1MHz can cause trouble */
+	WARN_ON(arch_timer_rate < 1000000);

This does not make sense to me. If the rate is out of bounds then why
warn an just continue instead of making it fail?

Because it's not a hard restriction, it's just atypical for the rate to
be below 1Mhz. The spec only mentions a typical range of 1 to 50MHz and
the warning is only here to flag a potentially problematic rate, below
what is assumed typical in the spec.

In [1], where I'm actually relying on arch_timer_rate being higher than
than 1/SCHED_CAPACITY_SCALE² of the maximum frequency, I am making it
fail, as, for that scenario, it is a hard restriction.


+	 * We use a factor of 2 * SCHED_CAPACITY_SHIFT -> SCHED_CAPACITY_SCALE²
+	 * in order to ensure a good resolution for arch_max_freq_scale for
+	 * very low arch timer frequencies (up to the KHz range which should be
+	 * unlikely).
+	 */
+	ratio = (u64)arch_timer_get_rate() << (2 * SCHED_CAPACITY_SHIFT);
+	ratio = div64_u64(ratio, max_freq_hz);
+	if (!ratio) {
+		pr_err("System timer frequency too low.\n");
+		return -EINVAL;
+	}
+

[1] https://lore.kernel.org/lkml/89339501-5ee4-e871-3076-c8b02c6fbf6e@arm.com/ (local)

I've mistakenly referenced a bad link ^

It was supposed to be:

[1] https://lore.kernel.org/lkml/20200211184542.29585-7-ionela.voinescu@arm.com/ (local)

Thanks,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel