diff mbox series

[1/3] printk: Allow architecture-specific timestamping function

Message ID 20190722103330.255312-2-marc.zyngier@arm.com
State New
Headers show
Series arm64: Allow early timestamping of kernel log | expand

Commit Message

Marc Zyngier July 22, 2019, 10:33 a.m. UTC 
printk currently relies on local_clock to time-stamp the kernel
messages. In order to allow the timestamping (and only that)
to be overridden by architecture-specific code, let's declare
a new timestamp_clock() function, which gets used by the printk
code. Architectures willing to make use of this facility will
have to define CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

The default is of course to return local_clock(), so that the
existing behaviour stays unchanged.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

---
 include/linux/sched/clock.h | 13 +++++++++++++
 kernel/printk/printk.c      |  4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.20.1

Comments

Petr Mladek July 22, 2019, 11:25 a.m. UTC | #1 
On Mon 2019-07-22 11:33:28, Marc Zyngier wrote:
> printk currently relies on local_clock to time-stamp the kernel

> messages. In order to allow the timestamping (and only that)

> to be overridden by architecture-specific code, let's declare

> a new timestamp_clock() function, which gets used by the printk

> code. Architectures willing to make use of this facility will

> have to define CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

>

> The default is of course to return local_clock(), so that the

> existing behaviour stays unchanged.

> 

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

> ---

>  include/linux/sched/clock.h | 13 +++++++++++++

>  kernel/printk/printk.c      |  4 ++--

>  2 files changed, 15 insertions(+), 2 deletions(-)

> 

> diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h

> index 867d588314e0..3cf4b2a8ce18 100644

> --- a/include/linux/sched/clock.h

> +++ b/include/linux/sched/clock.h

> @@ -98,4 +98,17 @@ static inline void enable_sched_clock_irqtime(void) {}

>  static inline void disable_sched_clock_irqtime(void) {}

>  #endif

>  

> +#ifdef CONFIG_ARCH_HAS_TIMESTAMP_CLOCK

> +/* Special need architectures can provide their timestamping function */


The commit message and the above comment should be more specific
about what are the special needs.

It must be clear how and why the clock differs from the other
clocks, especially from lock_clock().

Also the first mail says that timestamp_clock() might be
unstable. Is this true only during the early boot or all
the time?

The timestamp helps to order the events. An unstable clock
might be better than nothing during the boot. But it would
look strange to use it all the time, especially when it was
unrelated to any other clock used by the system.

Best Regards,
Petr
Marc Zyngier July 22, 2019, 12:47 p.m. UTC | #2 
On 22/07/2019 12:25, Petr Mladek wrote:
> On Mon 2019-07-22 11:33:28, Marc Zyngier wrote:

>> printk currently relies on local_clock to time-stamp the kernel

>> messages. In order to allow the timestamping (and only that)

>> to be overridden by architecture-specific code, let's declare

>> a new timestamp_clock() function, which gets used by the printk

>> code. Architectures willing to make use of this facility will

>> have to define CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

>>

>> The default is of course to return local_clock(), so that the

>> existing behaviour stays unchanged.

>>

>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

>> ---

>>  include/linux/sched/clock.h | 13 +++++++++++++

>>  kernel/printk/printk.c      |  4 ++--

>>  2 files changed, 15 insertions(+), 2 deletions(-)

>>

>> diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h

>> index 867d588314e0..3cf4b2a8ce18 100644

>> --- a/include/linux/sched/clock.h

>> +++ b/include/linux/sched/clock.h

>> @@ -98,4 +98,17 @@ static inline void enable_sched_clock_irqtime(void) {}

>>  static inline void disable_sched_clock_irqtime(void) {}

>>  #endif

>>  

>> +#ifdef CONFIG_ARCH_HAS_TIMESTAMP_CLOCK

>> +/* Special need architectures can provide their timestamping function */

> 

> The commit message and the above comment should be more specific

> about what are the special needs.

> 

> It must be clear how and why the clock differs from the other

> clocks, especially from lock_clock().


Fair enough. How about something along the lines of:

"An architecture can override the timestamp clock (which defaults to
local_clock) if local_clock is not significant early enough (sched_clock
being available too late)."

> Also the first mail says that timestamp_clock() might be

