From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1168022AbdDXJbD (ORCPT ); Mon, 24 Apr 2017 05:31:03 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:40371 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1167938AbdDXJa7 (ORCPT ); Mon, 24 Apr 2017 05:30:59 -0400 Date: Mon, 24 Apr 2017 11:30:51 +0200 From: Peter Zijlstra To: Byungchul Park Cc: mingo@kernel.org, tglx@linutronix.de, walken@google.com, boqun.feng@gmail.com, kirill@shutemov.name, linux-kernel@vger.kernel.org, linux-mm@kvack.org, iamjoonsoo.kim@lge.com, akpm@linux-foundation.org, willy@infradead.org, npiggin@gmail.com, kernel-team@lge.com Subject: Re: [PATCH v6 05/15] lockdep: Implement crossrelease feature Message-ID: <20170424093051.imizyhpifqf4t6bc@hirez.programming.kicks-ass.net> References: <1489479542-27030-1-git-send-email-byungchul.park@lge.com> <1489479542-27030-6-git-send-email-byungchul.park@lge.com> <20170419171954.tqp5tkxlsg4jp2xz@hirez.programming.kicks-ass.net> <20170424030412.GG21430@X58A-UD3R> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170424030412.GG21430@X58A-UD3R> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 24, 2017 at 12:04:12PM +0900, Byungchul Park wrote: > On Wed, Apr 19, 2017 at 07:19:54PM +0200, Peter Zijlstra wrote: > > > +/* > > > + * For crosslock. > > > + */ > > > +static int add_xlock(struct held_lock *hlock) > > > +{ > > > + struct cross_lock *xlock; > > > + unsigned int gen_id; > > > + > > > + if (!graph_lock()) > > > + return 0; > > > + > > > + xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock; > > > + > > > + gen_id = (unsigned int)atomic_inc_return(&cross_gen_id); > > > + xlock->hlock = *hlock; > > > + xlock->hlock.gen_id = gen_id; > > > + graph_unlock(); > > > > What does graph_lock protect here? > > Modifying xlock(not xhlock) instance should be protected with graph_lock. > Don't you think so? Ah, right you are. I think I got confused between our xhlock (local) array and the xlock instance thing. The latter needs protection to serialize concurrent acquires. > > > +static int commit_xhlocks(struct cross_lock *xlock) > > > +{ > > > + unsigned int cur = current->xhlock_idx; > > > + unsigned int i; > > > + > > > + if (!graph_lock()) > > > + return 0; > > > + > > > + for (i = cur - 1; !xhlock_same(i, cur); i--) { > > > + struct hist_lock *xhlock = &xhlock(i); > > > > *blink*, you mean this? > > > > for (i = 0; i < MAX_XHLOCKS_NR; i++) { > > struct hist_lock *xhlock = &xhlock(cur - i); > > I will change the loop to this form. > > > Except you seem to skip over the most recent element (@cur), why? > > Currently 'cur' points to the next *free* slot. Well, there's no such thing has a 'free' slot, its a _ring_ buffer. But: +static void add_xhlock(struct held_lock *hlock) +{ + unsigned int idx = current->xhlock_idx++; + struct hist_lock *xhlock = &xhlock(idx); Yes, I misread that. Then '0' has the oldest entry, which is slightly weird. Should we change that? > > > + > > > + if (!xhlock_used(xhlock)) > > > + break; > > > + > > > + if (before(xhlock->hlock.gen_id, xlock->hlock.gen_id)) > > > + break; > > > + > > > + if (same_context_xhlock(xhlock) && > > > + !commit_xhlock(xlock, xhlock)) > > > > return with graph_lock held? > > No. When commit_xhlock() returns 0, the lock was already unlocked. Please add a comment, because I completely missed that. That's at least 2 functions deeper.