From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966468AbcLVRKR (ORCPT ); Thu, 22 Dec 2016 12:10:17 -0500 Received: from mx2.suse.de ([195.135.220.15]:59052 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762052AbcLVRKQ (ORCPT ); Thu, 22 Dec 2016 12:10:16 -0500 Date: Thu, 22 Dec 2016 18:10:12 +0100 From: Petr Mladek To: Sergey Senozhatsky Cc: Linus Torvalds , Andrew Morton , Jan Kara , Tejun Heo , Calvin Owens , Steven Rostedt , Ingo Molnar , Peter Zijlstra , Andy Lutomirski , Peter Hurley , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCHv6 6/7] printk: use printk_safe buffers in printk Message-ID: <20161222171012.GC14894@pathway.suse.cz> References: <20161221143605.2272-1-sergey.senozhatsky@gmail.com> <20161221143605.2272-7-sergey.senozhatsky@gmail.com> <20161222053119.GE644@jagdpanzerIV.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161222053119.GE644@jagdpanzerIV.localdomain> 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-22 14:31:19, Sergey Senozhatsky wrote: > Hello, > > On (12/21/16 23:36), Sergey Senozhatsky wrote: > > Use printk_safe per-CPU buffers in printk recursion-prone blocks: > > -- around logbuf_lock protected sections in vprintk_emit() and > > console_unlock() > > -- around down_trylock_console_sem() and up_console_sem() > > > > Note that this solution addresses deadlocks caused by printk() > > recursive calls only. That is vprintk_emit() and console_unlock(). > > several questions. Good questions! > so my plan was to introduce printk-safe and to switch vprintk_emit() > and console_sem related functions (like console_unlock(), etc.) to > printk-safe first. and switch the remaining logbuf_lock users, like > devkmsg_open()/syslog_print()/etc, in a followup, pretty much > mechanical "find logbuf_lock - add printk_safe", patch. but that > followup patch is bigger than I expected (still mechanical tho); > so I want to re-group. > > there are > 9 raw_spin_lock_irq(&logbuf_lock) > 7 raw_spin_lock_irqsave(&logbuf_lock, flags) > and > 12 raw_spin_lock_irq(&logbuf_lock) > 8 raw_spin_unlock_irqrestore(&logbuf_lock, flags) I see. > wrapping each one of them in printk_safe_enter()/printk_safe_enter_irq() > and printk_safe_exit()/printk_safe_exit_irq() is a bit boring. so I have > several options: one of them is to add printk_safe_{enter,exit}_irq() and, > along with it, a bunch of help macros (to printk.c): > > (questions below) > > /* > * Helper macros to lock/unlock logbuf_lock in deadlock safe > * manner (logbuf_lock may spin_dump() in lock/unlock). > */ > #define lock_logbuf(flags) \ > do { \ > printk_safe_enter(flags); \ > raw_spin_lock(&logbuf_lock); \ > } while (0) There are many callers. I think that such wrappers make sense. I would only like to keep naming scheme similar to the classic locks. I mean: printk_safe_enter_irq() printk_safe_exit_irq() printk_safe_enter_irqsave(flags) printk_safe_exit_irqrestore(flags) and logbuf_lock_irq() logbuf_unlock_irq() logbuf_lock_irqsave(flags) logbuf_lock_irqrestore(flags) I actually like this change. It makes it clear that the operation has a side effect (disables/enables irq) which was not visible from the original name. Well, I wonder how many times we need to call printk_save_enter/exit standalone (ouside these macros). The question is if we really need all the variants of printk_safe_enter()/exit(). Alternative solution would be to handle only the printk_context in pritnk_safe_enter() and make sure that it is called with IRQs disabled. I mean to define only __printk_safe_enter()/exit() and do: #define logbuf_lock_irqsave(flags) \ do { \ local_irq_save(flags) \ __printk_safe_enter(); \ raw_spin_lock(&logbuf_lock); \ } while (0) > -- the approach > another solution? switch those raw_spin_{lock,unlock}_irq to irqsave/irqrestore > (?) and use the existing printk_safe_enter()/printk_safe_exit(), > so *_irq() versions of lock_logbuf/printk_safe macros will not be needed? This is bad idea. We should keep the information where the flags need to be stored and where not. It helps to understand in which context the function might be called. Best Regards, Petr PS: I still think if we could come with a better name than printk_safe() but I cannot find one.