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=ham 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 D658AC433DB for ; Fri, 19 Feb 2021 13:31:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8573864EB7 for ; Fri, 19 Feb 2021 13:31:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230201AbhBSNbb (ORCPT ); Fri, 19 Feb 2021 08:31:31 -0500 Received: from mx2.suse.de ([195.135.220.15]:44626 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229967AbhBSNb2 (ORCPT ); Fri, 19 Feb 2021 08:31:28 -0500 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=1613741441; 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=aK2ILRLf8YuM5i5fQZRYdq2o3A8u9wye0ZbzUZCnAAA=; b=SBSSfvHHryGLCMHzhHX00YnE9GQ68eLmpmyGVvYoV5f2WVW5Wm/7H8AbOg6uIE7vaPQAxR 6kti20J9CziOcMjib+LCvoUXyeSnYl3eq9fHgmr7Bxe1aDZs9oB3k69gempQwVh+fy2sam kZXRU5M5mIAPMTLBI8CXpb35soai/AM= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 898C2ABAE; Fri, 19 Feb 2021 13:30:41 +0000 (UTC) Date: Fri, 19 Feb 2021 14:30:41 +0100 From: Petr Mladek To: John Ogness Cc: Sergey Senozhatsky , Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org Subject: Re: [PATCH printk-rework 08/14] printk: add syslog_lock Message-ID: References: <20210218081817.28849-1-john.ogness@linutronix.de> <20210218081817.28849-9-john.ogness@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210218081817.28849-9-john.ogness@linutronix.de> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 2021-02-18 09:18:11, John Ogness wrote: > The global variables @syslog_seq, @syslog_partial, @syslog_time > and write access to @clear_seq are protected by @logbuf_lock. > Once @logbuf_lock is removed, these variables will need their > own synchronization method. Introduce @syslog_lock for this > purpose. > > @syslog_lock is a raw_spin_lock for now. This simplifies the > transition to removing @logbuf_lock. Once @logbuf_lock and the > safe buffers are removed, @syslog_lock can change to spin_lock. > --- > kernel/printk/printk.c | 41 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 37 insertions(+), 4 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 20c21a25143d..401df370832b 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > +/* Return a consistent copy of @syslog_seq. */ > +static u64 read_syslog_seq_irq(void) > +{ > + u64 seq; > + > + raw_spin_lock_irq(&syslog_lock); > + seq = syslog_seq; > + raw_spin_unlock_irq(&syslog_lock); Is there any particular reason to disable interrupts here? It would make sense only when the lock could be taken in IRQ context. Then we would need to always disable interrupts when the lock is taken. And if it is taken in IRQ context, we would need to safe flags. I guess that you got confused because it is used in wait_event_interruptible(). The name is misleading. "interruptible" means wake_up_process() and not IRQ here. IMHO, we should remove _irq suffix from the lock operations and also from the function name. > + > + return seq; > +} > + > int do_syslog(int type, char __user *buf, int len, int source) > { > struct printk_info info; > @@ -1664,8 +1688,9 @@ int do_syslog(int type, char __user *buf, int len, int source) > return 0; > if (!access_ok(buf, len)) > return -EFAULT; > + > error = wait_event_interruptible(log_wait, > - prb_read_valid(prb, syslog_seq, NULL)); > + prb_read_valid(prb, read_syslog_seq_irq(), NULL)); > if (error) > return error; > error = syslog_print(buf, len); Otherwise, the patch looks good to me. Best Regards, Petr