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;
Feedback-ID: iad51458e:Fastmail
Date:   Fri, 19 Aug 2022 22:18:37 -0700
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Vincent Whitchurch <vincent.whitchurch@axis.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        kernel <kernel@axis.com>, Waiman Long <longman@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] lockdep: Panic on warning if panic_on_warn is set
Message-ID: <YwBurQWDUSZPfyqm@boqun-archlinux>
References: <20220818114259.2203505-1-vincent.whitchurch@axis.com>
 <Yv6z3SlfpmN+ZeV1@boqun-archlinux>
 <Yv9tLO2nbRBVyXgl@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yv9tLO2nbRBVyXgl@axis.com>
Precedence: bulk

On Fri, Aug 19, 2022 at 12:59:56PM +0200, Vincent Whitchurch wrote:
> On Thu, Aug 18, 2022 at 11:49:17PM +0200, Boqun Feng wrote:
> > On Thu, Aug 18, 2022 at 01:42:58PM +0200, Vincent Whitchurch wrote:
> > > There does not seem to be any way to get the system to panic if a
> > > lockdep warning is emitted, since those warnings don't use the normal
> > > WARN() infrastructure.  Panicking on any lockdep warning can be
> > > desirable when the kernel is being run in a controlled environment
> > > solely for the purpose of testing.  Make lockdep respect panic_on_warn
> > > to allow this, similar to KASAN and others.
> > > 
> > 
> > I'm not completely against this, but could you explain why you want to
> > panic on lockdep warning? I assume you want to have a kdump so that you
> > can understand the lock bugs closely? But lockdep discovers lock issue
> > possiblity, so it's not an after-the-fact detector. In other words, when
> > lockdep warns, the deadlock cases don't happen in the meanwhile. And
> > also lockdep tries very hard to print useful information to locate the
> > issues.
> 
> I'm not trying to obtain a kdump in this case.  I test device drivers
> under UML[0] and I want to make the tests stop and fail immediately if
> the driver triggers any kind of problem which results in splats in the
> log.  I achieve this using panic_on_warn, panic_on_taint, and oops=panic
> which result in a panic and an error exit code from UML.
> 
> [0] https://lore.kernel.org/lkml/20220311162445.346685-1-vincent.whitchurch@axis.com/
> 
> For lockdep, without this patch, I would be forced to parse the logs
> after each test to determine if the test trigger a lockdep splat or not.
> 

In that case, would a standard line with every lockdep warning help? For
example:

[...] A LOCKDEP issue detected.

Two reasons I don't think making lockdep warning as panic is a good
idea:

* We don't know what other CIs expect, given today lockdep doesn't panic
  with panic_on_warn, this patch is a change of behaviors to them, and
  it may break their setups/scripts.

* As I said, lockdep warnings are different than other warnings, and
  panicking doesn't provide more information for debugging.

So I think an extra line helping scripts to parse may be better.

Work for you?

Regards,
Boqun

> > This patch add lockdep_panic() to a few places, and it's a pain for
> > maintaining. So why do you want to panic on lockdep warning?
> 
> It's adding the call to a lot of places since there is no existing
> common function indicating the end of a lockdep warning.  I can move the
> already duplicated dump_stack() calls into the new function too so that
> some code is removed.  The "stack backtrace" could possible be
> consolidated too, but one of the call sites uses printk instead of
> pr_warn so I wasn't sure if it was OK to change that to a warn too.