Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752465AbdHKIDY (ORCPT ); Fri, 11 Aug 2017 04:03:24 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:37165 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751559AbdHKIDT (ORCPT ); Fri, 11 Aug 2017 04:03:19 -0400 Date: Fri, 11 Aug 2017 16:03:29 +0800 From: Boqun Feng To: Byungchul Park Cc: peterz@infradead.org, mingo@kernel.org, tglx@linutronix.de, walken@google.com, kirill@shutemov.name, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, willy@infradead.org, npiggin@gmail.com, kernel-team@lge.com Subject: Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite Message-ID: <20170811080329.3ehu7pp7lcm62ji6@tardis> References: <1502089981-21272-1-git-send-email-byungchul.park@lge.com> <1502089981-21272-7-git-send-email-byungchul.park@lge.com> <20170810115922.kegrfeg6xz7mgpj4@tardis> <016b01d311d1$d02acfa0$70806ee0$@lge.com> <20170810125133.2poixhni4d5aqkpy@tardis> <20170810131737.skdyy4qcxlikbyeh@tardis> <20170811034328.GH20323@X58A-UD3R> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="53ffyjemh4ckify3" Content-Disposition: inline In-Reply-To: <20170811034328.GH20323@X58A-UD3R> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5774 Lines: 170 --53ffyjemh4ckify3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 11, 2017 at 12:43:28PM +0900, Byungchul Park wrote: > On Thu, Aug 10, 2017 at 09:17:37PM +0800, Boqun Feng wrote: > > > > > > @@ -4826,6 +4851,7 @@ static inline int depend_after(struct hel= d_lock > > > > > *hlock) > > > > > > * Check if the xhlock is valid, which would be false if, > > > > > > * > > > > > > * 1. Has not used after initializaion yet. > > > > > > + * 2. Got invalidated. > > > > > > * > > > > > > * Remind hist_lock is implemented as a ring buffer. > > > > > > */ > > > > > > @@ -4857,6 +4883,7 @@ static void add_xhlock(struct held_lock *= hlock) > > > > > > > > > > > > /* Initialize hist_lock's members */ > > > > > > xhlock->hlock =3D *hlock; > > > > > > + xhlock->hist_id =3D current->hist_id++; > > >=20 > > > Besides, is this code correct? Does this just make xhlock->hist_id > > > one-less-than the curr->hist_id, which cause the invalidation every t= ime > > > you do ring buffer unwinding? > > >=20 > > > Regards, > > > Boqun > > >=20 > >=20 > > So basically, I'm suggesting do this on top of your patch, there is also > > a fix in commit_xhlocks(), which I think you should swap the parameters > > in before(...), no matter using task_struct::hist_id or using > > task_struct::xhlock_idx as the timestamp. > >=20 > > Hope this could make my point more clear, and if I do miss something, > > please point it out, thanks ;-) >=20 > Sorry for mis-understanding. I like your patch. I think it works. >=20 Thanks for taking a look at it ;-) > Additionally.. See below.. >=20 > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 074872f016f8..886ba79bfc38 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -854,9 +854,6 @@ struct task_struct { > > unsigned int xhlock_idx; > > /* For restoring at history boundaries */ > > unsigned int xhlock_idx_hist[XHLOCK_NR]; > > - unsigned int hist_id; > > - /* For overwrite check at each context exit */ > > - unsigned int hist_id_save[XHLOCK_NR]; > > #endif > > =20 > > #ifdef CONFIG_UBSAN > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > > index 699fbeab1920..04c6c8d68e18 100644 > > --- a/kernel/locking/lockdep.c > > +++ b/kernel/locking/lockdep.c > > @@ -4752,10 +4752,8 @@ void crossrelease_hist_start(enum xhlock_context= _t c) > > { > > struct task_struct *cur =3D current; > > =20 > > - if (cur->xhlocks) { > > + if (cur->xhlocks) > > cur->xhlock_idx_hist[c] =3D cur->xhlock_idx; > > - cur->hist_id_save[c] =3D cur->hist_id; > > - } > > } > > =20 > > void crossrelease_hist_end(enum xhlock_context_t c) > > @@ -4769,7 +4767,7 @@ void crossrelease_hist_end(enum xhlock_context_t = c) > > cur->xhlock_idx =3D idx; > > =20 > > /* Check if the ring was overwritten. */ > > - if (h->hist_id !=3D cur->hist_id_save[c]) > > + if (h->hist_id !=3D idx) > > invalidate_xhlock(h); > > } > > } > > @@ -4849,7 +4847,7 @@ static void add_xhlock(struct held_lock *hlock) > > =20 > > /* Initialize hist_lock's members */ > > xhlock->hlock =3D *hlock; > > - xhlock->hist_id =3D current->hist_id++; > > + xhlock->hist_id =3D idx; > > =20 > > xhlock->trace.nr_entries =3D 0; > > xhlock->trace.max_entries =3D MAX_XHLOCK_TRACE_ENTRIES; > > @@ -5005,7 +5003,7 @@ static int commit_xhlock(struct cross_lock *xlock= , struct hist_lock *xhlock) > > static void commit_xhlocks(struct cross_lock *xlock) > > { > > unsigned int cur =3D current->xhlock_idx; > > - unsigned int prev_hist_id =3D xhlock(cur).hist_id; > > + unsigned int prev_hist_id =3D cur + 1; >=20 > I should have named it another. Could you suggest a better one? >=20 I think "prev" is fine, because I thought the "previous" means the xhlock item we visit _previously_. > > unsigned int i; > > =20 > > if (!graph_lock()) > > @@ -5030,7 +5028,7 @@ static void commit_xhlocks(struct cross_lock *xlo= ck) > > * hist_id than the following one, which is impossible > > * otherwise. >=20 > Or we need to modify the comment so that the word 'prev' does not make > readers confused. It was my mistake. >=20 I think the comment needs some help, but before you do it, could you have another look at what Peter proposed previously? Note you have a same_context_xhlock() check in the commit_xhlocks(), so the your previous overwrite case actually could be detected, I think. However, one thing may not be detected is this case: ppppppppppppppppppppppppppppppppppwwwwwwww wrapped > wwwwwww where p: process and w: worker. , because p and w are in the same task_irq_context(). I discussed this with Peter yesterday, and he has a good idea: unconditionally do a reset on the ring buffer whenever we do a crossrelease_hist_end(XHLOCK_PROC). Basically it means we empty the lock history whenever we finished a worker function in a worker thread or we are about to return to userspace after we finish the syscall. This could further save some memory and so I think this may be better than my approach. How does this sound to you? Regards, Boqun > Thanks, > Byungchul >=20 --53ffyjemh4ckify3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlmNZM0ACgkQSXnow7UH +rjEMwf9HGZP8GOdd7bEURaNvj/0wTHduSA86LpdlgF87tlDVgb7ATJKI4RGhNaW VUH1pf3UZKX2D0zIvZGSMUl4OeQ++HR+R2HPISc+3NFaHB+Gyxes/e2HbdLUnMZ+ 3ZGahEQvJindzYBIg7Y3mWU5jZ+sPWrUopIpyscFpVYKcHVlNqCTmqZDpFOwunYT CzMo4ZytgC6EXoDBtVl3HfmsV4iy/9FFX3p5HLC9+Zu4wolbp92/K8j+5RNw3rew 3w9Sd2gz7LLVR23PGgo5C/MvglntEjsqvqi2SY/eAKhRuVzXn9O8/U6Cv8c0JHwV bwLNBqIiH/BwJDou6al3Xxhb1prQ0w== =Wg2V -----END PGP SIGNATURE----- --53ffyjemh4ckify3--