[PATCH v1 0/1] Early boot time stamps for arm64

[PATCH v1 0/1] Early boot time stamps for arm64

[Pavel Tatashin]

I made early boot time stamps available for SPARC and X86.

x86:
https://lore.kernel.org/lkml/20180719205545.16512-1-pasha.tatashin@oracle.com

sparc:
https://www.spinics.net/lists/sparclinux/msg18063.html

As discussed at plumbers, I would like to add the same for arm64. The
implementation does not have to be perfect, and should work only when early
clock is easy to determine. arm64 defines a clock register, and thus makes
it easy, but on some platforms frequency register is broken, so if it is
not known, simply don't initialize clock early.

dmesg before:
https://paste.ubuntu.com/p/3pJ5kgJHyN

dmesg after:
https://paste.ubuntu.com/p/RHKGVd9nSM

As seen from the above with base smp_init is finished after 0.47s:
[    0.464585] SMP: Total of 8 processors activated.

But, in reality, 3.2s is missing which is a quiet long considering this is
only 60G domain.

Pavel Tatashin (1):
  arm64: early boot clock for aarch64

 arch/arm64/kernel/setup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.19.1

[PATCH v1 1/1] arm64: Early boot time stamps

[Pavel Tatashin]

Allow printk time stamps/sched_clock() to be available from the early
boot.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 arch/arm64/kernel/setup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index f4fc1e0544b7..4df41a66b403 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -40,6 +40,7 @@
 #include <linux/efi.h>
 #include <linux/psci.h>
 #include <linux/sched/task.h>
+#include <linux/sched_clock.h>
 #include <linux/mm.h>
 
 #include <asm/acpi.h>
@@ -279,8 +280,26 @@ arch_initcall(reserve_memblock_reserved_regions);
 
 u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
 
