On 13/11/17(Mon) 17:29, Visa Hankala wrote:
> On Mon, Nov 13, 2017 at 01:25:42PM +0100, Martin Pieuchot wrote:
> > I'd love is somebody could do the changes in the rwlock API to let us
> > check if we're holding a lock. Maybe this is already present in
> > witness(4), visa?
>
> witness(4) does maintain data about lock ownership. The patch below
> might do the trick for rwlocks without adding extra state tracking.
I wouldn't mind a solution that's always on. I'm afraid it will take
some time before we can enable WITNESS regularly.
> The patch also fixes the witness-side initialization of statically
> initialized locks, in functions witness_checkorder() and
> witness_lock(). The fix belongs to a separate commit though.
You should commit that.
Note that the diff below still includes a false positive. If the
current thread doesn't have the read-lock but another one has it,
then RW_READ will still be returned.
> Index: share/man/man9/rwlock.9
> ===================================================================
> RCS file: src/share/man/man9/rwlock.9,v
> retrieving revision 1.20
> diff -u -p -r1.20 rwlock.9
> --- share/man/man9/rwlock.9 30 Oct 2017 13:33:36 -0000 1.20
> +++ share/man/man9/rwlock.9 13 Nov 2017 17:05:51 -0000
> @@ -33,6 +33,7 @@
> .Nm rw_assert_anylock ,
> .Nm rw_assert_unlocked ,
> .Nm rw_status ,
> +.Nm rw_status_curproc ,
> .Nm RWLOCK_INITIALIZER ,
> .Nm rrw_init ,
> .Nm rrw_init_flags ,
> @@ -68,6 +69,8 @@
> .Fn rw_assert_unlocked "struct rwlock *rwl"
> .Ft int
> .Fn rw_status "struct rwlock *rwl"
> +.Ft int
> +.Fn rw_status_curproc "struct rwlock *rwl"
> .Fn RWLOCK_INITIALIZER "const char *name"
> .Ft void
> .Fn rrw_init "struct rrwlock *rrwl" "const char *name"
> @@ -181,7 +184,7 @@ functions check the status
> .Fa rwl ,
> panicking if it is not write-, read-, any-, or unlocked, respectively.
> .Pp
> -.Nm rw_status
> +.Fn rw_status
> returns the current state of the lock:
> .Pp
> .Bl -tag -width "RW_WRITE_OTHER" -offset indent -compact
> @@ -194,6 +197,22 @@ Lock is read locked.
> The current thread may be one of the threads that has it locked.
> .It 0
> Lock is not locked.
> +.El
> +.Pp
> +.Fn rw_status_curproc
> +returns the current state of the lock relative to the calling thread:
> +.Pp
> +.Bl -tag -width "RW_WRITE_OTHER" -offset indent -compact
> +.It Dv RW_WRITE
> +Lock is write locked by the calling thread.
> +.It Dv RW_READ
> +Lock is read locked.
> +The current thread may be one of the threads that has it locked.
> +If the kernel has been compiled with
> +.Xr witness 4 ,
> +the status is exact, and the lock is read locked by the current thread.
> +.It 0
> +Lock is not locked by the calling thread.
> .El
> .Pp
> A lock declaration may be initialised with the
> Index: sys/kern/kern_rwlock.c
> ===================================================================
> RCS file: src/sys/kern/kern_rwlock.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 kern_rwlock.c
> --- sys/kern/kern_rwlock.c 24 Oct 2017 08:53:15 -0000 1.32
> +++ sys/kern/kern_rwlock.c 13 Nov 2017 17:05:51 -0000
> @@ -325,6 +325,34 @@ rw_status(struct rwlock *rwl)
> return (0);
> }
>
> +int
> +rw_status_curproc(struct rwlock *rwl)
> +{
> + unsigned long owner;
> +#ifdef WITNESS
> + int status = witness_status(&rwl->rwl_lock_obj);
> +
> + if (status != -1) {
> + if (status & LA_XLOCKED)
> + return (RW_WRITE);
> + if (status & LA_SLOCKED)
> + return (RW_READ);
> + return (0);
> + }
> +#endif
> +
> + owner = rwl->rwl_owner;
> + if (owner & RWLOCK_WRLOCK) {
> + if (RW_PROC(curproc) == RW_PROC(owner))
> + return (RW_WRITE);
> + else
> + return (0);
> + }
> + if (owner)
> + return RW_READ;
> + return (0);
> +}
> +
> #ifdef DIAGNOSTIC
> void
> rw_assert_wrlock(struct rwlock *rwl)
> Index: sys/kern/subr_witness.c
> ===================================================================
> RCS file: src/sys/kern/subr_witness.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 subr_witness.c
> --- sys/kern/subr_witness.c 12 Aug 2017 03:13:23 -0000 1.4
> +++ sys/kern/subr_witness.c 13 Nov 2017 17:05:51 -0000
> @@ -744,8 +744,8 @@ witness_checkorder(struct lock_object *l
> struct witness *w, *w1;
> int i, j, s;
>
> - if (witness_cold || witness_watch < 1 || lock->lo_witness == NULL ||
> - panicstr != NULL)
> + if (witness_cold || witness_watch < 1 || panicstr != NULL ||
> + (lock->lo_flags & LO_WITNESS) == 0)
> return;
>
> w = lock->lo_witness;
> @@ -1078,8 +1078,8 @@ witness_lock(struct lock_object *lock, i
> struct witness *w;
> int s;
>
> - if (witness_cold || witness_watch == -1 || lock->lo_witness == NULL ||
> - panicstr != NULL)
> + if (witness_cold || witness_watch == -1 || panicstr != NULL ||
> + (lock->lo_flags & LO_WITNESS) == 0)
> return;
>
> w = lock->lo_witness;
> @@ -2004,6 +2004,42 @@ witness_assert(const struct lock_object
>
> }
> #endif /* INVARIANT_SUPPORT */
> +}
> +
> +int
> +witness_status(struct lock_object *lock)
> +{
> + struct lock_instance *instance;
> + struct lock_class *class;
> + int status = 0;
> +
> + if (witness_cold || witness_watch < 1 || panicstr != NULL ||
> + (lock->lo_flags & LO_WITNESS) == 0)
> + return -1;
> +
> + class = LOCK_CLASS(lock);
> + if ((class->lc_flags & LC_SLEEPLOCK) != 0)
> + instance = find_instance(curproc->p_sleeplocks, lock);
> + else if ((class->lc_flags & LC_SPINLOCK) != 0)
> + instance = find_instance(
> + witness_cpu[cpu_number()].wc_spinlocks, lock);
> + else
> + panic("lock (%s) %s is not sleep or spin!",
> + class->lc_name, lock->lo_name);
> +
> + if (instance != NULL) {
> + status |= LA_LOCKED;
> + if (instance->li_flags & LI_EXCLUSIVE)
> + status |= LA_XLOCKED;
> + else
> + status |= LA_SLOCKED;
> + if (instance->li_flags & LI_RECURSEMASK)
> + status |= LA_RECURSED;
> + else
> + status |= LA_NOTRECURSED;
> + }
> +
> + return (status);
> }
>
> static void
> Index: sys/sys/rwlock.h
> ===================================================================
> RCS file: src/sys/sys/rwlock.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 rwlock.h
> --- sys/sys/rwlock.h 12 Aug 2017 23:27:44 -0000 1.22
> +++ sys/sys/rwlock.h 13 Nov 2017 17:05:51 -0000
> @@ -170,6 +170,7 @@ void rw_assert_unlocked(struct rwlock *)
> int _rw_enter(struct rwlock *, int LOCK_FL_VARS);
> void _rw_exit(struct rwlock * LOCK_FL_VARS);
> int rw_status(struct rwlock *);
> +int rw_status_curproc(struct rwlock *);
>
> #define rw_enter(rwl, flags) _rw_enter(rwl, flags LOCK_FILE_LINE)
> #define rw_exit(rwl) _rw_exit(rwl LOCK_FILE_LINE)
> Index: sys/sys/systm.h
> ===================================================================
> RCS file: src/sys/sys/systm.h,v
> retrieving revision 1.135
> diff -u -p -r1.135 systm.h
> --- sys/sys/systm.h 13 Nov 2017 14:41:46 -0000 1.135
> +++ sys/sys/systm.h 13 Nov 2017 17:05:52 -0000
> @@ -323,7 +323,7 @@ do {
> \
>
> #define NET_ASSERT_LOCKED()
> \
> do { \
> - int _s = rw_status(&netlock); \
> + int _s = rw_status_curproc(&netlock); \
> if ((splassert_ctl > 0) && (_s != RW_WRITE && _s != RW_READ)) \
> splassert_fail(RW_READ, _s, __func__); \
> } while (0)
> Index: sys/sys/witness.h
> ===================================================================
> RCS file: src/sys/sys/witness.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 witness.h
> --- sys/sys/witness.h 20 Apr 2017 12:59:36 -0000 1.1
> +++ sys/sys/witness.h 13 Nov 2017 17:05:52 -0000
> @@ -88,6 +88,7 @@ int witness_line(struct lock_object *);
> void witness_norelease(struct lock_object *);
> void witness_releaseok(struct lock_object *);
> const char *witness_file(struct lock_object *);
> +int witness_status(struct lock_object *);
> void witness_thread_exit(struct proc *);
>
> #ifdef WITNESS
>
