Re: rw_enter_diag() vs WITNESS
On Tue, Feb 16, 2021 at 10:07:27AM +0100, Martin Pieuchot wrote:
> On 15/02/21(Mon) 16:58, Visa Hankala wrote:
> > On Mon, Feb 15, 2021 at 11:37:45AM +0100, Martin Pieuchot wrote:
> > > Diagnostic function rw_enter_diag() should be called before
> > > WITNESS_CHECKORDER() to have proper locking/debugging information.
> > >
> > > In the case of 'locking against myself' it is currently impossible
> > > to know where the lock has been previously acquired. Diff below fixes
> > > that, ok?
> >
> > Based on this description alone, it is not clear to me what exactly
> > gets solved. Doesn't the code reach rw_enter_diag() inside the loop
> > when trying to recurse on the rwlock?
>
> It does but before reaching WITNESS_CHECKORDER(). So if a panic is
> triggered "show all locks" will point to the previous place where the
> same lock has been acquired.
> Currently it points to the place triggering the panic which doesn't
> help figuring the problem.
Something is not right. "show all locks" should show the locks that are
currently being held by threads or CPUs. WITNESS_CHECKORDER() should
have no effect on those locks. Only WITNESS_LOCK() or WITNESS_UNLOCK()
should change the locked state.
So if there is an rwlock recursion, "show all locks", or "show locks",
should display where the thread did the initial rwlock acquisition (when
kern.witness.locktrace is enabled), and the panic's stack trace should
show where it attempted the second acquisition.
>
> > Does this change have implications with (panicstr || db_active)?
>
> Indeed, updated diff below.
>
> Index: kern/kern_rwlock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_rwlock.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 kern_rwlock.c
> --- kern/kern_rwlock.c8 Feb 2021 08:18:45 - 1.47
> +++ kern/kern_rwlock.c16 Feb 2021 09:04:04 -
> @@ -167,6 +167,9 @@ rw_exit_write(struct rwlock *rwl)
> static void
> rw_enter_diag(struct rwlock *rwl, int flags)
> {
> + if (panicstr || db_active)
> + return;
> +
> switch (flags & RW_OPMASK) {
> case RW_WRITE:
> case RW_READ:
> @@ -237,7 +240,11 @@ rw_enter(struct rwlock *rwl, int flags)
> int error, prio;
> #ifdef WITNESS
> int lop_flags;
> +#endif
> +
> + rw_enter_diag(rwl, flags);
>
> +#ifdef WITNESS
> lop_flags = LOP_NEWORDER;
> if (flags & RW_WRITE)
> lop_flags |= LOP_EXCLUSIVE;
> @@ -270,8 +277,6 @@ retry:
> continue;
> }
> #endif
> -
> - rw_enter_diag(rwl, flags);
>
> if (flags & RW_NOSLEEP)
> return (EBUSY);
>
Re: rw_enter_diag() vs WITNESS
On 15/02/21(Mon) 16:58, Visa Hankala wrote:
> On Mon, Feb 15, 2021 at 11:37:45AM +0100, Martin Pieuchot wrote:
> > Diagnostic function rw_enter_diag() should be called before
> > WITNESS_CHECKORDER() to have proper locking/debugging information.
> >
> > In the case of 'locking against myself' it is currently impossible
> > to know where the lock has been previously acquired. Diff below fixes
> > that, ok?
>
> Based on this description alone, it is not clear to me what exactly
> gets solved. Doesn't the code reach rw_enter_diag() inside the loop
> when trying to recurse on the rwlock?
It does but before reaching WITNESS_CHECKORDER(). So if a panic is
triggered "show all locks" will point to the previous place where the
same lock has been acquired.
Currently it points to the place triggering the panic which doesn't
help figuring the problem.
> Does this change have implications with (panicstr || db_active)?
Indeed, updated diff below.
Index: kern/kern_rwlock.c
===
RCS file: /cvs/src/sys/kern/kern_rwlock.c,v
retrieving revision 1.47
diff -u -p -r1.47 kern_rwlock.c
--- kern/kern_rwlock.c 8 Feb 2021 08:18:45 - 1.47
+++ kern/kern_rwlock.c 16 Feb 2021 09:04:04 -
@@ -167,6 +167,9 @@ rw_exit_write(struct rwlock *rwl)
static void
rw_enter_diag(struct rwlock *rwl, int flags)
{
+ if (panicstr || db_active)
+ return;
+
switch (flags & RW_OPMASK) {
case RW_WRITE:
case RW_READ:
@@ -237,7 +240,11 @@ rw_enter(struct rwlock *rwl, int flags)
int error, prio;
#ifdef WITNESS
int lop_flags;
+#endif
+
+ rw_enter_diag(rwl, flags);
+#ifdef WITNESS
lop_flags = LOP_NEWORDER;
if (flags & RW_WRITE)
lop_flags |= LOP_EXCLUSIVE;
@@ -270,8 +277,6 @@ retry:
continue;
}
#endif
-
- rw_enter_diag(rwl, flags);
if (flags & RW_NOSLEEP)
return (EBUSY);
Re: rw_enter_diag() vs WITNESS
On Mon, Feb 15, 2021 at 11:37:45AM +0100, Martin Pieuchot wrote: > Diagnostic function rw_enter_diag() should be called before > WITNESS_CHECKORDER() to have proper locking/debugging information. > > In the case of 'locking against myself' it is currently impossible > to know where the lock has been previously acquired. Diff below fixes > that, ok? Based on this description alone, it is not clear to me what exactly gets solved. Doesn't the code reach rw_enter_diag() inside the loop when trying to recurse on the rwlock? Does this change have implications with (panicstr || db_active)? > Index: kern/kern_rwlock.c > === > RCS file: /cvs/src/sys/kern/kern_rwlock.c,v > retrieving revision 1.47 > diff -u -p -r1.47 kern_rwlock.c > --- kern/kern_rwlock.c8 Feb 2021 08:18:45 - 1.47 > +++ kern/kern_rwlock.c15 Feb 2021 10:32:57 - > @@ -237,7 +237,11 @@ rw_enter(struct rwlock *rwl, int flags) > int error, prio; > #ifdef WITNESS > int lop_flags; > +#endif > + > + rw_enter_diag(rwl, flags); > > +#ifdef WITNESS > lop_flags = LOP_NEWORDER; > if (flags & RW_WRITE) > lop_flags |= LOP_EXCLUSIVE; > @@ -270,8 +274,6 @@ retry: > continue; > } > #endif > - > - rw_enter_diag(rwl, flags); > > if (flags & RW_NOSLEEP) > return (EBUSY); >
rw_enter_diag() vs WITNESS
Diagnostic function rw_enter_diag() should be called before WITNESS_CHECKORDER() to have proper locking/debugging information. In the case of 'locking against myself' it is currently impossible to know where the lock has been previously acquired. Diff below fixes that, ok? Index: kern/kern_rwlock.c === RCS file: /cvs/src/sys/kern/kern_rwlock.c,v retrieving revision 1.47 diff -u -p -r1.47 kern_rwlock.c --- kern/kern_rwlock.c 8 Feb 2021 08:18:45 - 1.47 +++ kern/kern_rwlock.c 15 Feb 2021 10:32:57 - @@ -237,7 +237,11 @@ rw_enter(struct rwlock *rwl, int flags) int error, prio; #ifdef WITNESS int lop_flags; +#endif + + rw_enter_diag(rwl, flags); +#ifdef WITNESS lop_flags = LOP_NEWORDER; if (flags & RW_WRITE) lop_flags |= LOP_EXCLUSIVE; @@ -270,8 +274,6 @@ retry: continue; } #endif - - rw_enter_diag(rwl, flags); if (flags & RW_NOSLEEP) return (EBUSY);
