From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752505AbbJTMS6 (ORCPT ); Tue, 20 Oct 2015 08:18:58 -0400 Received: from casper.infradead.org ([85.118.1.10]:54818 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752118AbbJTMS4 (ORCPT ); Tue, 20 Oct 2015 08:18:56 -0400 Date: Tue, 20 Oct 2015 14:18:53 +0200 From: Peter Zijlstra To: j.glisse@gmail.com Cc: linux-kernel@vger.kernel.org, =?iso-8859-1?B?Suly9G1l?= Glisse , Ingo Molnar , Sasha Levin Subject: Re: [PATCH] locking/lockdep: Fix expected depth value in __lock_release() Message-ID: <20151020121853.GD17308@twins.programming.kicks-ass.net> References: <1445283028-7865-1-git-send-email-j.glisse@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1445283028-7865-1-git-send-email-j.glisse@gmail.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 19, 2015 at 03:30:28PM -0400, j.glisse@gmail.com wrote: > From: Jérôme Glisse > > In __lock_release() we are removing one entry from the stack and > rebuilding the hash chain by re-adding entry above the entry we > just removed. If the entry removed was between 2 entry of same > class then this 2 entry might be coalesced into one single entry > which in turns means that the lockdep_depth value will not be > incremented and thus the expected lockdep_depth value after this > operation will be wrong triggering an unjustified WARN_ONCE() at > the end of __lock_release(). This is the nest_lock stuff, right? Where it checks: if (hlock->class_idx == class_idx && nest_lock) { ... return 1; } What code did you find that triggered this? That is, what code is taking nested locks with other locks in the middle? (Not wrong per-se, just curious how that would come about). > This patch adjust the expect depth value by decrementing it if > what was previously 2 entry inside the stack are coalesced into > only one entry. Would it not make more sense to scan the entire hlock stack on __lock_acquire() and avoid getting collapsible entries in the first place? Something like so... --- kernel/locking/lockdep.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 4e49cc4c9952..6fcd98b86e7b 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3125,15 +3125,21 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, class_idx = class - lock_classes + 1; - if (depth) { - hlock = curr->held_locks + depth - 1; - if (hlock->class_idx == class_idx && nest_lock) { - if (hlock->references) - hlock->references++; - else - hlock->references = 2; + if (depth && nest_lock) { + int i; - return 1; + for (i = depth; i; --i) { + hlock = curr->held_locks + i - 1; + if (hlock->class_idx == class_idx && + hlock->nest_lock == nest_lock) { + + if (hlock->references) + hlock->references++; + else + hlock->references = 2; + + return 1; + } } }