Date:   Mon, 13 Feb 2023 13:49:35 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     Ingo Molnar <mingo@kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Waiman Long <longman@redhat.com>,
        Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH v3] locking/lockdep: add debug_show_all_lock_holders()
Message-ID: <Y+ox39WhgY/iaVsG@hirez.programming.kicks-ass.net>
References: <ed17797b-e732-0dd0-2b4e-dc293653c0ac@I-love.SAKURA.ne.jp>
 <Y+oY3Xd43nNnkDSB@hirez.programming.kicks-ass.net>
 <274adab4-9922-1586-7593-08d9db5479a1@I-love.SAKURA.ne.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <274adab4-9922-1586-7593-08d9db5479a1@I-love.SAKURA.ne.jp>
Precedence: bulk

On Mon, Feb 13, 2023 at 08:34:55PM +0900, Tetsuo Handa wrote:
> On 2023/02/13 20:02, Peter Zijlstra wrote:
> >> @@ -213,7 +213,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
> >>   unlock:
> >>  	rcu_read_unlock();
> >>  	if (hung_task_show_lock)
> >> -		debug_show_all_locks();
> >> +		debug_show_all_lock_holders();
> >>  
> >>  	if (hung_task_show_all_bt) {
> >>  		hung_task_show_all_bt = false;
> > 
> > This being the hung-task detector, which is mostly about sleeping locks.
> 
> Yes, the intent of this patch is to report tasks sleeping with locks held,
> for the cause of hung task is sometimes a deadlock.
> 
> >> +	rcu_read_lock();
> >> +	for_each_process_thread(g, p) {
> >> +		if (!p->lockdep_depth)
> >> +			continue;
> >> +		if (p == current && p->lockdep_depth == 1)
> >> +			continue;
> >> +		sched_show_task(p);
> > 
> > And sched_show_task() being an utter piece of crap that will basically
> > print garbage for anything that's running (it doesn't have much
> > options).
> > 
> > Should we try and do better? dump_cpu_task() prefers
> > trigger_single_cpu_backtrace(), which sends an interrupt in order to get
> > active registers for the CPU.
> 
> What is the intent of using trigger_single_cpu_backtrace() here?
> check_hung_uninterruptible_tasks() is calling trigger_all_cpu_backtrace()
> if sysctl_hung_task_all_cpu_backtrace is set.

Then have that also print the held locks for those tasks. And skip over
them again later.

> Locks held and kernel backtrace are helpful for describing deadlock
> situation, but registers values are not.

Register state is required to start the unwind. You can't unwind a
running task out of thin-air.

> What is important is that tasks which are not on CPUs are reported,
> for when a task is reported as hung, that task must be sleeping.
> Therefore, I think sched_show_task() is fine.

The backtraces generated by sched_show_task() for a running task are
absolutely worthless, might as well not print them.

And if I read your Changelog right, you explicitly wanted useful
backtraces for the running tasks -- such that you could see what they
were doing while holding the lock the other tasks were blocked on.

The only way to do that is to send an interrupt, the interrupt will have
the register state for the interrupted task -- including the stack
pointer. By virtue of running the interrupt handler we know the stack
won't shrink, so we can then safely traverse the stack starting from the
given stack pointer.