From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 218EBC43460 for ; Thu, 1 Apr 2021 18:17:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DCEA96023C for ; Thu, 1 Apr 2021 18:17:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239554AbhDASQh (ORCPT ); Thu, 1 Apr 2021 14:16:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32876 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236795AbhDAR6m (ORCPT ); Thu, 1 Apr 2021 13:58:42 -0400 Received: from galois.linutronix.de (Galois.linutronix.de [IPv6:2a0a:51c0:0:12e:550::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 70B00C08E81A for ; Thu, 1 Apr 2021 06:20:02 -0700 (PDT) From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1617283194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ropfd/50hZwFHygWvoKwWUv09SHlJREMcmKg0y/1COk=; b=w4oEaTFxHJvv3fqbRVPTIUlUbLzczXOhRKdyeF4GvLQP185Tberl86XRlFEJoX/AGcs+1N PFWuHAkJaPq/eyChQAN8RQHAy6FcaTZP/itmUCwpRj6Ws413xT6OkUB++KuMIpsSNjxhGR M1NS9Avk4diwz5/PMur9fUzzJRwKj4n9cS+HQVzXKva8E9UKXjamvB/9kntsOuCcwAbfqe bqvr0W/f0ltISNaSfpoBFOUG6HYkv5SBd0YTPHiT5W3ZZ+2HOC/ZcTZR88sMXHKtf4QcaD 6qjwd/JpUfsZWIwddumcLQVSi8CRRObqQ+1vk3FB5GI300dd9g9I4nNwUOmV5Q== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1617283194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ropfd/50hZwFHygWvoKwWUv09SHlJREMcmKg0y/1COk=; b=GSHqJTUL1+ALVe/5Kh9GKkiAVzFtP+9xFkfM37puBv2WdO1AxshdgLE2Iv9js81yEIcZ3L eUZabLq8YYv87HBg== To: Petr Mladek Cc: Sergey Senozhatsky , Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Eric Biederman , Christophe Leroy , Nicholas Piggin , Alistair Popple , Jordan Niethe , Peter Zijlstra , "Aneesh Kumar K.V" , Andrew Morton , Kees Cook , Tiezhu Yang , Alexey Kardashevskiy , Yue Hu , Rafael Aquini , "Guilherme G. Piccoli" , "Paul E. McKenney" , linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org Subject: Re: [PATCH printk v2 2/5] printk: remove safe buffers In-Reply-To: References: <20210330153512.1182-1-john.ogness@linutronix.de> <20210330153512.1182-3-john.ogness@linutronix.de> Date: Thu, 01 Apr 2021 15:19:52 +0200 Message-ID: <87a6qiqgzr.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-04-01, Petr Mladek wrote: >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -1142,24 +1128,37 @@ void __init setup_log_buf(int early) >> new_descs, ilog2(new_descs_count), >> new_infos); >> >> - printk_safe_enter_irqsave(flags); >> + local_irq_save(flags); > > IMHO, we actually do not have to disable IRQ here. We already copy > messages that might appear in the small race window in NMI. It would > work the same way also for IRQ context. We do not have to, but why open up this window? We are still in early boot and interrupts have always been disabled here. I am not happy that this window even exists. I really prefer to keep it NMI-only. >> --- a/lib/nmi_backtrace.c >> +++ b/lib/nmi_backtrace.c >> @@ -75,12 +75,6 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask, >> touch_softlockup_watchdog(); >> } >> >> - /* >> - * Force flush any remote buffers that might be stuck in IRQ context >> - * and therefore could not run their irq_work. >> - */ >> - printk_safe_flush(); > > Sigh, this reminds me that the nmi_safe buffers serialized backtraces > from all CPUs. > > I am afraid that we have to put back the spinlock into > nmi_cpu_backtrace(). Please no. That spinlock is a disaster. It can cause deadlocks with other cpu-locks (such as in kdb) and it will cause a major problem for atomic consoles. We need to be very careful about introducing locks where NMIs are waiting on other CPUs. > It has been repeatedly added and removed depending > on whether the backtrace was printed into the main log buffer > or into the per-CPU buffers. Last time it was removed by > the commit 03fc7f9c99c1e7ae2925d ("printk/nmi: Prevent deadlock > when accessing the main log buffer in NMI"). > > It should be safe because there should not be any other locks in the > code path. Note that only one backtrace might be triggered at the same > time, see @backtrace_flag in nmi_trigger_cpumask_backtrace(). It is adding a lock around a lockless ringbuffer. For me that is a step backwards. > We _must_ serialize it somehow[*]. The lock in nmi_cpu_backtrace() > looks less evil than the nmi_safe machinery. nmi_safe() shrinks > too long backtraces, lose timestamps, needs to be explicitely > flushed here and there, is a non-trivial code. > > [*] Non-serialized bactraces are real mess. Caller-id is visible > only on consoles or via syslogd interface. And it is not much > convenient. Caller-id solves this problem and is easy to sort for anyone with `grep'. Yes, it is a shame that `dmesg' does not show it, but directly using any of the printk interfaces does show it (kmsg_dump, /dev/kmsg, syslog, console). > I get this with "echo l >/proc/sysrq-trigger" and this patchset: Of course. Without caller-id, it is a mess. But this has nothing to do with NMI. The same problem exists for WARN_ON() on multiple CPUs simultaneously. If the user is not using caller-id, they are lost. Caller-id is the current solution to the interlaced logs. For the long term, we should introduce a printk-context API that allows callers to perfectly pack their multi-line output into a single entry. We discussed [0][1] this back in August 2020. John Ogness [0] https://lore.kernel.org/lkml/472f2e553805b52d9834d64e4056db965edee329.camel@perches.com [1] offlist message-id: 87d03k9ymz.fsf@jogness.linutronix.de From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A75BDC433B4 for ; Thu, 1 Apr 2021 13:20:32 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9A01A61210 for ; Thu, 1 Apr 2021 13:20:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9A01A61210 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linutronix.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4FB3hY5tF7z3brd for ; Fri, 2 Apr 2021 00:20:29 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; secure) header.d=linutronix.de header.i=@linutronix.de header.a=rsa-sha256 header.s=2020 header.b=w4oEaTFx; dkim=fail reason="signature verification failed" header.d=linutronix.de header.i=@linutronix.de header.a=ed25519-sha256 header.s=2020e header.b=GSHqJTUL; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linutronix.de (client-ip=193.142.43.55; helo=galois.linutronix.de; envelope-from=john.ogness@linutronix.de; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; secure) header.d=linutronix.de header.i=@linutronix.de header.a=rsa-sha256 header.s=2020 header.b=w4oEaTFx; dkim=pass header.d=linutronix.de header.i=@linutronix.de header.a=ed25519-sha256 header.s=2020e header.b=GSHqJTUL; dkim-atps=neutral Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4FB3gy6fk3z303M for ; Fri, 2 Apr 2021 00:19:58 +1100 (AEDT) From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1617283194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ropfd/50hZwFHygWvoKwWUv09SHlJREMcmKg0y/1COk=; b=w4oEaTFxHJvv3fqbRVPTIUlUbLzczXOhRKdyeF4GvLQP185Tberl86XRlFEJoX/AGcs+1N PFWuHAkJaPq/eyChQAN8RQHAy6FcaTZP/itmUCwpRj6Ws413xT6OkUB++KuMIpsSNjxhGR M1NS9Avk4diwz5/PMur9fUzzJRwKj4n9cS+HQVzXKva8E9UKXjamvB/9kntsOuCcwAbfqe bqvr0W/f0ltISNaSfpoBFOUG6HYkv5SBd0YTPHiT5W3ZZ+2HOC/ZcTZR88sMXHKtf4QcaD 6qjwd/JpUfsZWIwddumcLQVSi8CRRObqQ+1vk3FB5GI300dd9g9I4nNwUOmV5Q== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1617283194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ropfd/50hZwFHygWvoKwWUv09SHlJREMcmKg0y/1COk=; b=GSHqJTUL1+ALVe/5Kh9GKkiAVzFtP+9xFkfM37puBv2WdO1AxshdgLE2Iv9js81yEIcZ3L eUZabLq8YYv87HBg== To: Petr Mladek Subject: Re: [PATCH printk v2 2/5] printk: remove safe buffers In-Reply-To: References: <20210330153512.1182-1-john.ogness@linutronix.de> <20210330153512.1182-3-john.ogness@linutronix.de> Date: Thu, 01 Apr 2021 15:19:52 +0200 Message-ID: <87a6qiqgzr.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sergey Senozhatsky , Alexey Kardashevskiy , Paul Mackerras , Tiezhu Yang , Rafael Aquini , "Aneesh Kumar K.V" , Peter Zijlstra , Yue Hu , Jordan Niethe , Kees Cook , "Paul E. McKenney" , Alistair Popple , "Guilherme G. Piccoli" , Nicholas Piggin , Steven Rostedt , Thomas Gleixner , kexec@lists.infradead.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Eric Biederman , Andrew Morton , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On 2021-04-01, Petr Mladek wrote: >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -1142,24 +1128,37 @@ void __init setup_log_buf(int early) >> new_descs, ilog2(new_descs_count), >> new_infos); >> >> - printk_safe_enter_irqsave(flags); >> + local_irq_save(flags); > > IMHO, we actually do not have to disable IRQ here. We already copy > messages that might appear in the small race window in NMI. It would > work the same way also for IRQ context. We do not have to, but why open up this window? We are still in early boot and interrupts have always been disabled here. I am not happy that this window even exists. I really prefer to keep it NMI-only. >> --- a/lib/nmi_backtrace.c >> +++ b/lib/nmi_backtrace.c >> @@ -75,12 +75,6 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask, >> touch_softlockup_watchdog(); >> } >> >> - /* >> - * Force flush any remote buffers that might be stuck in IRQ context >> - * and therefore could not run their irq_work. >> - */ >> - printk_safe_flush(); > > Sigh, this reminds me that the nmi_safe buffers serialized backtraces > from all CPUs. > > I am afraid that we have to put back the spinlock into > nmi_cpu_backtrace(). Please no. That spinlock is a disaster. It can cause deadlocks with other cpu-locks (such as in kdb) and it will cause a major problem for atomic consoles. We need to be very careful about introducing locks where NMIs are waiting on other CPUs. > It has been repeatedly added and removed depending > on whether the backtrace was printed into the main log buffer > or into the per-CPU buffers. Last time it was removed by > the commit 03fc7f9c99c1e7ae2925d ("printk/nmi: Prevent deadlock > when accessing the main log buffer in NMI"). > > It should be safe because there should not be any other locks in the > code path. Note that only one backtrace might be triggered at the same > time, see @backtrace_flag in nmi_trigger_cpumask_backtrace(). It is adding a lock around a lockless ringbuffer. For me that is a step backwards. > We _must_ serialize it somehow[*]. The lock in nmi_cpu_backtrace() > looks less evil than the nmi_safe machinery. nmi_safe() shrinks > too long backtraces, lose timestamps, needs to be explicitely > flushed here and there, is a non-trivial code. > > [*] Non-serialized bactraces are real mess. Caller-id is visible > only on consoles or via syslogd interface. And it is not much > convenient. Caller-id solves this problem and is easy to sort for anyone with `grep'. Yes, it is a shame that `dmesg' does not show it, but directly using any of the printk interfaces does show it (kmsg_dump, /dev/kmsg, syslog, console). > I get this with "echo l >/proc/sysrq-trigger" and this patchset: Of course. Without caller-id, it is a mess. But this has nothing to do with NMI. The same problem exists for WARN_ON() on multiple CPUs simultaneously. If the user is not using caller-id, they are lost. Caller-id is the current solution to the interlaced logs. For the long term, we should introduce a printk-context API that allows callers to perfectly pack their multi-line output into a single entry. We discussed [0][1] this back in August 2020. John Ogness [0] https://lore.kernel.org/lkml/472f2e553805b52d9834d64e4056db965edee329.camel@perches.com [1] offlist message-id: 87d03k9ymz.fsf@jogness.linutronix.de From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galois.linutronix.de ([193.142.43.55]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lRxEq-009fUq-C5 for kexec@lists.infradead.org; Thu, 01 Apr 2021 13:19:58 +0000 From: John Ogness Subject: Re: [PATCH printk v2 2/5] printk: remove safe buffers In-Reply-To: References: <20210330153512.1182-1-john.ogness@linutronix.de> <20210330153512.1182-3-john.ogness@linutronix.de> Date: Thu, 01 Apr 2021 15:19:52 +0200 Message-ID: <87a6qiqgzr.fsf@jogness.linutronix.de> MIME-Version: 1.0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Petr Mladek Cc: Sergey Senozhatsky , Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Eric Biederman , Christophe Leroy , Nicholas Piggin , Alistair Popple , Jordan Niethe , Peter Zijlstra , "Aneesh Kumar K.V" , Andrew Morton , Kees Cook , Tiezhu Yang , Alexey Kardashevskiy , Yue Hu , Rafael Aquini , "Guilherme G. Piccoli" , "Paul E. McKenney" , linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org On 2021-04-01, Petr Mladek wrote: >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -1142,24 +1128,37 @@ void __init setup_log_buf(int early) >> new_descs, ilog2(new_descs_count), >> new_infos); >> >> - printk_safe_enter_irqsave(flags); >> + local_irq_save(flags); > > IMHO, we actually do not have to disable IRQ here. We already copy > messages that might appear in the small race window in NMI. It would > work the same way also for IRQ context. We do not have to, but why open up this window? We are still in early boot and interrupts have always been disabled here. I am not happy that this window even exists. I really prefer to keep it NMI-only. >> --- a/lib/nmi_backtrace.c >> +++ b/lib/nmi_backtrace.c >> @@ -75,12 +75,6 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask, >> touch_softlockup_watchdog(); >> } >> >> - /* >> - * Force flush any remote buffers that might be stuck in IRQ context >> - * and therefore could not run their irq_work. >> - */ >> - printk_safe_flush(); > > Sigh, this reminds me that the nmi_safe buffers serialized backtraces > from all CPUs. > > I am afraid that we have to put back the spinlock into > nmi_cpu_backtrace(). Please no. That spinlock is a disaster. It can cause deadlocks with other cpu-locks (such as in kdb) and it will cause a major problem for atomic consoles. We need to be very careful about introducing locks where NMIs are waiting on other CPUs. > It has been repeatedly added and removed depending > on whether the backtrace was printed into the main log buffer > or into the per-CPU buffers. Last time it was removed by > the commit 03fc7f9c99c1e7ae2925d ("printk/nmi: Prevent deadlock > when accessing the main log buffer in NMI"). > > It should be safe because there should not be any other locks in the > code path. Note that only one backtrace might be triggered at the same > time, see @backtrace_flag in nmi_trigger_cpumask_backtrace(). It is adding a lock around a lockless ringbuffer. For me that is a step backwards. > We _must_ serialize it somehow[*]. The lock in nmi_cpu_backtrace() > looks less evil than the nmi_safe machinery. nmi_safe() shrinks > too long backtraces, lose timestamps, needs to be explicitely > flushed here and there, is a non-trivial code. > > [*] Non-serialized bactraces are real mess. Caller-id is visible > only on consoles or via syslogd interface. And it is not much > convenient. Caller-id solves this problem and is easy to sort for anyone with `grep'. Yes, it is a shame that `dmesg' does not show it, but directly using any of the printk interfaces does show it (kmsg_dump, /dev/kmsg, syslog, console). > I get this with "echo l >/proc/sysrq-trigger" and this patchset: Of course. Without caller-id, it is a mess. But this has nothing to do with NMI. The same problem exists for WARN_ON() on multiple CPUs simultaneously. If the user is not using caller-id, they are lost. Caller-id is the current solution to the interlaced logs. For the long term, we should introduce a printk-context API that allows callers to perfectly pack their multi-line output into a single entry. We discussed [0][1] this back in August 2020. John Ogness [0] https://lore.kernel.org/lkml/472f2e553805b52d9834d64e4056db965edee329.camel@perches.com [1] offlist message-id: 87d03k9ymz.fsf@jogness.linutronix.de _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec