From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757222AbcLACKl (ORCPT ); Wed, 30 Nov 2016 21:10:41 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:33703 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754510AbcLACKk (ORCPT ); Wed, 30 Nov 2016 21:10:40 -0500 Date: Thu, 1 Dec 2016 11:10:42 +0900 From: Sergey Senozhatsky To: Petr Mladek Cc: Sergey Senozhatsky , Andrew Morton , Jan Kara , Tejun Heo , Calvin Owens , Thomas Gleixner , Mel Gorman , Steven Rostedt , Ingo Molnar , Peter Zijlstra , Laura Abbott , Andy Lutomirski , Linus Torvalds , Kees Cook , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [RFC][PATCHv4 4/6] printk: report lost messages in printk safe/nmi contexts Message-ID: <20161201021005.GE12039@jagdpanzerIV> References: <20161027154933.1211-1-sergey.senozhatsky@gmail.com> <20161027154933.1211-5-sergey.senozhatsky@gmail.com> <20161125110730.GH24103@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161125110730.GH24103@pathway.suse.cz> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (11/25/16 12:07), Petr Mladek wrote: [..] > > +static void report_message_lost(atomic_t *num_lost, char *fmt) > > +{ > > + int lost = atomic_xchg(num_lost, 0); > > + > > + if (lost) { > > + char msg[56]; > > I would really like to avoid a hard coded buffer size. Such things > are likely to bite us in the future. why would scnprintf() overflow. > I thought about reshuffling a lot of logic, adding more wrappers, > ... But the solution might be easy in the end, see below. > > > + > > + scnprintf(msg, sizeof(msg), fmt, lost); > > + > > + printk_safe_flush_line(msg, strlen(msg)); > > This made my brain spin a lot. I wondered if it did what we wanted > and it was safe. > > On one hand, it is supposed to work because use exactly this > function in __printk_safe_flush() where you call this from. > > One question is if it does what we want in different contexts. > Let's look at it: > > 1. It calls printk_deferred() in NMI context. There is a risk > of a deadlock. But it is called only from > printk_safe_flush_on_panic() which is the last resort. Therefore > it does exactly what we want. > > 2. It calls printk()->printk_func()->vprintk_emit() in normal context. > It is what we want in normal context. > > 3. It calls printk()->printk_func()->v printk_safe() in printk_safe > context. This does not look correct. IMHO, this might happen > only printk_safe_flush_on_panic() and we want to use > printk_deferred() here as well. [..] > The easiest solution would be to simply call printk_deferred() > here. Everything will be deferred after the async printk() patchset > anyway. > > I would even use printk_deferred() in printk_safe_flush_line() > for each context. It is not optimal but it works very well > and it makes the code much more straightforward. yes, good point. we can call deferred printk for anything there; or replace that in_nmi() check with the `printk_safe_context != 0' one, and then route the message via printk or printk_deferred. [..] > > * Flush data from the associated per-CPU buffer. The function > > * can be called either via IRQ work or independently. > > @@ -147,6 +183,9 @@ static void __printk_safe_flush(struct irq_work *work) > > > > i = 0; > > more: > > + report_nmi_message_lost(); > > + report_safe_message_lost(); > > Please, move this at the end of this function. ok. -ss