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.2 required=3.0 tests=BAYES_00, BUG6152_INVALID_DATE_TZ_ABSURD,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,INVALID_DATE_TZ_ABSURD, 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 B828EC433FE for ; Sun, 6 Dec 2020 20:24:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7A0C02310E for ; Sun, 6 Dec 2020 20:24:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727913AbgLFUYm (ORCPT ); Sun, 6 Dec 2020 15:24:42 -0500 Received: from Galois.linutronix.de ([193.142.43.55]:59934 "EHLO galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726731AbgLFUYm (ORCPT ); Sun, 6 Dec 2020 15:24:42 -0500 From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1607286240; h=from:from:reply-to:subject:subject: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=JTrbLxSVsa+AJjMwFCvM3YrglsqeqmRd6O81SfPFnrY=; b=D762acncMNFT4eJbtzjA0Nb0CFGslfEYsX7pbMjZ87qVVBpH1SgenykDfkBbkhEFO3eY7C SnGggmLVnd2Z24lgJv/bsjYFX2CCHZruFYHnrfc4wk+WksBh4WPc/3xWJ3VfF+E1wDJUfT 1siFxsqaV7zuyftTCOxLHwyxX5qMyTVkcyZbOD724ecSwtT1pJUgjO98uB+khyPvuWz+JV iNQ/cCDTFDnSV3tJPvxEWlzd1xbWel4/IFZjwExVxom2MInIdKx0tR3pyaIY2isoXZV2lF hXi9vUvUMSfNMgFYtIXZdNiZmbP0VtYp0SJWdl1GfIbbeVT4XU92XMox64hyUA== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1607286240; h=from:from:reply-to:subject:subject: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=JTrbLxSVsa+AJjMwFCvM3YrglsqeqmRd6O81SfPFnrY=; b=SqDCGd2gx88hR2ztINUYQhOdlUxxSr+Z1zTtv5YFOmH0xbqz7l0EyNueCpiPBjl9a0p17z lYTnkcTsAk+AXDCg== To: Petr Mladek Cc: Sergey Senozhatsky , Sergey Senozhatsky , Steven Rostedt , Linus Torvalds , Greg Kroah-Hartman , Peter Zijlstra , Thomas Gleixner , linux-kernel@vger.kernel.org Subject: Re: [PATCH next v2 2/3] printk: change @clear_seq to atomic64_t In-Reply-To: References: <20201201205341.3871-1-john.ogness@linutronix.de> <20201201205341.3871-3-john.ogness@linutronix.de> Date: Sun, 06 Dec 2020 21:29:59 +0106 Message-ID: <875z5eof8g.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2020-12-04, Petr Mladek wrote: > On Tue 2020-12-01 21:59:40, John Ogness wrote: >> Currently @clear_seq access is protected by @logbuf_lock. Once >> @logbuf_lock is removed some other form of synchronization will be >> required. Change the type of @clear_seq to atomic64_t to provide the >> synchronization. >> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> index fc5e3a7d6d89..e9018c4e1b66 100644 >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -3412,7 +3418,7 @@ EXPORT_SYMBOL_GPL(kmsg_dump_get_buffer); >> */ >> void kmsg_dump_rewind_nolock(struct kmsg_dumper *dumper) >> { >> - dumper->cur_seq = clear_seq; >> + dumper->cur_seq = atomic64_read(&clear_seq); > > Sigh, atomic64_read() uses a spin lock in the generic implementation > that is used on some architectures. > > Hmm, this seems to be the only location where the lock must not be > used. Yes, and it is read-only access. Perhaps atomic64_t is the wrong thing to use here. We could use a seqcount_latch and a shadow variable so that if a writer has been preempted, we can use the previous value. (Only kmsg_dump would need to use the lockless variant to read the value.) void clear_seq_set(u64 val) { spin_lock_irq(&clear_lock); raw_write_seqcount_latch(&clear_latch); clear_seq[0] = val; raw_write_seqcount_latch(&clear_latch); clear_seq[1] = val; spin_unlock_irq(&clear_lock); } u64 clear_seq_get_nolock(void) { unsigned int seq, idx; u64 val; do { seq = raw_read_seqcount_latch(&clear_latch); idx = seq & 0x1; val = clear_seq[idx]; } while (read_seqcount_latch_retry(&clear_latch, seq)); return val; } u64 clear_seq_get(void) { u64 val; spin_lock_irq(&clear_lock); val = clear_seq[0]; spin_unlock_irq(&clear_lock); return val; } > Alternative solution would to always fallback to the first_seq on > these architectures. Few people would complain when they see more > messages. We could always improve it when it causes problems. I am also OK with this solution. John Ogness