| Message ID | 1432931068-4980-4-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 2, 2015 at 3:31 AM, Jiri Bohac <jbohac@suse.cz> wrote: > Hi, > > On Fri, May 29, 2015 at 01:24:27PM -0700, John Stultz wrote: >> Looking over the leapsecond code, I noticed the printk messages >> reporting the leapsecond insertion in the second_overflow path >> were not using the printk_deferred method. This was surprising >> since the printk_deferred method was added in part to avoid >> printk-ing while holding the timekeeping locks. >> >> See 6d9bcb621b0b (timekeeping: use printk_deferred when holding >> timekeeping seqlock) for further rational. >> >> I can only guess that this omission was a git add -p oversight. > > second_overflow() is called from accumulate_nsecs_to_secs(). > > accumulate_nsecs_to_secs() is called from update_wall_time() > - once directly > - once via logarithmic_accumulation() > Both calls are before write_seqcount_begin(&tk_core.seq). > > So it looks safe to use printk there. Good point. The update is being done to the shadow-timekeeper, so we won't block readers. This can probably be dropped then. Although I'm almost consider keeping it for consistency so I don't forget this again and worry about it in the future. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Jun 2, 2015 at 3:43 AM, Jiri Kosina <jkosina@suse.cz> wrote: > On Tue, 2 Jun 2015, Jiri Bohac wrote: > >> > Looking over the leapsecond code, I noticed the printk messages >> > reporting the leapsecond insertion in the second_overflow path >> > were not using the printk_deferred method. This was surprising >> > since the printk_deferred method was added in part to avoid >> > printk-ing while holding the timekeeping locks. >> > >> > See 6d9bcb621b0b (timekeeping: use printk_deferred when holding >> > timekeeping seqlock) for further rational. >> > >> > I can only guess that this omission was a git add -p oversight. >> >> second_overflow() is called from accumulate_nsecs_to_secs(). >> >> accumulate_nsecs_to_secs() is called from update_wall_time() >> - once directly >> - once via logarithmic_accumulation() >> Both calls are before write_seqcount_begin(&tk_core.seq). >> >> So it looks safe to use printk there. > > Couldn't we stuff a couple of > > !lockdep_is_held() > > assertions into printk() so that we don't have to keep rediscovering this > sort of problems over and over again? Yea. I was thinking if we could add something very early in printk before we disable lockdep where we lockdep_aquire/release a few of the locks we know printk might take, it would help close the gap on these sorts of call paths that surprise us. Lockdep is *such* a great tool, because it provides some confidence that changes don't cause locking regressions, so to have printk poke a hole in that confidence is frustrating. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index 7a68100..472591e 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -393,7 +393,7 @@ int second_overflow(unsigned long secs) else if (secs % 86400 == 0) { leap = -1; time_state = TIME_OOP; - printk(KERN_NOTICE + printk_deferred(KERN_NOTICE "Clock: inserting leap second 23:59:60 UTC\n"); } break; @@ -403,7 +403,7 @@ int second_overflow(unsigned long secs) else if ((secs + 1) % 86400 == 0) { leap = 1; time_state = TIME_WAIT; - printk(KERN_NOTICE + printk_deferred(KERN_NOTICE "Clock: deleting leap second 23:59:59 UTC\n"); } break;
Looking over the leapsecond code, I noticed the printk messages reporting the leapsecond insertion in the second_overflow path were not using the printk_deferred method. This was surprising since the printk_deferred method was added in part to avoid printk-ing while holding the timekeeping locks. See 6d9bcb621b0b (timekeeping: use printk_deferred when holding timekeeping seqlock) for further rational. I can only guess that this omission was a git add -p oversight. Folks particularly worried about leapsecond crashes should probably pay attention to this patch. Pending review, Its likely a -stable candidate. Cc: Prarit Bhargava <prarit@redhat.com> Cc: Daniel Bristot de Oliveira <bristot@redhat.com> Cc: Richard Cochran <richardcochran@gmail.com> Cc: Jan Kara <jack@suse.cz> Cc: Jiri Bohac <jbohac@suse.cz> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Shuah Khan <shuahkh@osg.samsung.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- kernel/time/ntp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)