From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758406AbcLAMuS (ORCPT ); Thu, 1 Dec 2016 07:50:18 -0500 Received: from mx2.suse.de ([195.135.220.15]:60760 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752147AbcLAMuR (ORCPT ); Thu, 1 Dec 2016 07:50:17 -0500 Date: Thu, 1 Dec 2016 13:50:13 +0100 From: Petr Mladek To: Sergey Senozhatsky 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 Subject: Re: [RFC][PATCHv4 4/6] printk: report lost messages in printk safe/nmi contexts Message-ID: <20161201125013.GN21230@pathway.suse.cz> References: <20161027154933.1211-1-sergey.senozhatsky@gmail.com> <20161027154933.1211-5-sergey.senozhatsky@gmail.com> <20161125110730.GH24103@pathway.suse.cz> <20161201021005.GE12039@jagdpanzerIV> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161201021005.GE12039@jagdpanzerIV> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 2016-12-01 11:10:42, Sergey Senozhatsky wrote: > 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. Sure, it will not overflow. But still, such a small hard coded buffer size looks too hacky to me. > > 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. Yup, it might be an option and sounds good. Anyway, I would use printk_deferred() to print the warnings about lost messages. It is perfectly fine and you will not need the hard coded temporary buffer. Best regards, Petr