Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1705718 > unrolled thread
| Started by | John Stultz <john.stultz@linaro.org> |
|---|---|
| First post | 2017-08-07 19:00 +0200 |
| Last post | 2017-08-09 19:30 +0200 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps John Stultz <john.stultz@linaro.org> - 2017-08-07 19:00 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps Peter Zijlstra <peterz@infradead.org> - 2017-08-07 19:20 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps Prarit Bhargava <prarit@redhat.com> - 2017-08-07 20:10 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps John Stultz <john.stultz@linaro.org> - 2017-08-07 20:50 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps Prarit Bhargava <prarit@redhat.com> - 2017-08-07 22:10 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-08-07 22:40 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps Peter Zijlstra <peterz@infradead.org> - 2017-08-08 10:30 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps Prarit Bhargava <prarit@redhat.com> - 2017-08-09 01:10 +0200
Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-08-09 19:30 +0200
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2017-08-07 19:00 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <ubTxM-2Qn-21@gated-at.bofh.it> |
On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
> timestamp to printk messages. The local hardware clock loses time each
> day making it difficult to determine exactly when an issue has occurred in
> the kernel log, and making it difficult to determine how kernel and
> hardware issues relate to each other in real time.
>
> Make printk output different timestampes by adding options for no
> timestamp, the local hardware clock, the monotonic clock, the boottime
> clock, and the real clock. Allow a user to pick one of the clocks by
> using the printk.time kernel parameter. Output the type of clock in
> /sys/module/printk/parameters/time so userspace programs can interpret the
> timestamp.
>
> Real clock & 32-bit systems: Selecting the real clock printk timestamp may
> lead to unlikely situations where a timestamp is wrong because the real time
> offset is read without the protection of a sequence lock in the call to
> ktime_get_log_ts() in printk_get_ts().
>
> v2: Use peterz's suggested Kconfig options. Merge patchset together.
> Fix i386 !CONFIG_PRINTK builds.
>
> v3: Fixed x86_64_defconfig. Added printk_time_type enum and
> printk_time_str for better output. Added BOOTTIME clock functionality.
>
> v4: Fix messages, add additional printk.time options, and fix configs.
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Christoffer Dall <cdall@linaro.org>
> Cc: Deepa Dinamani <deepa.kernel@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Joel Fernandes <joelaf@google.com>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: linux-doc@vger.kernel.org
> ---
> Documentation/admin-guide/kernel-parameters.txt | 6 +-
> arch/arm/configs/aspeed_g4_defconfig | 2 +-
> arch/arm/configs/aspeed_g5_defconfig | 2 +-
> arch/arm/configs/axm55xx_defconfig | 2 +-
> arch/arm/configs/bcm2835_defconfig | 2 +-
> arch/arm/configs/colibri_pxa270_defconfig | 2 +-
> arch/arm/configs/colibri_pxa300_defconfig | 2 +-
> arch/arm/configs/dove_defconfig | 2 +-
> arch/arm/configs/efm32_defconfig | 2 +-
> arch/arm/configs/exynos_defconfig | 2 +-
> arch/arm/configs/ezx_defconfig | 2 +-
> arch/arm/configs/h5000_defconfig | 2 +-
> arch/arm/configs/hisi_defconfig | 2 +-
> arch/arm/configs/imote2_defconfig | 2 +-
> arch/arm/configs/imx_v6_v7_defconfig | 2 +-
> arch/arm/configs/keystone_defconfig | 2 +-
> arch/arm/configs/lpc18xx_defconfig | 2 +-
> arch/arm/configs/magician_defconfig | 2 +-
> arch/arm/configs/mmp2_defconfig | 2 +-
> arch/arm/configs/moxart_defconfig | 2 +-
> arch/arm/configs/mps2_defconfig | 2 +-
> arch/arm/configs/multi_v7_defconfig | 2 +-
> arch/arm/configs/mvebu_v7_defconfig | 2 +-
> arch/arm/configs/mxs_defconfig | 2 +-
> arch/arm/configs/omap2plus_defconfig | 2 +-
> arch/arm/configs/pxa168_defconfig | 2 +-
> arch/arm/configs/pxa3xx_defconfig | 2 +-
> arch/arm/configs/pxa910_defconfig | 2 +-
> arch/arm/configs/pxa_defconfig | 2 +-
> arch/arm/configs/qcom_defconfig | 2 +-
> arch/arm/configs/raumfeld_defconfig | 2 +-
> arch/arm/configs/shmobile_defconfig | 2 +-
> arch/arm/configs/socfpga_defconfig | 2 +-
> arch/arm/configs/stm32_defconfig | 2 +-
> arch/arm/configs/sunxi_defconfig | 2 +-
> arch/arm/configs/tango4_defconfig | 2 +-
> arch/arm/configs/tegra_defconfig | 2 +-
> arch/arm/configs/u300_defconfig | 2 +-
> arch/arm/configs/u8500_defconfig | 2 +-
> arch/arm/configs/vt8500_v6_v7_defconfig | 2 +-
> arch/arm/configs/xcep_defconfig | 2 +-
> arch/arm/configs/zx_defconfig | 2 +-
> arch/arm64/configs/defconfig | 2 +-
> arch/m68k/configs/amcore_defconfig | 2 +-
> arch/mips/configs/ath25_defconfig | 2 +-
> arch/mips/configs/bcm47xx_defconfig | 2 +-
> arch/mips/configs/bmips_be_defconfig | 2 +-
> arch/mips/configs/bmips_stb_defconfig | 2 +-
> arch/mips/configs/ci20_defconfig | 2 +-
> arch/mips/configs/generic_defconfig | 2 +-
> arch/mips/configs/lemote2f_defconfig | 2 +-
> arch/mips/configs/loongson3_defconfig | 2 +-
> arch/mips/configs/nlm_xlp_defconfig | 2 +-
> arch/mips/configs/nlm_xlr_defconfig | 2 +-
> arch/mips/configs/pistachio_defconfig | 2 +-
> arch/mips/configs/qi_lb60_defconfig | 2 +-
> arch/mips/configs/rt305x_defconfig | 2 +-
> arch/mips/configs/xway_defconfig | 2 +-
> arch/parisc/configs/generic-64bit_defconfig | 2 +-
> arch/powerpc/configs/40x/virtex_defconfig | 2 +-
> arch/powerpc/configs/44x/fsp2_defconfig | 2 +-
> arch/powerpc/configs/44x/virtex5_defconfig | 2 +-
> arch/powerpc/configs/44x/warp_defconfig | 2 +-
> arch/powerpc/configs/52xx/cm5200_defconfig | 2 +-
> arch/powerpc/configs/52xx/lite5200b_defconfig | 2 +-
> arch/powerpc/configs/52xx/motionpro_defconfig | 2 +-
> arch/powerpc/configs/52xx/tqm5200_defconfig | 2 +-
> arch/powerpc/configs/gamecube_defconfig | 2 +-
> arch/powerpc/configs/mpc5200_defconfig | 2 +-
> arch/powerpc/configs/pasemi_defconfig | 2 +-
> arch/powerpc/configs/wii_defconfig | 2 +-
> arch/s390/configs/default_defconfig | 2 +-
> arch/s390/configs/gcov_defconfig | 2 +-
> arch/s390/configs/performance_defconfig | 2 +-
> arch/s390/configs/zfcpdump_defconfig | 2 +-
> arch/sh/configs/edosk7760_defconfig | 2 +-
> arch/sh/configs/sdk7786_defconfig | 2 +-
> arch/sh/configs/se7722_defconfig | 2 +-
> arch/sh/configs/sh7785lcr_32bit_defconfig | 2 +-
> arch/sh/configs/urquell_defconfig | 2 +-
> arch/sparc/configs/sparc64_defconfig | 2 +-
> arch/x86/configs/i386_defconfig | 2 +-
> arch/x86/configs/x86_64_defconfig | 2 +-
> arch/xtensa/configs/audio_kc705_defconfig | 2 +-
> arch/xtensa/configs/cadence_csp_defconfig | 2 +-
> arch/xtensa/configs/generic_kc705_defconfig | 2 +-
> arch/xtensa/configs/nommu_kc705_defconfig | 2 +-
> arch/xtensa/configs/smp_lx200_defconfig | 2 +-
Still not quite following why you're updating all the defconfigs. I'd
make sure the Kconfig default settings are right, and leave updating
the defconfig to arch/device maintainers. It adds a lot of noise to
the patch.
> +u64 ktime_get_real_log_ts(u64 *offset_real)
> +{
> + *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> +
> + if (timekeeping_active)
> + return ktime_get_mono_fast_ns();
> + else
> + return local_clock();
> +}
> +
> +u64 ktime_get_boot_log_ts(void)
> +{
> + if (timekeeping_active)
> + return ktime_get_boot_fast_ns();
> + else
> + return local_clock();
> +}
This feels a little tacked on and duplicative. I'd suggest having one
function that returns the offset_real and offset_boot or have a
separate get_mono_log_ts() so its at least consistent. Additionally,
in the commit message, you call out the lack of locking between
grabing the offs_real and calling get_mono_fast_ns(), but I worry it
may be particularly problematic on 32bit systems, and you don't have
any notes in the actual code about it (it looks like an oversight).
Also, when timekeeping_active flips over, and we change from local
clock to the specified clock, do we see a discontinuity in the log? I
know folks use to gripe about that back in the day.
thanks
-john
[toc] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-08-07 19:20 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <ubTR7-3eW-11@gated-at.bofh.it> |
| In reply to | #1705718 |
On Mon, Aug 07, 2017 at 09:52:10AM -0700, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> > +u64 ktime_get_real_log_ts(u64 *offset_real)
> > +{
> > + *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> > +
> > + if (timekeeping_active)
> > + return ktime_get_mono_fast_ns();
> > + else
> > + return local_clock();
> > +}
> > +
> > +u64 ktime_get_boot_log_ts(void)
> > +{
> > + if (timekeeping_active)
> > + return ktime_get_boot_fast_ns();
> > + else
> > + return local_clock();
> > +}
>
> This feels a little tacked on and duplicative. I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent. Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
>
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.
Yeah, yuck, this smells. Please don't mix clocks like this.
I expect all you want is to avoid the explosion you get from calling the
fast things too early, right? Please use the below, which should result
in it returning 0.
---
kernel/time/timekeeping.c | 47 +++++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa008de5..d111039e0245 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -60,8 +60,39 @@ struct tk_fast {
struct tk_read_base base[2];
};
-static struct tk_fast tk_fast_mono ____cacheline_aligned;
-static struct tk_fast tk_fast_raw ____cacheline_aligned;
+/* Suspend-time cycles value for halted fast timekeeper. */
+static u64 cycles_at_suspend;
+
+static u64 dummy_clock_read(struct clocksource *cs)
+{
+ return cycles_at_suspend;
+}
+
+static struct clocksource dummy_clock = {
+ .read = dummy_clock_read,
+};
+
+static struct tk_fast tk_fast_mono ____cacheline_aligned = {
+ .base = {
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ },
+};
+
+static struct tk_fast tk_fast_raw ____cacheline_aligned = {
+ .base = {
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ },
+};
/* flag for if timekeeping is suspended */
int __read_mostly timekeeping_suspended;
@@ -477,18 +508,6 @@ u64 notrace ktime_get_boot_fast_ns(void)
}
EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns);
-/* Suspend-time cycles value for halted fast timekeeper. */
-static u64 cycles_at_suspend;
-
-static u64 dummy_clock_read(struct clocksource *cs)
-{
- return cycles_at_suspend;
-}
-
-static struct clocksource dummy_clock = {
- .read = dummy_clock_read,
-};
-
/**
* halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
* @tk: Timekeeper to snapshot.
[toc] | [prev] | [next] | [standalone]
| From | Prarit Bhargava <prarit@redhat.com> |
|---|---|
| Date | 2017-08-07 20:10 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <ubUDw-3Ol-17@gated-at.bofh.it> |
| In reply to | #1705718 |
On 08/07/2017 12:52 PM, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
>> timestamp to printk messages. The local hardware clock loses time each
>> day making it difficult to determine exactly when an issue has occurred in
>> the kernel log, and making it difficult to determine how kernel and
>> hardware issues relate to each other in real time.
>>
>> Make printk output different timestampes by adding options for no
>> timestamp, the local hardware clock, the monotonic clock, the boottime
>> clock, and the real clock. Allow a user to pick one of the clocks by
>> using the printk.time kernel parameter. Output the type of clock in
>> /sys/module/printk/parameters/time so userspace programs can interpret the
>> timestamp.
>>
>> Real clock & 32-bit systems: Selecting the real clock printk timestamp may
>> lead to unlikely situations where a timestamp is wrong because the real time
>> offset is read without the protection of a sequence lock in the call to
>> ktime_get_log_ts() in printk_get_ts().
>>
>> v2: Use peterz's suggested Kconfig options. Merge patchset together.
>> Fix i386 !CONFIG_PRINTK builds.
>>
>> v3: Fixed x86_64_defconfig. Added printk_time_type enum and
>> printk_time_str for better output. Added BOOTTIME clock functionality.
>>
>> v4: Fix messages, add additional printk.time options, and fix configs.
>>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: Mark Salyzyn <salyzyn@android.com>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Petr Mladek <pmladek@suse.com>
>> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>> Cc: Christoffer Dall <cdall@linaro.org>
>> Cc: Deepa Dinamani <deepa.kernel@gmail.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Joel Fernandes <joelaf@google.com>
>> Cc: Prarit Bhargava <prarit@redhat.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
>> Cc: Olof Johansson <olof@lixom.net>
>> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>> Cc: linux-doc@vger.kernel.org
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 6 +-
>> arch/arm/configs/aspeed_g4_defconfig | 2 +-
>> arch/arm/configs/aspeed_g5_defconfig | 2 +-
>> arch/arm/configs/axm55xx_defconfig | 2 +-
>> arch/arm/configs/bcm2835_defconfig | 2 +-
>> arch/arm/configs/colibri_pxa270_defconfig | 2 +-
>> arch/arm/configs/colibri_pxa300_defconfig | 2 +-
>> arch/arm/configs/dove_defconfig | 2 +-
>> arch/arm/configs/efm32_defconfig | 2 +-
>> arch/arm/configs/exynos_defconfig | 2 +-
>> arch/arm/configs/ezx_defconfig | 2 +-
>> arch/arm/configs/h5000_defconfig | 2 +-
>> arch/arm/configs/hisi_defconfig | 2 +-
>> arch/arm/configs/imote2_defconfig | 2 +-
>> arch/arm/configs/imx_v6_v7_defconfig | 2 +-
>> arch/arm/configs/keystone_defconfig | 2 +-
>> arch/arm/configs/lpc18xx_defconfig | 2 +-
>> arch/arm/configs/magician_defconfig | 2 +-
>> arch/arm/configs/mmp2_defconfig | 2 +-
>> arch/arm/configs/moxart_defconfig | 2 +-
>> arch/arm/configs/mps2_defconfig | 2 +-
>> arch/arm/configs/multi_v7_defconfig | 2 +-
>> arch/arm/configs/mvebu_v7_defconfig | 2 +-
>> arch/arm/configs/mxs_defconfig | 2 +-
>> arch/arm/configs/omap2plus_defconfig | 2 +-
>> arch/arm/configs/pxa168_defconfig | 2 +-
>> arch/arm/configs/pxa3xx_defconfig | 2 +-
>> arch/arm/configs/pxa910_defconfig | 2 +-
>> arch/arm/configs/pxa_defconfig | 2 +-
>> arch/arm/configs/qcom_defconfig | 2 +-
>> arch/arm/configs/raumfeld_defconfig | 2 +-
>> arch/arm/configs/shmobile_defconfig | 2 +-
>> arch/arm/configs/socfpga_defconfig | 2 +-
>> arch/arm/configs/stm32_defconfig | 2 +-
>> arch/arm/configs/sunxi_defconfig | 2 +-
>> arch/arm/configs/tango4_defconfig | 2 +-
>> arch/arm/configs/tegra_defconfig | 2 +-
>> arch/arm/configs/u300_defconfig | 2 +-
>> arch/arm/configs/u8500_defconfig | 2 +-
>> arch/arm/configs/vt8500_v6_v7_defconfig | 2 +-
>> arch/arm/configs/xcep_defconfig | 2 +-
>> arch/arm/configs/zx_defconfig | 2 +-
>> arch/arm64/configs/defconfig | 2 +-
>> arch/m68k/configs/amcore_defconfig | 2 +-
>> arch/mips/configs/ath25_defconfig | 2 +-
>> arch/mips/configs/bcm47xx_defconfig | 2 +-
>> arch/mips/configs/bmips_be_defconfig | 2 +-
>> arch/mips/configs/bmips_stb_defconfig | 2 +-
>> arch/mips/configs/ci20_defconfig | 2 +-
>> arch/mips/configs/generic_defconfig | 2 +-
>> arch/mips/configs/lemote2f_defconfig | 2 +-
>> arch/mips/configs/loongson3_defconfig | 2 +-
>> arch/mips/configs/nlm_xlp_defconfig | 2 +-
>> arch/mips/configs/nlm_xlr_defconfig | 2 +-
>> arch/mips/configs/pistachio_defconfig | 2 +-
>> arch/mips/configs/qi_lb60_defconfig | 2 +-
>> arch/mips/configs/rt305x_defconfig | 2 +-
>> arch/mips/configs/xway_defconfig | 2 +-
>> arch/parisc/configs/generic-64bit_defconfig | 2 +-
>> arch/powerpc/configs/40x/virtex_defconfig | 2 +-
>> arch/powerpc/configs/44x/fsp2_defconfig | 2 +-
>> arch/powerpc/configs/44x/virtex5_defconfig | 2 +-
>> arch/powerpc/configs/44x/warp_defconfig | 2 +-
>> arch/powerpc/configs/52xx/cm5200_defconfig | 2 +-
>> arch/powerpc/configs/52xx/lite5200b_defconfig | 2 +-
>> arch/powerpc/configs/52xx/motionpro_defconfig | 2 +-
>> arch/powerpc/configs/52xx/tqm5200_defconfig | 2 +-
>> arch/powerpc/configs/gamecube_defconfig | 2 +-
>> arch/powerpc/configs/mpc5200_defconfig | 2 +-
>> arch/powerpc/configs/pasemi_defconfig | 2 +-
>> arch/powerpc/configs/wii_defconfig | 2 +-
>> arch/s390/configs/default_defconfig | 2 +-
>> arch/s390/configs/gcov_defconfig | 2 +-
>> arch/s390/configs/performance_defconfig | 2 +-
>> arch/s390/configs/zfcpdump_defconfig | 2 +-
>> arch/sh/configs/edosk7760_defconfig | 2 +-
>> arch/sh/configs/sdk7786_defconfig | 2 +-
>> arch/sh/configs/se7722_defconfig | 2 +-
>> arch/sh/configs/sh7785lcr_32bit_defconfig | 2 +-
>> arch/sh/configs/urquell_defconfig | 2 +-
>> arch/sparc/configs/sparc64_defconfig | 2 +-
>> arch/x86/configs/i386_defconfig | 2 +-
>> arch/x86/configs/x86_64_defconfig | 2 +-
>> arch/xtensa/configs/audio_kc705_defconfig | 2 +-
>> arch/xtensa/configs/cadence_csp_defconfig | 2 +-
>> arch/xtensa/configs/generic_kc705_defconfig | 2 +-
>> arch/xtensa/configs/nommu_kc705_defconfig | 2 +-
>> arch/xtensa/configs/smp_lx200_defconfig | 2 +-
>
> Still not quite following why you're updating all the defconfigs. I'd
> make sure the Kconfig default settings are right, and leave updating
> the defconfig to arch/device maintainers. It adds a lot of noise to
> the patch.
Hmm ... I thought it was up to the patch submitter to make sure that
'make defconfig' still worked? Are you sure I can leave that broken?
/me *really* doesn't want to get yelled at by every arch maintainer.
>
>
>> +u64 ktime_get_real_log_ts(u64 *offset_real)
>> +{
>> + *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
>> +
>> + if (timekeeping_active)
>> + return ktime_get_mono_fast_ns();
>> + else
>> + return local_clock();
>> +}
>> +
>> +u64 ktime_get_boot_log_ts(void)
>> +{
>> + if (timekeeping_active)
>> + return ktime_get_boot_fast_ns();
>> + else
>> + return local_clock();
>> +}
>
> This feels a little tacked on and duplicative. I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent.
I have a better suggestion that I was toying with -- exporting
timekeeping_active and using the existing ktime_get_mono|boot|real|_fast_ns()
functions with a function pointer would simplify this code.
> Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
>
I was told to move that comment to the kdoc description by Luis R. Rodriguez.
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.
>
I have tested this across many systems and haven't seen a discontinunity
yet. I've done both large and small cpu footprint systems and haven't seen
anything like that.
P.
> thanks
> -john
>
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2017-08-07 20:50 +0200 |
| Message-ID | <ubVge-45a-23@gated-at.bofh.it> |
| In reply to | #1705767 |
On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> On 08/07/2017 12:52 PM, John Stultz wrote:
>> Still not quite following why you're updating all the defconfigs. I'd
>> make sure the Kconfig default settings are right, and leave updating
>> the defconfig to arch/device maintainers. It adds a lot of noise to
>> the patch.
>
> Hmm ... I thought it was up to the patch submitter to make sure that
> 'make defconfig' still worked? Are you sure I can leave that broken?
>
> /me *really* doesn't want to get yelled at by every arch maintainer.
No. Don't break systems, but at the same time, can't you use the
default value in Kconfig to set it properly so the old defconfig
settings don't really matter?
Apologies if I've not followed the issue properly, but it is odd, as
I'm not sure I can think of a patch I've seen before that had so much
defconfig noise in it. Again, I've not looked into it closely, so it
may just be my own ignorance, but it makes me suspect there is a
better way.
>>> +u64 ktime_get_real_log_ts(u64 *offset_real)
>>> +{
>>> + *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
>>> +
>>> + if (timekeeping_active)
>>> + return ktime_get_mono_fast_ns();
>>> + else
>>> + return local_clock();
>>> +}
>>> +
>>> +u64 ktime_get_boot_log_ts(void)
>>> +{
>>> + if (timekeeping_active)
>>> + return ktime_get_boot_fast_ns();
>>> + else
>>> + return local_clock();
>>> +}
>>
>> This feels a little tacked on and duplicative. I'd suggest having one
>> function that returns the offset_real and offset_boot or have a
>> separate get_mono_log_ts() so its at least consistent.
>
> I have a better suggestion that I was toying with -- exporting
> timekeeping_active and using the existing ktime_get_mono|boot|real|_fast_ns()
> functions with a function pointer would simplify this code.
Yea. That sounds cleaner.
>> Additionally,
>> in the commit message, you call out the lack of locking between
>> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
>> may be particularly problematic on 32bit systems, and you don't have
>> any notes in the actual code about it (it looks like an oversight).
>>
>
> I was told to move that comment to the kdoc description by Luis R. Rodriguez.
You can have it both ways. :) Its just without any mention near the
function, it just looks buggy (well, because it technically is), so
either we should fix it properly or at least document that its
intentionally buggy (which again, doesn't feel great here - we already
have lots of caveats around the _fast_ns() accessors, so this is
layering subtle breakage on top of other subtle behavior).
>> Also, when timekeeping_active flips over, and we change from local
>> clock to the specified clock, do we see a discontinuity in the log? I
>> know folks use to gripe about that back in the day.
>>
>
> I have tested this across many systems and haven't seen a discontinunity
> yet. I've done both large and small cpu footprint systems and haven't seen
> anything like that.
Ok.
thanks
-john
[toc] | [prev] | [next] | [standalone]
| From | Prarit Bhargava <prarit@redhat.com> |
|---|---|
| Date | 2017-08-07 22:10 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <ubWvE-56O-29@gated-at.bofh.it> |
| In reply to | #1705801 |
On 08/07/2017 02:47 PM, John Stultz wrote: > On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava <prarit@redhat.com> wrote: >> On 08/07/2017 12:52 PM, John Stultz wrote: >>> Still not quite following why you're updating all the defconfigs. I'd >>> make sure the Kconfig default settings are right, and leave updating >>> the defconfig to arch/device maintainers. It adds a lot of noise to >>> the patch. >> >> Hmm ... I thought it was up to the patch submitter to make sure that >> 'make defconfig' still worked? Are you sure I can leave that broken? >> >> /me *really* doesn't want to get yelled at by every arch maintainer. > > No. Don't break systems, but at the same time, can't you use the > default value in Kconfig to set it properly so the old defconfig > settings don't really matter? > > Apologies if I've not followed the issue properly, but it is odd, as > I'm not sure I can think of a patch I've seen before that had so much > defconfig noise in it. Again, I've not looked into it closely, so it > may just be my own ignorance, but it makes me suspect there is a > better way. > peterz? Want to offer a suggestion? The issue is that I'm changing a bool config option to an int and that impacts all the arch's defconfigs. John points out that this is a lot of churn and we're both wondering if there's a better way to do the configs. P.
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-08-07 22:40 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <ubWYF-5rz-1@gated-at.bofh.it> |
| In reply to | #1705827 |
On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote: > > > On 08/07/2017 02:47 PM, John Stultz wrote: > > On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava <prarit@redhat.com> wrote: > >> On 08/07/2017 12:52 PM, John Stultz wrote: > >>> Still not quite following why you're updating all the defconfigs. I'd > >>> make sure the Kconfig default settings are right, and leave updating > >>> the defconfig to arch/device maintainers. It adds a lot of noise to > >>> the patch. > >> > >> Hmm ... I thought it was up to the patch submitter to make sure that > >> 'make defconfig' still worked? Are you sure I can leave that broken? > >> > >> /me *really* doesn't want to get yelled at by every arch maintainer. > > > > No. Don't break systems, but at the same time, can't you use the > > default value in Kconfig to set it properly so the old defconfig > > settings don't really matter? > > > > Apologies if I've not followed the issue properly, but it is odd, as > > I'm not sure I can think of a patch I've seen before that had so much > > defconfig noise in it. Again, I've not looked into it closely, so it > > may just be my own ignorance, but it makes me suspect there is a > > better way. > > > > peterz? Want to offer a suggestion? The issue is that I'm changing a bool > config option to an int and that impacts all the arch's defconfigs. John points > out that this is a lot of churn and we're both wondering if there's a better way > to do the configs. The usual approach is to keep the old bool Kconfig option, and add another int Kconfig option that depends on the original one. The tests for the int value get a bit more complex, but one way to handle this is to define a cpp macro something like the following: #ifdef CONFIG_OLD_OPTION #define CPP_NEW_OPTION 0 #else #define CPP_NEW_OPTION CONFIG_NEW_OPTION #endif Then use CPP_NEW_OPTION, where zero means disabled and other numbers select the available options. Adjust to suit depending on what values mean what. Another approach is to make the range of the new Kconfig option depend on the old option: config NEW_OPTION int "your description here" range 1 5 if OLD_OPTION range 0 0 if !OLD_OPTION default 0 help your help here Again, adjust to suit depending on what values mean what. Thanx, Paul
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-08-08 10:30 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <uc83M-5ix-1@gated-at.bofh.it> |
| In reply to | #1705835 |
On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote: > On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote: > > peterz? Want to offer a suggestion? The issue is that I'm changing a bool > > config option to an int and that impacts all the arch's defconfigs. John points > > out that this is a lot of churn and we're both wondering if there's a better way > > to do the configs. > > The usual approach is to keep the old bool Kconfig option, and add another > int Kconfig option that depends on the original one. The tests for > the int value get a bit more complex, but one way to handle this is to > define a cpp macro something like the following: > > #ifdef CONFIG_OLD_OPTION > #define CPP_NEW_OPTION 0 > #else > #define CPP_NEW_OPTION CONFIG_NEW_OPTION > #endif > > Then use CPP_NEW_OPTION, where zero means disabled and other numbers > select the available options. > > Adjust to suit depending on what values mean what. > > Another approach is to make the range of the new Kconfig option > depend on the old option: > > config NEW_OPTION > int "your description here" > range 1 5 if OLD_OPTION > range 0 0 if !OLD_OPTION > default 0 > help > your help here > > Again, adjust to suit depending on what values mean what. Right this. Except I don't see the !OLD_OPTION working as expected. A 'new' config will not include the old one, so the !OLD_OPTION thing will 'always' be true. So your: > @@ -1,8 +1,46 @@ > menu "printk and dmesg options" > > +choice > + prompt "printk default clock" > + config PRINTK_TIME_DISABLE > + bool "Disabled" > + help > + Selecting this option disables the time stamps of printk(). > + > + config PRINTK_TIME_LOCAL > + bool "Local Clock" > + help > + Selecting this option causes the time stamps of printk() to be > + stamped with the unadjusted hardware clock. > + > + config PRINTK_TIME_BOOT > + bool "CLOCK_BOOTTIME" > + help > + Selecting this option causes the time stamps of printk() to be > + stamped with the adjusted boottime clock. > + > + config PRINTK_TIME_MONO > + bool "CLOCK_MONOTONIC" > + help > + Selecting this option causes the time stamps of printk() to be > + stamped with the adjusted monotonic clock. > + > + config PRINTK_TIME_REAL > + bool "CLOCK_REALTIME" > + help > + Selecting this option causes the time stamps of printk() to be > + stamped with the adjusted realtime clock. > + > +endchoice > + > config PRINTK_TIME Change that into something like: config PRINTK_CLOCK > - bool "Show timing information on printks" > + int "Show time stamp information on printks" > depends on PRINTK > + default 0 if PRINTK_TIME_DISABLE > + default 1 if PRINTK_TIME_LOCAL And that into: default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME > + default 2 if PRINTK_TIME_BOOT > + default 3 if PRINTK_TIME_MONO > + default 4 if PRINTK_TIME_REAL > help > Selecting this option causes time stamps of the printk() Then the old PRINTK_TIME symbol will auto-convert into the new equivalent.
[toc] | [prev] | [next] | [standalone]
| From | Prarit Bhargava <prarit@redhat.com> |
|---|---|
| Date | 2017-08-09 01:10 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <uclNo-6X2-31@gated-at.bofh.it> |
| In reply to | #1706147 |
On 08/08/2017 04:28 AM, Peter Zijlstra wrote: > On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote: >> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote: > >>> peterz? Want to offer a suggestion? The issue is that I'm changing a bool >>> config option to an int and that impacts all the arch's defconfigs. John points >>> out that this is a lot of churn and we're both wondering if there's a better way >>> to do the configs. >> >> The usual approach is to keep the old bool Kconfig option, and add another >> int Kconfig option that depends on the original one. The tests for >> the int value get a bit more complex, but one way to handle this is to >> define a cpp macro something like the following: >> >> #ifdef CONFIG_OLD_OPTION >> #define CPP_NEW_OPTION 0 >> #else >> #define CPP_NEW_OPTION CONFIG_NEW_OPTION >> #endif >> >> Then use CPP_NEW_OPTION, where zero means disabled and other numbers >> select the available options. >> >> Adjust to suit depending on what values mean what. >> >> Another approach is to make the range of the new Kconfig option >> depend on the old option: >> >> config NEW_OPTION >> int "your description here" >> range 1 5 if OLD_OPTION >> range 0 0 if !OLD_OPTION >> default 0 >> help >> your help here >> >> Again, adjust to suit depending on what values mean what. > > Right this. Except I don't see the !OLD_OPTION working as expected. > A 'new' config will not include the old one, so the !OLD_OPTION thing > will 'always' be true. > > So your: > >> @@ -1,8 +1,46 @@ >> menu "printk and dmesg options" >> >> +choice >> + prompt "printk default clock" >> + config PRINTK_TIME_DISABLE >> + bool "Disabled" >> + help >> + Selecting this option disables the time stamps of printk(). >> + >> + config PRINTK_TIME_LOCAL >> + bool "Local Clock" >> + help >> + Selecting this option causes the time stamps of printk() to be >> + stamped with the unadjusted hardware clock. >> + >> + config PRINTK_TIME_BOOT >> + bool "CLOCK_BOOTTIME" >> + help >> + Selecting this option causes the time stamps of printk() to be >> + stamped with the adjusted boottime clock. >> + >> + config PRINTK_TIME_MONO >> + bool "CLOCK_MONOTONIC" >> + help >> + Selecting this option causes the time stamps of printk() to be >> + stamped with the adjusted monotonic clock. >> + >> + config PRINTK_TIME_REAL >> + bool "CLOCK_REALTIME" >> + help >> + Selecting this option causes the time stamps of printk() to be >> + stamped with the adjusted realtime clock. >> + >> +endchoice >> + >> config PRINTK_TIME > > Change that into something like: > > config PRINTK_CLOCK > > >> - bool "Show timing information on printks" >> + int "Show time stamp information on printks" >> depends on PRINTK >> + default 0 if PRINTK_TIME_DISABLE >> + default 1 if PRINTK_TIME_LOCAL > > And that into: > > default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME > >> + default 2 if PRINTK_TIME_BOOT >> + default 3 if PRINTK_TIME_MONO >> + default 4 if PRINTK_TIME_REAL >> help >> Selecting this option causes time stamps of the printk() > > Then the old PRINTK_TIME symbol will auto-convert into the new > equivalent. > I don't think there's an easy code way around this. Essentially this Kconfig code boils down to properly evaluating config PRINTK_CLOCK default 1 if PRINTK_TIME default 0 where there is no Kconfig entry for PRINTK_TIME. If undefined CONFIG_PRINTK_TIME is used in a config, it is immediately scrubbed by the kconfig script so it doesn't "exist" when CONFIG_PRINTK_CLOCK is evaluated. The result of that is CONFIG_PRINT_CLOCK=0. I tried config PRINTK_TIME bool "old config option" then I end up with both a CONFIG_PRINTK_CLOCK=1 and a CONFIG_PRINTK_TIME=y in the resulting config which is confusing. I've debated using the other suggestion that Paul made but TBH (sorry Paul) it seems like I'm avoiding the real but noisy solution of s/PRINTK_TIME=y/PRINTK_TIME=1/g I'm obviously open to other suggestions... P.
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-08-09 19:30 +0200 |
| Subject | Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps |
| Message-ID | <ucCXU-1Uf-29@gated-at.bofh.it> |
| In reply to | #1706944 |
On Tue, Aug 08, 2017 at 07:08:00PM -0400, Prarit Bhargava wrote: > > > On 08/08/2017 04:28 AM, Peter Zijlstra wrote: > > On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote: > >> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote: > > > >>> peterz? Want to offer a suggestion? The issue is that I'm changing a bool > >>> config option to an int and that impacts all the arch's defconfigs. John points > >>> out that this is a lot of churn and we're both wondering if there's a better way > >>> to do the configs. > >> > >> The usual approach is to keep the old bool Kconfig option, and add another > >> int Kconfig option that depends on the original one. The tests for > >> the int value get a bit more complex, but one way to handle this is to > >> define a cpp macro something like the following: > >> > >> #ifdef CONFIG_OLD_OPTION > >> #define CPP_NEW_OPTION 0 > >> #else > >> #define CPP_NEW_OPTION CONFIG_NEW_OPTION > >> #endif > >> > >> Then use CPP_NEW_OPTION, where zero means disabled and other numbers > >> select the available options. > >> > >> Adjust to suit depending on what values mean what. > >> > >> Another approach is to make the range of the new Kconfig option > >> depend on the old option: > >> > >> config NEW_OPTION > >> int "your description here" > >> range 1 5 if OLD_OPTION > >> range 0 0 if !OLD_OPTION > >> default 0 > >> help > >> your help here > >> > >> Again, adjust to suit depending on what values mean what. > > > > Right this. Except I don't see the !OLD_OPTION working as expected. > > A 'new' config will not include the old one, so the !OLD_OPTION thing > > will 'always' be true. > > > > So your: > > > >> @@ -1,8 +1,46 @@ > >> menu "printk and dmesg options" > >> > >> +choice > >> + prompt "printk default clock" > >> + config PRINTK_TIME_DISABLE > >> + bool "Disabled" > >> + help > >> + Selecting this option disables the time stamps of printk(). > >> + > >> + config PRINTK_TIME_LOCAL > >> + bool "Local Clock" > >> + help > >> + Selecting this option causes the time stamps of printk() to be > >> + stamped with the unadjusted hardware clock. > >> + > >> + config PRINTK_TIME_BOOT > >> + bool "CLOCK_BOOTTIME" > >> + help > >> + Selecting this option causes the time stamps of printk() to be > >> + stamped with the adjusted boottime clock. > >> + > >> + config PRINTK_TIME_MONO > >> + bool "CLOCK_MONOTONIC" > >> + help > >> + Selecting this option causes the time stamps of printk() to be > >> + stamped with the adjusted monotonic clock. > >> + > >> + config PRINTK_TIME_REAL > >> + bool "CLOCK_REALTIME" > >> + help > >> + Selecting this option causes the time stamps of printk() to be > >> + stamped with the adjusted realtime clock. > >> + > >> +endchoice > >> + > >> config PRINTK_TIME > > > > Change that into something like: > > > > config PRINTK_CLOCK > > > > > >> - bool "Show timing information on printks" > >> + int "Show time stamp information on printks" > >> depends on PRINTK > >> + default 0 if PRINTK_TIME_DISABLE > >> + default 1 if PRINTK_TIME_LOCAL > > > > And that into: > > > > default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME > > > >> + default 2 if PRINTK_TIME_BOOT > >> + default 3 if PRINTK_TIME_MONO > >> + default 4 if PRINTK_TIME_REAL > >> help > >> Selecting this option causes time stamps of the printk() > > > > Then the old PRINTK_TIME symbol will auto-convert into the new > > equivalent. > > > > I don't think there's an easy code way around this. Essentially this Kconfig > code boils down to properly evaluating > > config PRINTK_CLOCK > default 1 if PRINTK_TIME > default 0 > > where there is no Kconfig entry for PRINTK_TIME. > > If undefined CONFIG_PRINTK_TIME is used in a config, it is immediately > scrubbed by the kconfig script so it doesn't "exist" when CONFIG_PRINTK_CLOCK > is evaluated. The result of that is CONFIG_PRINT_CLOCK=0. > > I tried > > config PRINTK_TIME > bool "old config option" > > then I end up with both a CONFIG_PRINTK_CLOCK=1 and a CONFIG_PRINTK_TIME=y in > the resulting config which is confusing. > > I've debated using the other suggestion that Paul made but TBH (sorry > Paul) it seems like I'm avoiding the real but noisy solution of > > s/PRINTK_TIME=y/PRINTK_TIME=1/g > > I'm obviously open to other suggestions... It is someone else's turn to provide a suggestion. ;-) Thanx, Paul
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web