+/*
+ * Get time stamps available early in boot, useful to identify boot time issues
+ * from the early boot.
+ */
+static __init void sched_clock_early_init(void)
+{
+	u64 freq = arch_timer_get_cntfrq();
+	u64 (*read_time)(void) = arch_counter_get_cntvct;
+
+	/* Early clock is available only on platforms with known freqs */
+	if (!freq)
+		return;
+
+	sched_clock_register(read_time, BITS_PER_LONG, freq);
+}
+
 void __init setup_arch(char **cmdline_p)
 {
+	sched_clock_early_init();
+
 	init_mm.start_code = (unsigned long) _text;
 	init_mm.end_code   = (unsigned long) _etext;
 	init_mm.end_data   = (unsigned long) _edata;
-- 
2.19.1

Re: [PATCH v1 1/1] arm64: Early boot time stamps

[Marc Zyngier]

Hi Pavel,

On 19/11/2018 21:44, Pavel Tatashin wrote:
> Allow printk time stamps/sched_clock() to be available from the early
> boot.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>  arch/arm64/kernel/setup.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index f4fc1e0544b7..4df41a66b403 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -40,6 +40,7 @@
>  #include <linux/efi.h>
>  #include <linux/psci.h>
>  #include <linux/sched/task.h>
> +#include <linux/sched_clock.h>
>  #include <linux/mm.h>
>  
>  #include <asm/acpi.h>
> @@ -279,8 +280,26 @@ arch_initcall(reserve_memblock_reserved_regions);
>  
>  u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
>  
> +/*
> + * Get time stamps available early in boot, useful to identify boot time issues
> + * from the early boot.
> + */
> +static __init void sched_clock_early_init(void)
> +{
> +	u64 freq = arch_timer_get_cntfrq();
> +	u64 (*read_time)(void) = arch_counter_get_cntvct;

We already have arch_timer_read_counter which is exposed from
arm_arch_timer.h.

> +
> +	/* Early clock is available only on platforms with known freqs */

This comment is misleading. It should read something like:

	/*
	 * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects
	 * the timer frequency. To avoid breakage on misconfigured
	 * systems, do not register the early sched_clock if the
	 * programmed value if zero. Other random values will just
	 * result in random output.
	 */

> +	if (!freq)
> +		return;
> +
> +	sched_clock_register(read_time, BITS_PER_LONG, freq);

This doesn't seem right. The counter has an architected minimum of 56
bits, and you can't assume that it is going to be more than that.

> +}
> +
>  void __init setup_arch(char **cmdline_p)
>  {
> +	sched_clock_early_init();
> +
>  	init_mm.start_code = (unsigned long) _text;
>  	init_mm.end_code   = (unsigned long) _etext;
>  	init_mm.end_data   = (unsigned long) _edata;
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v1 1/1] arm64: Early boot time stamps

[Pavel Tatashin]

> > +static __init void sched_clock_early_init(void)
> > +{
> > +     u64 freq = arch_timer_get_cntfrq();
> > +     u64 (*read_time)(void) = arch_counter_get_cntvct;
>
> We already have arch_timer_read_counter which is exposed from
> arm_arch_timer.h.

OK

>
> > +
> > +     /* Early clock is available only on platforms with known freqs */
>
> This comment is misleading. It should read something like:
>
>         /*
>          * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects
>          * the timer frequency. To avoid breakage on misconfigured
>          * systems, do not register the early sched_clock if the
>          * programmed value if zero. Other random values will just
>          * result in random output.
>          */
>

OK

> > +     if (!freq)
> > +             return;
> > +
> > +     sched_clock_register(read_time, BITS_PER_LONG, freq);
>
> This doesn't seem right. The counter has an architected minimum of 56
> bits, and you can't assume that it is going to be more than that.

Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where
this minimum is defined in aarch64 specs. I will change it to 56.

I will send v2 soon.

Thank you,
Pasha

Re: [PATCH v1 1/1] arm64: Early boot time stamps

[Andrew Murray]

On Tue, Nov 20, 2018 at 09:40:10AM -0500, Pavel Tatashin wrote:
> > > +static __init void sched_clock_early_init(void)
> > > +{
> > > +     u64 freq = arch_timer_get_cntfrq();
> > > +     u64 (*read_time)(void) = arch_counter_get_cntvct;
> >
> > We already have arch_timer_read_counter which is exposed from
> > arm_arch_timer.h.
> 
> OK
> 
> >
> > > +
> > > +     /* Early clock is available only on platforms with known freqs */
> >
> > This comment is misleading. It should read something like:
> >
> >         /*
> >          * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects
> >          * the timer frequency. To avoid breakage on misconfigured
> >          * systems, do not register the early sched_clock if the
> >          * programmed value if zero. Other random values will just
> >          * result in random output.
> >          */
> >
> 
> OK
> 
> > > +     if (!freq)
> > > +             return;
> > > +
> > > +     sched_clock_register(read_time, BITS_PER_LONG, freq);
> >
> > This doesn't seem right. The counter has an architected minimum of 56
> > bits, and you can't assume that it is going to be more than that.
> 
> Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where
> this minimum is defined in aarch64 specs. I will change it to 56.

See section G5.1.2 of the ARM ARM for details.

Thanks,

Andrew Murray

> 
> I will send v2 soon.
> 
> Thank you,
> Pasha

Re: [PATCH v1 1/1] arm64: Early boot time stamps

[Marc Zyngier]

On 20/11/2018 14:40, Pavel Tatashin wrote:
>>> +static __init void sched_clock_early_init(void)
>>> +{
>>> +     u64 freq = arch_timer_get_cntfrq();
>>> +     u64 (*read_time)(void) = arch_counter_get_cntvct;
>>
>> We already have arch_timer_read_counter which is exposed from
>> arm_arch_timer.h.
> 
> OK
> 
>>
>>> +
>>> +     /* Early clock is available only on platforms with known freqs */
>>
>> This comment is misleading. It should read something like:
>>
>>         /*
>>          * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects
>>          * the timer frequency. To avoid breakage on misconfigured
>>          * systems, do not register the early sched_clock if the
>>          * programmed value if zero. Other random values will just
>>          * result in random output.
>>          */
>>
> 
> OK
> 
>>> +     if (!freq)
>>> +             return;
>>> +
>>> +     sched_clock_register(read_time, BITS_PER_LONG, freq);
>>
>> This doesn't seem right. The counter has an architected minimum of 56
>> bits, and you can't assume that it is going to be more than that.
> 
> Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where
> this minimum is defined in aarch64 specs. I will change it to 56.

See D10.1.2 (The system counter) in the ARM ARM[1], which says:

<quote>
The Generic Timer provides a system counter with the following
specification:
Width	At least 56 bits wide.
	The value returned by any 64-bit read of the counter is
	zero-extended to 64 bits.
</quote>

This is not AArch64-specific though, and 32bit systems implementing the
generic timers have the exact same requirements.

> I will send v2 soon.

Please wait a bit and give others a chance to review this too.

Thanks,

	M.

[1] https://static.docs.arm.com/ddi0487/da/DDI0487D_a_armv8_arm.pdf
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v1 1/1] arm64: Early boot time stamps

[Pavel Tatashin]

On 18-11-20 14:55:23, Marc Zyngier wrote:
> > Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where
> > this minimum is defined in aarch64 specs. I will change it to 56.
> 
> See D10.1.2 (The system counter) in the ARM ARM[1], which says:
> 
> <quote>
> The Generic Timer provides a system counter with the following
> specification:
> Width	At least 56 bits wide.
> 	The value returned by any 64-bit read of the counter is
> 	zero-extended to 64 bits.
> </quote>
> 
> This is not AArch64-specific though, and 32bit systems implementing the
> generic timers have the exact same requirements.

Thank you, makes sense. I was thinking there could be AArch64 specific
minimum limit.

> 
> > I will send v2 soon.
> 
> Please wait a bit and give others a chance to review this too.

I already sent, but will wait in the future before sending v3.

> 
> Thanks,
> 
> 	M.
> 
> [1] https://static.docs.arm.com/ddi0487/da/DDI0487D_a_armv8_arm.pdf
> -- 
> Jazz is not dead. It just smells funny...