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 6C176C433E2 for ; Mon, 22 Mar 2021 18:03:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 367306199E for ; Mon, 22 Mar 2021 18:03:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231704AbhCVSDK (ORCPT ); Mon, 22 Mar 2021 14:03:10 -0400 Received: from mx2.suse.de ([195.135.220.15]:53974 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231463AbhCVSCs (ORCPT ); Mon, 22 Mar 2021 14:02:48 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1616436167; h=from:from:reply-to: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=D5pjW6hl1L/E/T34O6RfxDfAes9ajgyDFpOMsQvndO0=; b=ARHbvQqqW6A+tZ/E1qNFclMm2QCKHvi+lJJpUnJcHZOHvfUgWblMNeIqhQbhYmkEdquavU L23UY55/WkSvPHsqe8uWVaHLajSsSUvn1LEuqfoh7zze+K1wAJpFWHDXQkE33QXbUf7K0x q7MReOqUGRxWOQ5jfqJxFHKyr7rPSQU= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 141A4AB8A; Mon, 22 Mar 2021 18:02:47 +0000 (UTC) Date: Mon, 22 Mar 2021 19:02:43 +0100 From: Petr Mladek To: John Ogness Cc: Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Eric Biederman , Nicholas Piggin , Christophe Leroy , Alistair Popple , Jordan Niethe , Peter Zijlstra , =?iso-8859-1?Q?C=E9dric?= Le Goater , Andrew Morton , Kees Cook , Yue Hu , Alexey Kardashevskiy , Rafael Aquini , Tiezhu Yang , "Guilherme G. Piccoli" , "Paul E. McKenney" , linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org Subject: Re: [PATCH next v1 2/3] printk: remove safe buffers Message-ID: References: <20210316233326.10778-1-john.ogness@linutronix.de> <20210316233326.10778-3-john.ogness@linutronix.de> <87k0pzmoao.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87k0pzmoao.fsf@jogness.linutronix.de> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 2021-03-22 12:16:15, John Ogness wrote: > On 2021-03-21, Sergey Senozhatsky wrote: > >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list args) > >> * Use the main logbuf even in NMI. But avoid calling console > >> * drivers that might have their own locks. > >> */ > >> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) { > >> + if (this_cpu_read(printk_context) & > >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK | > >> + PRINTK_NMI_CONTEXT_MASK | > >> + PRINTK_SAFE_CONTEXT_MASK)) { > > > > Do we need printk_nmi_direct_enter/exit() and > > PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths > > are now DIRECT - we store messages to the prb, but don't call console > > drivers. > > I was planning on waiting until the kthreads are introduced, in which > case printk_safe.c is completely removed. You want to keep printk_safe() context because it prevents calling consoles even in normal context. Namely, it prevents deadlock by recursively taking, for example, sem->lock in console_lock() or console_owner_lock in console_trylock_spinning(). Am I right? > But I suppose I could switch > the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that > PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a > 4th patch of the series. Yes, please unify the PRINTK_NMI_CONTEXT. One is enough. I wonder if it would make sense to go even further at this stage. There will still be 4 contexts that modify the printk behavior after this patchset: + printk_count set by printk_enter()/exit() + prevents: infinite recursion + context: any context + action: skips entire printk at 3rd recursion level + prink_context set by printk_safe_enter()/exit() + prevents: dead lock caused by recursion into some console code in any context + context: any + action: skips console call at 1st recursion level + printk_context set by printk_nmi_enter()/exit() + prevents: dead lock caused by any console lock recursion + context: NMI + action: skips console calls at 0th recursion level + kdb_trap_printk + redirects printk() to kdb_printk() in kdb context What is possible? 1. We could get rid of printk_nmi_enter()/exit() and PRINTK_NMI_CONTEXT completely already now. It is enough to check in_nmi() in printk_func(). printk_nmi_enter() was added by the commit 42a0bb3f71383b457a7db362 ("printk/nmi: generic solution for safe printk in NMI"). It was really needed to modify @printk_func pointer. We did not remove it later when printk_function became a real function. The idea was to track all printk contexts in a single variable. But we never added kdb context. It might make sense to remove it now. Peter Zijstra would be happy. There already were some churns with tracking printk_context in NMI. For example, see https://lore.kernel.org/r/20200219150744.428764577@infradead.org IMHO, it does not make sense to wait until the entire console-stuff rework is done in this case. 2. I thought about unifying printk_safe_enter()/exit() and printk_enter()/exit(). They both count recursion with IRQs disabled, have similar name. But they are used different way. But better might be to rename printk_safe_enter()/exit() to console_enter()/exit() or to printk_deferred_enter()/exit(). It would make more clear what it does now. And it might help to better distinguish it from the new printk_enter()/exit(). This patchset actually splits the original printk_safe() functionality into two: + printk_count prevents infinite recursion + printk_deferred_enter() deffers console handling. I am not sure if it is worth it. But it might help people (even me) when digging into the printk history. Different name will help to understand the functionality at the given time. What do you think, please? Best Regards, Petr 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 F40ABC433C1 for ; Mon, 22 Mar 2021 18:03:20 +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 6640361874 for ; Mon, 22 Mar 2021 18:03:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6640361874 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com 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 4F42RV4xLWz30Mn for ; Tue, 23 Mar 2021 05:03:18 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=suse.com header.i=@suse.com header.a=rsa-sha256 header.s=susede1 header.b=ARHbvQqq; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=suse.com (client-ip=195.135.220.15; helo=mx2.suse.de; envelope-from=pmladek@suse.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=suse.com header.i=@suse.com header.a=rsa-sha256 header.s=susede1 header.b=ARHbvQqq; dkim-atps=neutral Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4F42R04ttDz302v for ; Tue, 23 Mar 2021 05:02:50 +1100 (AEDT) X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1616436167; h=from:from:reply-to: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=D5pjW6hl1L/E/T34O6RfxDfAes9ajgyDFpOMsQvndO0=; b=ARHbvQqqW6A+tZ/E1qNFclMm2QCKHvi+lJJpUnJcHZOHvfUgWblMNeIqhQbhYmkEdquavU L23UY55/WkSvPHsqe8uWVaHLajSsSUvn1LEuqfoh7zze+K1wAJpFWHDXQkE33QXbUf7K0x q7MReOqUGRxWOQ5jfqJxFHKyr7rPSQU= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 141A4AB8A; Mon, 22 Mar 2021 18:02:47 +0000 (UTC) Date: Mon, 22 Mar 2021 19:02:43 +0100 From: Petr Mladek To: John Ogness Subject: Re: [PATCH next v1 2/3] printk: remove safe buffers Message-ID: References: <20210316233326.10778-1-john.ogness@linutronix.de> <20210316233326.10778-3-john.ogness@linutronix.de> <87k0pzmoao.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87k0pzmoao.fsf@jogness.linutronix.de> 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: Rafael Aquini , Alexey Kardashevskiy , Paul Mackerras , Tiezhu Yang , 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, =?iso-8859-1?Q?C=E9dric?= Le Goater Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Mon 2021-03-22 12:16:15, John Ogness wrote: > On 2021-03-21, Sergey Senozhatsky wrote: > >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list args) > >> * Use the main logbuf even in NMI. But avoid calling console > >> * drivers that might have their own locks. > >> */ > >> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) { > >> + if (this_cpu_read(printk_context) & > >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK | > >> + PRINTK_NMI_CONTEXT_MASK | > >> + PRINTK_SAFE_CONTEXT_MASK)) { > > > > Do we need printk_nmi_direct_enter/exit() and > > PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths > > are now DIRECT - we store messages to the prb, but don't call console > > drivers. > > I was planning on waiting until the kthreads are introduced, in which > case printk_safe.c is completely removed. You want to keep printk_safe() context because it prevents calling consoles even in normal context. Namely, it prevents deadlock by recursively taking, for example, sem->lock in console_lock() or console_owner_lock in console_trylock_spinning(). Am I right? > But I suppose I could switch > the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that > PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a > 4th patch of the series. Yes, please unify the PRINTK_NMI_CONTEXT. One is enough. I wonder if it would make sense to go even further at this stage. There will still be 4 contexts that modify the printk behavior after this patchset: + printk_count set by printk_enter()/exit() + prevents: infinite recursion + context: any context + action: skips entire printk at 3rd recursion level + prink_context set by printk_safe_enter()/exit() + prevents: dead lock caused by recursion into some console code in any context + context: any + action: skips console call at 1st recursion level + printk_context set by printk_nmi_enter()/exit() + prevents: dead lock caused by any console lock recursion + context: NMI + action: skips console calls at 0th recursion level + kdb_trap_printk + redirects printk() to kdb_printk() in kdb context What is possible? 1. We could get rid of printk_nmi_enter()/exit() and PRINTK_NMI_CONTEXT completely already now. It is enough to check in_nmi() in printk_func(). printk_nmi_enter() was added by the commit 42a0bb3f71383b457a7db362 ("printk/nmi: generic solution for safe printk in NMI"). It was really needed to modify @printk_func pointer. We did not remove it later when printk_function became a real function. The idea was to track all printk contexts in a single variable. But we never added kdb context. It might make sense to remove it now. Peter Zijstra would be happy. There already were some churns with tracking printk_context in NMI. For example, see https://lore.kernel.org/r/20200219150744.428764577@infradead.org IMHO, it does not make sense to wait until the entire console-stuff rework is done in this case. 2. I thought about unifying printk_safe_enter()/exit() and printk_enter()/exit(). They both count recursion with IRQs disabled, have similar name. But they are used different way. But better might be to rename printk_safe_enter()/exit() to console_enter()/exit() or to printk_deferred_enter()/exit(). It would make more clear what it does now. And it might help to better distinguish it from the new printk_enter()/exit(). This patchset actually splits the original printk_safe() functionality into two: + printk_count prevents infinite recursion + printk_deferred_enter() deffers console handling. I am not sure if it is worth it. But it might help people (even me) when digging into the printk history. Different name will help to understand the functionality at the given time. What do you think, please? Best Regards, Petr From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx2.suse.de ([195.135.220.15]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lOOt5-00CDhA-Ru for kexec@lists.infradead.org; Mon, 22 Mar 2021 18:02:50 +0000 Date: Mon, 22 Mar 2021 19:02:43 +0100 From: Petr Mladek Subject: Re: [PATCH next v1 2/3] printk: remove safe buffers Message-ID: References: <20210316233326.10778-1-john.ogness@linutronix.de> <20210316233326.10778-3-john.ogness@linutronix.de> <87k0pzmoao.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87k0pzmoao.fsf@jogness.linutronix.de> 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: John Ogness Cc: Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Eric Biederman , Nicholas Piggin , Christophe Leroy , Alistair Popple , Jordan Niethe , Peter Zijlstra , =?iso-8859-1?Q?C=E9dric?= Le Goater , Andrew Morton , Kees Cook , Yue Hu , Alexey Kardashevskiy , Rafael Aquini , Tiezhu Yang , "Guilherme G. Piccoli" , "Paul E. McKenney" , linuxppc-dev@lists.ozlabs.org, kexec@lists.infradead.org On Mon 2021-03-22 12:16:15, John Ogness wrote: > On 2021-03-21, Sergey Senozhatsky wrote: > >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list args) > >> * Use the main logbuf even in NMI. But avoid calling console > >> * drivers that might have their own locks. > >> */ > >> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) { > >> + if (this_cpu_read(printk_context) & > >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK | > >> + PRINTK_NMI_CONTEXT_MASK | > >> + PRINTK_SAFE_CONTEXT_MASK)) { > > > > Do we need printk_nmi_direct_enter/exit() and > > PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths > > are now DIRECT - we store messages to the prb, but don't call console > > drivers. > > I was planning on waiting until the kthreads are introduced, in which > case printk_safe.c is completely removed. You want to keep printk_safe() context because it prevents calling consoles even in normal context. Namely, it prevents deadlock by recursively taking, for example, sem->lock in console_lock() or console_owner_lock in console_trylock_spinning(). Am I right? > But I suppose I could switch > the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that > PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a > 4th patch of the series. Yes, please unify the PRINTK_NMI_CONTEXT. One is enough. I wonder if it would make sense to go even further at this stage. There will still be 4 contexts that modify the printk behavior after this patchset: + printk_count set by printk_enter()/exit() + prevents: infinite recursion + context: any context + action: skips entire printk at 3rd recursion level + prink_context set by printk_safe_enter()/exit() + prevents: dead lock caused by recursion into some console code in any context + context: any + action: skips console call at 1st recursion level + printk_context set by printk_nmi_enter()/exit() + prevents: dead lock caused by any console lock recursion + context: NMI + action: skips console calls at 0th recursion level + kdb_trap_printk + redirects printk() to kdb_printk() in kdb context What is possible? 1. We could get rid of printk_nmi_enter()/exit() and PRINTK_NMI_CONTEXT completely already now. It is enough to check in_nmi() in printk_func(). printk_nmi_enter() was added by the commit 42a0bb3f71383b457a7db362 ("printk/nmi: generic solution for safe printk in NMI"). It was really needed to modify @printk_func pointer. We did not remove it later when printk_function became a real function. The idea was to track all printk contexts in a single variable. But we never added kdb context. It might make sense to remove it now. Peter Zijstra would be happy. There already were some churns with tracking printk_context in NMI. For example, see https://lore.kernel.org/r/20200219150744.428764577@infradead.org IMHO, it does not make sense to wait until the entire console-stuff rework is done in this case. 2. I thought about unifying printk_safe_enter()/exit() and printk_enter()/exit(). They both count recursion with IRQs disabled, have similar name. But they are used different way. But better might be to rename printk_safe_enter()/exit() to console_enter()/exit() or to printk_deferred_enter()/exit(). It would make more clear what it does now. And it might help to better distinguish it from the new printk_enter()/exit(). This patchset actually splits the original printk_safe() functionality into two: + printk_count prevents infinite recursion + printk_deferred_enter() deffers console handling. I am not sure if it is worth it. But it might help people (even me) when digging into the printk history. Different name will help to understand the functionality at the given time. What do you think, please? Best Regards, Petr _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec