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 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 07C62C433DB for ; Tue, 23 Mar 2021 10:47:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CCDC261582 for ; Tue, 23 Mar 2021 10:47:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230284AbhCWKrS (ORCPT ); Tue, 23 Mar 2021 06:47:18 -0400 Received: from mx2.suse.de ([195.135.220.15]:39698 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230305AbhCWKrL (ORCPT ); Tue, 23 Mar 2021 06:47:11 -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=1616496429; 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=NaRa85INpsZEqfLCsgSNBVcWiWiVhHqblotAh21dc+g=; b=oHzwm0gbjvu2H5TBXXIAZpZ2mBJqANURQ9iX6ikT28yQVji58JxGeeppjss0XrZM1ZZWea caTlRvhtIaZKPvfm2oLB1aYphDNG6LDGwjb1hEScbeSQxt3V1REGNc9Ip/XFnnXpxWMwqN Rq4yFjFTmENEiqJ9XtTZAmurd9j+EgY= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 8A7A0AD6D; Tue, 23 Mar 2021 10:47:09 +0000 (UTC) Date: Tue, 23 Mar 2021 11:47:06 +0100 From: Petr Mladek To: John Ogness Cc: Sergey Senozhatsky , 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210316233326.10778-3-john.ogness@linutronix.de> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 2021-03-17 00:33:25, John Ogness wrote: > With @logbuf_lock removed, the high level printk functions for > storing messages are lockless. Messages can be stored from any > context, so there is no need for the NMI and safe buffers anymore. > Remove the NMI and safe buffers. > > Although the safe buffers are removed, the NMI and safe context > tracking is still in place. In these contexts, store the message > immediately but still use irq_work to defer the console printing. > > Since printk recursion tracking is in place, safe context tracking > for most of printk is not needed. Remove it. Only safe context > tracking relating to the console lock is left in place. This is > because the console lock is needed for the actual printing. I have two more questions after actually checking the entire patch. See below. > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1084,7 +1069,6 @@ void __init setup_log_buf(int early) > struct printk_record r; > size_t new_descs_size; > size_t new_infos_size; > - unsigned long flags; > char *new_log_buf; > unsigned int free; > u64 seq; > @@ -1142,8 +1126,6 @@ void __init setup_log_buf(int early) > new_descs, ilog2(new_descs_count), > new_infos); > > - printk_safe_enter_irqsave(flags); > - > log_buf_len = new_log_buf_len; > log_buf = new_log_buf; > new_log_buf_len = 0; > @@ -1159,8 +1141,6 @@ void __init setup_log_buf(int early) > */ > prb = &printk_rb_dynamic; > > - printk_safe_exit_irqrestore(flags); This will allow to add new messages from the IRQ context when we are copying them to the new buffer. They might get lost in the small race window. Also the messages from NMI might get lost because they are not longer stored in the per-CPU buffer. A possible solution might be to do something like this: prb_for_each_record(0, &printk_rb_static, seq, &r) free -= add_to_rb(&printk_rb_dynamic, &r); prb = &printk_rb_dynamic; /* * Copy the remaining messages that might have appeared * from IRQ or NMI context after we ended copying and * before we switched the buffers. They must be finalized * because only one CPU is up at this stage. */ prb_for_each_record(seq, &printk_rb_static, seq, &r) free -= add_to_rb(&printk_rb_dynamic, &r); > - > if (seq != prb_next_seq(&printk_rb_static)) { > pr_err("dropped %llu messages\n", > prb_next_seq(&printk_rb_static) - seq); > @@ -2666,7 +2631,6 @@ void console_unlock(void) > size_t ext_len = 0; > size_t len; > > - printk_safe_enter_irqsave(flags); > skip: > if (!prb_read_valid(prb, console_seq, &r)) > break; > @@ -2711,6 +2675,8 @@ void console_unlock(void) > printk_time); > console_seq++; > > + printk_safe_enter_irqsave(flags); What is the purpose of the printk_safe context here, please? I guess that you wanted to prevent calling console drivers recursively. But it is already serialized by console_lock(). IMHO, the only risk is when manipulating console_sem->lock or console_owner_lock. But they are already guarded by printk_safe context, for example, in console_lock() or console_lock_spinning_enable(). Do I miss something, please? > + > /* > * While actively printing out messages, if another printk() > * were to occur on another CPU, it may wait for this one to > @@ -2745,8 +2711,6 @@ void console_unlock(void) > * flush, no worries. > */ > retry = prb_read_valid(prb, console_seq, NULL); > - printk_safe_exit_irqrestore(flags); > - > if (retry && console_trylock()) > goto again; > } Heh, all these patches feels like stripping printk of an armour. I hope that we trained it enough to be flexible and avoid any damage. Best Regards, Petr