Message ID | 20221210135825.241167123@goodmis.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp1368839wrr; Sat, 10 Dec 2022 07:33:13 -0800 (PST) X-Google-Smtp-Source: AA0mqf502LUbU2uZfskg738avP9wifOlBZNEkD55TTDcApe3XyiZ20wUmMkT1yKWSDeKZdFql+6Z X-Received: by 2002:a17:906:86d9:b0:7c1:2a0f:55b1 with SMTP id j25-20020a17090686d900b007c12a0f55b1mr7652452ejy.14.1670680816733; Sat, 10 Dec 2022 06:00:16 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1670680816; cv=none; d=google.com; s=arc-20160816; b=DxoyzTJesurpbhs9HxsV9DSEXMshW2WuRbrkDwcrYJIGzR1aqZjR6bLueLd5yqTOma /AHgqJOcQFZu6+fRGEZfpUE09DSEXocsd1pdUhGO3/p9zgkMJIVB0cxUUTEtDSBQqH8x Vr8m9RNa0FCnom2IFT+1HHZa1cyWgA1N1sXIavjiYHBgYiesFBXTb6rxZMW7QmdBbSYp x0qKgDd6KTgS6v89WzAlzaIjIdIdm+C5EMeOIbLOCvzFy3DxqdaX6gGzlIJZT6A+NT2D MrF/dyUt2H4cNDc8a0Bf6OP/R+DV1VptkBo38Nhozple6KPelVc2g8BuouukZ0OF63XR cH/A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:references:subject:cc:to:from:date :user-agent:message-id; bh=G6d6VCt6LqFwGKmi07dpXs0U39zP7DBF/mXiB/dTDUM=; b=m4bMef1Yrrs1RWDBb4vCJvH1EefTCkECO610axQde4MUgu8GZNhPLY14lqEfH4np1T oHd1R3bUblsNKyoXtgncik2B8v8r/SPvAaLs0eIZ+hhBX3Hxeqs1ldXdDBZJqUCyCY2F FkT0G/Ql09xbo8qRY+UjlOHpSjwOis7fvMnYSPo4UpJZbU6PUQJbbQqra8ay1RO9/cVb 91U94juByTW3Olf9XrPQwSV0pfu+Xk1ctqs3F9OTX8uPm9L8dyH0HKOojohGtcdqIP8S mOKoKuWEZdCJIHviGYgkzeZkQMz2rrPUa/hcIkY5hPE2nSBnwfedfALM5FksYC7gpyhb Akug== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id qa30-20020a170907869e00b007c08e3b0e32si2254874ejc.414.2022.12.10.05.59.50; Sat, 10 Dec 2022 06:00:16 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229960AbiLJN6u (ORCPT <rfc822;jz.zhangjin@gmail.com> + 99 others); Sat, 10 Dec 2022 08:58:50 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34882 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229813AbiLJN61 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 10 Dec 2022 08:58:27 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ED1BC335 for <linux-kernel@vger.kernel.org>; Sat, 10 Dec 2022 05:58:26 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7C5C460C22 for <linux-kernel@vger.kernel.org>; Sat, 10 Dec 2022 13:58:26 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 55577C433F2; Sat, 10 Dec 2022 13:58:26 +0000 (UTC) Received: from rostedt by gandalf.local.home with local (Exim 4.96) (envelope-from <rostedt@goodmis.org>) id 1p40Mz-000kqq-1D; Sat, 10 Dec 2022 08:58:25 -0500 Message-ID: <20221210135825.241167123@goodmis.org> User-Agent: quilt/0.66 Date: Sat, 10 Dec 2022 08:58:03 -0500 From: Steven Rostedt <rostedt@goodmis.org> To: linux-kernel@vger.kernel.org Cc: Masami Hiramatsu <mhiramat@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, Karol Herbst <karolherbst@gmail.com>, Pekka Paalanen <ppaalanen@gmail.com>, Dave Hansen <dave.hansen@linux.intel.com>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>, "Paul E. McKenney" <paulmck@kernel.org> Subject: [for-next][PATCH 13/25] x86/mm/kmmio: Use rcu_read_lock_sched_notrace() References: <20221210135750.425719934@goodmis.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1751835808479363044?= X-GMAIL-MSGID: =?utf-8?q?1751835808479363044?= |
Series |
tracing: Updates for 6.2
|
|
Commit Message
Steven Rostedt
Dec. 10, 2022, 1:58 p.m. UTC
From: Steven Rostedt <rostedt@goodmis.org> The mmiotrace tracer is "special". The purpose is to help reverse engineer binary drivers by removing the memory allocated by the driver and when the driver goes to access it, a fault occurs, the mmiotracer will record what the driver was doing and then do the work on its behalf by single stepping through the process. But to achieve this ability, it must do some special things. One is to take the rcu_read_lock() when the fault occurs, and then release it in the breakpoint that is single stepping. This makes lockdep unhappy, as it changes the state of RCU from within an exception that is not contained in that exception, and we get a nasty splat from lockdep. Instead, switch to rcu_read_lock_sched_notrace() as the RCU sched variant has the same grace period as normal RCU. This is basically the same as rcu_read_lock() but does not make lockdep complain about it. Note, the preempt_disable() is still needed as it uses preempt_enable_no_resched(). Link: https://lore.kernel.org/linux-trace-kernel/20221209134144.04f33626@gandalf.local.home Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Karol Herbst <karolherbst@gmail.com> Cc: Pekka Paalanen <ppaalanen@gmail.com> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@kernel.org> Cc: Borislav Petkov <bp@alien8.de> Cc: "Paul E. McKenney" <paulmck@kernel.org> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> --- arch/x86/mm/kmmio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Comments
On Sat, Dec 10, 2022 at 08:58:03AM -0500, Steven Rostedt wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The mmiotrace tracer is "special". The purpose is to help reverse engineer > binary drivers by removing the memory allocated by the driver and when the > driver goes to access it, a fault occurs, the mmiotracer will record what > the driver was doing and then do the work on its behalf by single stepping > through the process. > > But to achieve this ability, it must do some special things. One is to > take the rcu_read_lock() when the fault occurs, and then release it in the > breakpoint that is single stepping. This makes lockdep unhappy, as it > changes the state of RCU from within an exception that is not contained in > that exception, and we get a nasty splat from lockdep. > > Instead, switch to rcu_read_lock_sched_notrace() as the RCU sched variant > has the same grace period as normal RCU. This is basically the same as > rcu_read_lock() but does not make lockdep complain about it. > > Note, the preempt_disable() is still needed as it uses preempt_enable_no_resched(). > > Link: https://lore.kernel.org/linux-trace-kernel/20221209134144.04f33626@gandalf.local.home > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Karol Herbst <karolherbst@gmail.com> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> Executable code can be the best form of comment. ;-) This does mess with preempt_count() redundantly, but the overhead from that should be way down in the noise. Acked-by: Paul E. McKenney <paulmck@kernel.org> > --- > arch/x86/mm/kmmio.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c > index edb486450158..853c49877c16 100644 > --- a/arch/x86/mm/kmmio.c > +++ b/arch/x86/mm/kmmio.c > @@ -254,7 +254,7 @@ int kmmio_handler(struct pt_regs *regs, unsigned long addr) > * again. > */ > preempt_disable(); > - rcu_read_lock(); > + rcu_read_lock_sched_notrace(); > > faultpage = get_kmmio_fault_page(page_base); > if (!faultpage) { > @@ -323,7 +323,7 @@ int kmmio_handler(struct pt_regs *regs, unsigned long addr) > return 1; /* fault handled */ > > no_kmmio: > - rcu_read_unlock(); > + rcu_read_unlock_sched_notrace(); > preempt_enable_no_resched(); > return ret; > } > @@ -363,7 +363,7 @@ static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs) > /* These were acquired in kmmio_handler(). */ > ctx->active--; > BUG_ON(ctx->active); > - rcu_read_unlock(); > + rcu_read_unlock_sched_notrace(); > preempt_enable_no_resched(); > > /* > -- > 2.35.1 > >
On Sat, 10 Dec 2022 09:47:53 -0800 "Paul E. McKenney" <paulmck@kernel.org> wrote: > > Note, the preempt_disable() is still needed as it uses preempt_enable_no_resched(). > > ... > Executable code can be the best form of comment. ;-) > > This does mess with preempt_count() redundantly, but the overhead from > that should be way down in the noise. I was going to remove it, but then I realized that it would be a functional change, as from the comment above, it uses "preempt_enable_no_resched(), which there is not a rcu_read_unlock_sched() variant. > > Acked-by: Paul E. McKenney <paulmck@kernel.org> Thanks! I'll add this to the commit. -- Steve
On Sat, Dec 10, 2022 at 01:34:25PM -0500, Steven Rostedt wrote: > On Sat, 10 Dec 2022 09:47:53 -0800 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > Note, the preempt_disable() is still needed as it uses preempt_enable_no_resched(). > > > > > ... > > > Executable code can be the best form of comment. ;-) > > > > This does mess with preempt_count() redundantly, but the overhead from > > that should be way down in the noise. > > I was going to remove it, but then I realized that it would be a functional > change, as from the comment above, it uses "preempt_enable_no_resched(), > which there is not a rcu_read_unlock_sched() variant. If this happens often enough, it might be worth adding something like rcu_read_unlock_sched_no_resched(), but we clearly are not there yet. Especially not with a name like that! ;-) Thanx, Paul > > Acked-by: Paul E. McKenney <paulmck@kernel.org> > > Thanks! I'll add this to the commit. > > -- Steve
On Sat, 10 Dec 2022 13:34:12 -0800 "Paul E. McKenney" <paulmck@kernel.org> wrote: > > I was going to remove it, but then I realized that it would be a functional > > change, as from the comment above, it uses "preempt_enable_no_resched(), > > which there is not a rcu_read_unlock_sched() variant. > > If this happens often enough, it might be worth adding something like > rcu_read_unlock_sched_no_resched(), but we clearly are not there yet. > Especially not with a name like that! ;-) Please don't ;-) This is only to handle the bizarre case that mmio tracing does. Remember, this tracer is only for those that want to reverse engineer a binary driver. It's not even SMP safe! When you enable it, it shuts down all but one CPU. This is actually the reason I worked so hard to keep it working with lockdep. The shutting down of CPUs has caught so many bugs in other parts of the kernel! ;-) Thus, anything that mmio tracer does, is considered niche, and not something to much care about. -- Steve
On Sat, Dec 10 2022 at 13:34, Steven Rostedt wrote: > On Sat, 10 Dec 2022 09:47:53 -0800 "Paul E. McKenney" <paulmck@kernel.org> wrote: >> This does mess with preempt_count() redundantly, but the overhead from >> that should be way down in the noise. > > I was going to remove it, but then I realized that it would be a functional > change, as from the comment above, it uses "preempt_enable_no_resched(), > which there is not a rcu_read_unlock_sched() variant. preempt_enable_no_resched() in this context is simply garbage. preempt_enable_no_resched() tries to avoid the overhead of checking whether rescheduling is due after decrementing preempt_count() because the code which it this claims to know that it is _not_ the outermost one which brings preempt count back to preemtible state. I concede that there are hot paths which actually can benefit, but this code has exactly _ZERO_ benefit from that. Taking that tracing exception and handling it is orders of magnitudes more expensive than a regular preempt_enable(). So just get rid of it and don't proliferate cargo cult programming. Thanks, tglx
On Sun, 11 Dec 2022 00:30:36 +0100 Thomas Gleixner <tglx@linutronix.de> wrote: > On Sat, Dec 10 2022 at 13:34, Steven Rostedt wrote: > > On Sat, 10 Dec 2022 09:47:53 -0800 "Paul E. McKenney" <paulmck@kernel.org> wrote: > >> This does mess with preempt_count() redundantly, but the overhead from > >> that should be way down in the noise. > > > > I was going to remove it, but then I realized that it would be a functional > > change, as from the comment above, it uses "preempt_enable_no_resched(), > > which there is not a rcu_read_unlock_sched() variant. > > preempt_enable_no_resched() in this context is simply garbage. > > preempt_enable_no_resched() tries to avoid the overhead of checking > whether rescheduling is due after decrementing preempt_count() because > the code which it this claims to know that it is _not_ the outermost one > which brings preempt count back to preemtible state. > > I concede that there are hot paths which actually can benefit, but this > code has exactly _ZERO_ benefit from that. Taking that tracing exception > and handling it is orders of magnitudes more expensive than a regular > preempt_enable(). > > So just get rid of it and don't proliferate cargo cult programming. > The point of the patch is to just fix the lockdep issue. I'm happy to remove that "no_resched" (I was planning to), but that would be a separate change, with a different purpose, and thus a separate patch. -- Steve
On Sat, Dec 10, 2022 at 05:32:27PM -0500, Steven Rostedt wrote: > On Sat, 10 Dec 2022 13:34:12 -0800 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > I was going to remove it, but then I realized that it would be a functional > > > change, as from the comment above, it uses "preempt_enable_no_resched(), > > > which there is not a rcu_read_unlock_sched() variant. > > > > If this happens often enough, it might be worth adding something like > > rcu_read_unlock_sched_no_resched(), but we clearly are not there yet. > > Especially not with a name like that! ;-) > > Please don't ;-) > > This is only to handle the bizarre case that mmio tracing does. Remember, > this tracer is only for those that want to reverse engineer a binary > driver. It's not even SMP safe! When you enable it, it shuts down all but > one CPU. This is actually the reason I worked so hard to keep it working > with lockdep. The shutting down of CPUs has caught so many bugs in other > parts of the kernel! ;-) > > Thus, anything that mmio tracer does, is considered niche, and not > something to much care about. Agreed, as I said, we are clearly not there yet. Thanx, Paul
On Sat, Dec 10 2022 at 18:55, Steven Rostedt wrote: > On Sun, 11 Dec 2022 00:30:36 +0100 > Thomas Gleixner <tglx@linutronix.de> wrote: >> I concede that there are hot paths which actually can benefit, but this >> code has exactly _ZERO_ benefit from that. Taking that tracing exception >> and handling it is orders of magnitudes more expensive than a regular >> preempt_enable(). >> >> So just get rid of it and don't proliferate cargo cult programming. >> > The point of the patch is to just fix the lockdep issue. I'm happy to > remove that "no_resched" (I was planning to), but that would be a separate > change, with a different purpose, and thus a separate patch. Right, but please make that part of the series. Thanks, tglx
On Mon, 12 Dec 2022 11:51:51 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:
> Right, but please make that part of the series.
I just pushed out a patch to do this.
https://lore.kernel.org/all/20221212103703.7129cc5d@gandalf.local.home/
Feel free to ack it.
Thanks,
-- Steve
diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c index edb486450158..853c49877c16 100644 --- a/arch/x86/mm/kmmio.c +++ b/arch/x86/mm/kmmio.c @@ -254,7 +254,7 @@ int kmmio_handler(struct pt_regs *regs, unsigned long addr) * again. */ preempt_disable(); - rcu_read_lock(); + rcu_read_lock_sched_notrace(); faultpage = get_kmmio_fault_page(page_base); if (!faultpage) { @@ -323,7 +323,7 @@ int kmmio_handler(struct pt_regs *regs, unsigned long addr) return 1; /* fault handled */ no_kmmio: - rcu_read_unlock(); + rcu_read_unlock_sched_notrace(); preempt_enable_no_resched(); return ret; } @@ -363,7 +363,7 @@ static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs) /* These were acquired in kmmio_handler(). */ ctx->active--; BUG_ON(ctx->active); - rcu_read_unlock(); + rcu_read_unlock_sched_notrace(); preempt_enable_no_resched(); /*