Received-SPF: pass (google.com: domain of linux-kernel+bounces-65057-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Received-SPF: Fail (protection.outlook.com: domain of sony.com does not
 designate 121.100.38.198 as permitted sender)
 receiver=protection.outlook.com; client-ip=121.100.38.198;
 helo=gepdcl09.sg.gdce.sony.com.sg;
Date: Wed, 14 Feb 2024 16:03:50 +0530
From: Sreenath Vijayan <sreenath.vijayan@sony.com>
To: Petr Mladek <pmladek@suse.com>
Cc: john.ogness@linutronix.de, corbet@lwn.net, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, rdunlap@infradead.org, rostedt@goodmis.org,
        senozhatsky@chromium.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        taichi.shimoyashiki@sony.com, daniel.palmer@sony.com,
        anandakumar.balasubramaniam@sony.com
Subject: Re: [PATCH v4 1/2] printk: Add function to dump printk buffer
 directly to consoles
Message-ID: <ZcyXDvBBqYvkLr9e@sony.com>
References: <cover.1706772349.git.sreenath.vijayan@sony.com>
 <8cb5936021c5811bd03a6bc18300b1384009ac26.1706772349.git.sreenath.vijayan@sony.com>
 <ZcOXEyPsRnfewb4Y@alley>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcOXEyPsRnfewb4Y@alley>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2024 10:35:55.2605
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 56417c6c-81c2-4744-c38d-08dc2d48b9e7
X-MS-Exchange-CrossTenant-Id: 66c65d8a-9158-4521-a2d8-664963db48e4
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=66c65d8a-9158-4521-a2d8-664963db48e4;Ip=[121.100.38.198];Helo=[gepdcl09.sg.gdce.sony.com.sg]
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: TreatMessagesAsInternal-CY4PEPF0000E9DC.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR13MB5990

On Wed, Feb 07, 2024 at 03:43:52PM +0100, Petr Mladek wrote:
> On Thu 2024-02-01 15:53:40, Sreenath Vijayan wrote:
> > It is useful to be able to dump the printk buffer directly to
> > consoles in some situations so as to not flood the buffer.
> 
> This is not longer true. I think that it was valid for
> the previous versions which used separate buffers with
> the kmsg_dump API.
> 
> > To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
> > console_flush_on_panic() by moving the code to a helper function
> > console_rewind_all(). This is done because console_flush_on_panic()
> > sets console_may_schedule to 0 but this should not be done in our
> > case.
> 
> Also the "c->seq = seq;" is not safe in the panic version.
> But it will be safe when called under the console_lock.
> 
> > Then console_rewind_all() is called from the new function
> > dump_printk_buffer() with console lock held to set the console
> > sequence number to oldest record in the buffer for all consoles.
> > Releasing the console lock will flush the contents of printk buffer
> > to the consoles.
> 
> My proposed commit message is:
> 
> <proposal>
> Add a generic function for replaying the kernel log on consoles.
> It would allow seeing the the log on an unresponsive terminal
> via sysrq interface.
> 
> Reuse the existing code from console_flush_on_panic() for
> reseting the sequence numbers. It will be safe when called
> under console_lock(). Also the console_unlock() will
> automatically flush the messages on the consoles.
> 
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3134,6 +3134,32 @@ void console_unblank(void)
> >  		pr_flush(1000, true);
> >  }
> >  
> 
> I would call this function __console_rewind_all(void)
> because it is not safe on its own. Also It would
> deserve a comment, something like:
> 
> /*
>  * Rewind all consoles to the oldest available record.
>  *
>  * IMPORTANT: The function is safe only when called under
>  *            console_lock(). It is not enforced because
>  *	      it is used as a best effort in panic().
>  */
> static void __console_rewind_all(void)
> 
> 
> This would deserve a comment because it is not safe by
> default.
> 
> > +static void console_rewind_all(void)
> > +{
> > +	struct console *c;
> > +	short flags;
> > +	int cookie;
> > +	u64 seq;
> > +
> > +	seq = prb_first_valid_seq(prb);
> > +
> > +	cookie = console_srcu_read_lock();
> > +	for_each_console_srcu(c) {
> > +		flags = console_srcu_read_flags(c);
> > +
> > +		if (flags & CON_NBCON) {
> > +			nbcon_seq_force(c, seq);
> > +		} else {
> > +			/*
> > +			 * This is an unsynchronized assignment. On
> > +			 * panic legacy consoles are only best effort.
> > +			 */
> 
> We should change this to something like:
> 
> 			/*
> 			 * This assigment is safe only when called under
> 			 * console_lock(). */
> 			 */
> 
> > +			c->seq = seq;
> > +		}
> > +	}
> > +	console_srcu_read_unlock(cookie);
> > +}
> > +
> >  /**
> >   * console_flush_on_panic - flush console content on panic
> >   * @mode: flush all messages in buffer or just the pending ones
> > @@ -3162,30 +3188,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
> >  	 */
> >  	console_may_schedule = 0;
> >  
> > -	if (mode == CONSOLE_REPLAY_ALL) {
> > -		struct console *c;
> > -		short flags;
> > -		int cookie;
> > -		u64 seq;
> > -
> > -		seq = prb_first_valid_seq(prb);
> > -
> > -		cookie = console_srcu_read_lock();
> > -		for_each_console_srcu(c) {
> > -			flags = console_srcu_read_flags(c);
> > -
> > -			if (flags & CON_NBCON) {
> > -				nbcon_seq_force(c, seq);
> > -			} else {
> > -				/*
> > -				 * This is an unsynchronized assignment. On
> > -				 * panic legacy consoles are only best effort.
> > -				 */
> > -				c->seq = seq;
> > -			}
> > -		}
> > -		console_srcu_read_unlock(cookie);
> > -	}
> > +	if (mode == CONSOLE_REPLAY_ALL)
> > +		console_rewind_all();
> >  
> >  	console_flush_all(false, &next_seq, &handover);
> >  }
> > @@ -4259,6 +4263,15 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> >  }
> >  EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
> >  
> > +/**
> > + * Dump the printk ring buffer directly to consoles
> > + */
> > +void dump_printk_buffer(void)
> 
> I would call this function console_replay_all(). IMHO, it better describes
> what it does.
> 
> > +{
> > +	console_lock();
> > +	console_rewind_all();
> 
> I would add a comment:
> 
> 	/* Consoles are flushed as part of console_unlock(). */
> 
> > +	console_unlock();
> > +}
> >  #endif
> 
> Best Regards,
> Petr

Thank you for the review comments. Will fix all these points in the
next version.

Regards,
Sreenath