> unstable. Is this true only during the early boot or all

> the time?


With the current proposal, the instability period only exists until
sched_clock gets initialized, at which point it takes over and the
timestamping becomes stable.

Note that this is the arm64 implementation, and not something that is
currently guaranteed by just selecting CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

> 

> The timestamp helps to order the events. An unstable clock

> might be better than nothing during the boot. But it would

> look strange to use it all the time, especially when it was

> unrelated to any other clock used by the system.


And that's exactly what patch #3 implements. Once local_clock() returns
something significant, we use that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
Russell King (Oracle) July 22, 2019, 1:03 p.m. UTC | #3 
On Mon, Jul 22, 2019 at 01:47:57PM +0100, Marc Zyngier wrote:
> On 22/07/2019 12:25, Petr Mladek wrote:

> > On Mon 2019-07-22 11:33:28, Marc Zyngier wrote:

> >> printk currently relies on local_clock to time-stamp the kernel

> >> messages. In order to allow the timestamping (and only that)

> >> to be overridden by architecture-specific code, let's declare

> >> a new timestamp_clock() function, which gets used by the printk

> >> code. Architectures willing to make use of this facility will

> >> have to define CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

> >>

> >> The default is of course to return local_clock(), so that the

> >> existing behaviour stays unchanged.

> >>

> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

> >> ---

> >>  include/linux/sched/clock.h | 13 +++++++++++++

> >>  kernel/printk/printk.c      |  4 ++--

> >>  2 files changed, 15 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h

> >> index 867d588314e0..3cf4b2a8ce18 100644

> >> --- a/include/linux/sched/clock.h

> >> +++ b/include/linux/sched/clock.h

> >> @@ -98,4 +98,17 @@ static inline void enable_sched_clock_irqtime(void) {}

> >>  static inline void disable_sched_clock_irqtime(void) {}

> >>  #endif

> >>  

> >> +#ifdef CONFIG_ARCH_HAS_TIMESTAMP_CLOCK

> >> +/* Special need architectures can provide their timestamping function */

> > 

> > The commit message and the above comment should be more specific

> > about what are the special needs.

> > 

> > It must be clear how and why the clock differs from the other

> > clocks, especially from lock_clock().

> 

> Fair enough. How about something along the lines of:

> 

> "An architecture can override the timestamp clock (which defaults to

> local_clock) if local_clock is not significant early enough (sched_clock

> being available too late)."


We have:
1) the standard clocksource
2) the sched_clock, which is _supposed_ to be initialised early
3) persistent_clock

Do we really need another clock?

Why not initialise sched_clock() early (as in, before sched_init(),
which is where the first sched_clock() read occurs) ?

We've already been around the argument that sched_clock() apparently
can't be initialised early enough (which is the argument I had in reply
to the sched_clock() situation on ARM32) then how does inventing
timestamp_clock() solve this problem?

Wouldn't timestamp_clock() also suffer from the very same "we can't
initialise it early enough" issue, and it'll just be setup along side
clocksources, just like sched_clock() has become?

I fail to see what adding yet another architecture specific clock
implementation buys, apart from yet more complexity.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Marc Zyngier July 22, 2019, 1:26 p.m. UTC | #4 
On 22/07/2019 14:03, Russell King - ARM Linux admin wrote:
> On Mon, Jul 22, 2019 at 01:47:57PM +0100, Marc Zyngier wrote:

>> On 22/07/2019 12:25, Petr Mladek wrote:

>>> On Mon 2019-07-22 11:33:28, Marc Zyngier wrote:

>>>> printk currently relies on local_clock to time-stamp the kernel

>>>> messages. In order to allow the timestamping (and only that)

>>>> to be overridden by architecture-specific code, let's declare

>>>> a new timestamp_clock() function, which gets used by the printk

>>>> code. Architectures willing to make use of this facility will

>>>> have to define CONFIG_ARCH_HAS_TIMESTAMP_CLOCK.

>>>>

>>>> The default is of course to return local_clock(), so that the

>>>> existing behaviour stays unchanged.

>>>>

>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

>>>> ---

>>>>  include/linux/sched/clock.h | 13 +++++++++++++

>>>>  kernel/printk/printk.c      |  4 ++--

>>>>  2 files changed, 15 insertions(+), 2 deletions(-)

>>>>

>>>> diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h

>>>> index 867d588314e0..3cf4b2a8ce18 100644

>>>> --- a/include/linux/sched/clock.h

>>>> +++ b/include/linux/sched/clock.h

>>>> @@ -98,4 +98,17 @@ static inline void enable_sched_clock_irqtime(void) {}

>>>>  static inline void disable_sched_clock_irqtime(void) {}

>>>>  #endif

>>>>  

>>>> +#ifdef CONFIG_ARCH_HAS_TIMESTAMP_CLOCK

>>>> +/* Special need architectures can provide their timestamping function */

>>>

>>> The commit message and the above comment should be more specific

>>> about what are the special needs.

>>>

>>> It must be clear how and why the clock differs from the other

>>> clocks, especially from lock_clock().

>>

>> Fair enough. How about something along the lines of:

>>

>> "An architecture can override the timestamp clock (which defaults to

>> local_clock) if local_clock is not significant early enough (sched_clock

>> being available too late)."

> 

> We have:

> 1) the standard clocksource

> 2) the sched_clock, which is _supposed_ to be initialised early

> 3) persistent_clock

> 

> Do we really need another clock?

> 

> Why not initialise sched_clock() early (as in, before sched_init(),

> which is where the first sched_clock() read occurs) ?


Because, as you hint at below, that's not generally possible if you need
to identify the system early enough to discover that you need to apply
an erratum workaround. If you init sched_clock() before you know what
you're running on, you may end-up with a clock that can jump in either
direction.

And while the first call to sched_clock happens pretty late, the
timestamping code uses it pretty early, via the local_clock() indirection.

> 

> We've already been around the argument that sched_clock() apparently

> can't be initialised early enough (which is the argument I had in reply

> to the sched_clock() situation on ARM32) then how does inventing

> timestamp_clock() solve this problem?


It allows the kernel message to be timestamped with a potentially
unreliable clock without breaking the promise that sched_clock() will
not go backward or otherwise behave erratically.

> Wouldn't timestamp_clock() also suffer from the very same "we can't

> initialise it early enough" issue, and it'll just be setup along side

> clocksources, just like sched_clock() has become?


At least on arm64, the architected counter is always available, and
doesn't require any setup (at least none by the time the kernel is booted).

> I fail to see what adding yet another architecture specific clock

> implementation buys, apart from yet more complexity.

> 


It buys us early timestamping without forcing us to deal with an
unreliable. The additional complexity looks pretty minimal to me, and no
other architecture is forced to use it.

	M.
-- 
Jazz is not dead. It just smells funny...
diff mbox series

Patch

diff --git a/include/linux/sched/clock.h b/include/linux/sched/clock.h
index 867d588314e0..3cf4b2a8ce18 100644
--- a/include/linux/sched/clock.h
+++ b/include/linux/sched/clock.h
@@ -98,4 +98,17 @@  static inline void enable_sched_clock_irqtime(void) {}
 static inline void disable_sched_clock_irqtime(void) {}
 #endif
 
+#ifdef CONFIG_ARCH_HAS_TIMESTAMP_CLOCK
+/* Special need architectures can provide their timestamping function */
+extern u64 timestamp_clock(void);
+
+#else
+
+static inline u64 timestamp_clock(void)
+{
+	return local_clock();
+}
+
+#endif
+
 #endif /* _LINUX_SCHED_CLOCK_H */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1888f6a3b694..166702316714 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -638,7 +638,7 @@  static int log_store(u32 caller_id, int facility, int level,
 	if (ts_nsec > 0)
 		msg->ts_nsec = ts_nsec;
 	else
-		msg->ts_nsec = local_clock();
+		msg->ts_nsec = timestamp_clock();
 #ifdef CONFIG_PRINTK_CALLER
 	msg->caller_id = caller_id;
 #endif
@@ -1841,7 +1841,7 @@  static bool cont_add(u32 caller_id, int facility, int level,
 		cont.facility = facility;
 		cont.level = level;
 		cont.caller_id = caller_id;
-		cont.ts_nsec = local_clock();
+		cont.ts_nsec = timestamp_clock();
 		cont.flags = flags;
 	}