Re: un-KERNEL_LOCK() TCP/UDP input & co

2017-11-14 Thread Visa Hankala
On Tue, Nov 14, 2017 at 10:37:16AM +0100, Martin Pieuchot wrote:
> 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.

That's true if witness(4) has not been enabled.



Re: un-KERNEL_LOCK() TCP/UDP input & co

2017-11-14 Thread Martin Pieuchot
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 -  1.20
> +++ share/man/man9/rwlock.9   13 Nov 2017 17:05:51 -
> @@ -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.c24 Oct 2017 08:53:15 -  1.32
> +++ sys/kern/kern_rwlock.c13 Nov 2017 17:05:51 -
> @@ -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_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 -  1.4
> +++ sys/kern/subr_witness.c   13 Nov 2017 17:05:51 -
> @@ -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 ||

Re: un-KERNEL_LOCK() TCP/UDP input & co

2017-11-13 Thread Visa Hankala
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.

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.

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 -  1.20
+++ share/man/man9/rwlock.9 13 Nov 2017 17:05:51 -
@@ -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 -  1.32
+++ sys/kern/kern_rwlock.c  13 Nov 2017 17:05:51 -
@@ -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_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 -  1.4
+++ sys/kern/subr_witness.c 13 Nov 2017 17:05:51 -
@@ -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);

Re: un-KERNEL_LOCK() TCP/UDP input & co

2017-11-13 Thread Alexandr Nedvedicky
Hello,

> > So I'd like to know, what's the plan for NET_ASSERT_LOCKED() macro?
> > 
> > a) are we going to relax it, so it will test if the net_lock is
> > locked?
> 
> Yep, that's already done.

cool, thanks a lot. I've just noticed the change is there while ago, while
reading another thread here [1] (assertion "_kernel_lock_held()" failed,
uipc_socket2.c: ipsec)


thanks and
regards
sasha

[1] https://marc.info/?l=openbsd-tech=151052226224227=2 



Re: un-KERNEL_LOCK() TCP/UDP input & co

2017-11-13 Thread Martin Pieuchot
On 13/11/17(Mon) 12:14, Alexandr Nedvedicky wrote:
> your change looks good to me. Though I have a comment/question to your diff.
> 
> > Index: net/if_vxlan.c
> > ===
> > RCS file: /cvs/src/sys/net/if_vxlan.c,v
> > retrieving revision 1.63
> > diff -u -p -r1.63 if_vxlan.c
> > --- net/if_vxlan.c  25 Oct 2017 09:24:09 -  1.63
> > +++ net/if_vxlan.c  8 Nov 2017 14:49:58 -
> > @@ -611,6 +611,7 @@ vxlan_lookup(struct mbuf *m, struct udph
> > vni = VXLAN_VNI_UNSET;
> > }
> >  
> > +   NET_ASSERT_LOCKED();
> > /* First search for a vxlan(4) interface with the packet's VNI */
> > LIST_FOREACH(sc, _tagh[VXLAN_TAGHASH(vni)], sc_entry) {
> > if ((uh->uh_dport == sc->sc_dstport) &&
> > Index: net/pf.c
> 
> I think we are approaching point, which requires us to revisit
> NET_ASSERT_LOCKED(). The assert currently tests caller is writer
> on net_lock.

Since last week NET_ASSERT_LOCKED() is checking if the calling thread
owns the (write) lock or if it is hold by at least one reader.

Without changing our rwlock implementation there's no way to check if
the calling thread is holding a read lock.  That would be definitively
useful.

> Since we are going to 'soften' the NET_LOCK() to R-lock for
> some tasks/threads the NET_ASSERT_LOCKED() will become invalid
> (false positive) assertion for functions, which are being grabbed
> occasionally as readers and other times as writers (?typically in
> local outbound path?). I've seen such smoke already in my diffs
> I'm working on currently.
> 
> So I'd like to know, what's the plan for NET_ASSERT_LOCKED() macro?
> 
>   a) are we going to relax it, so it will test if the net_lock is
>   locked?

Yep, that's already done.

>   b) are we going to keep it, but a new 'soft' assert will be introduced
>   e.g. NET_ASSERT_ALOCKED() (A for any lock)?

We should turn NET_ASSERT_LOCKED() macros that require a write lock to
NET_ASSERT_WLOCKED().  But that's mostly in the ioctl(2) path.

>   c) add parameter to current NET_ASSERT_LOCKED() stating desired
>   lock level: 0 - unlocked, 1 - reader, 2 - writer

I don't care how the API looks like, I'd just argue for coherency
between all or locks.

> As I've said the patch looks good to me as-is and should go in. I just
> would like to hear some greater plan for NET_ASSERT_LOCKED(). Perhaps
> we should go for it before we will be further playing with parallel
> softnet.

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?



Re: un-KERNEL_LOCK() TCP/UDP input & co

2017-11-13 Thread Alexandr Nedvedicky
Hello,

your change looks good to me. Though I have a comment/question to your diff.


> Index: net/if_vxlan.c
> ===
> RCS file: /cvs/src/sys/net/if_vxlan.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 if_vxlan.c
> --- net/if_vxlan.c25 Oct 2017 09:24:09 -  1.63
> +++ net/if_vxlan.c8 Nov 2017 14:49:58 -
> @@ -611,6 +611,7 @@ vxlan_lookup(struct mbuf *m, struct udph
>   vni = VXLAN_VNI_UNSET;
>   }
>  
> + NET_ASSERT_LOCKED();
>   /* First search for a vxlan(4) interface with the packet's VNI */
>   LIST_FOREACH(sc, _tagh[VXLAN_TAGHASH(vni)], sc_entry) {
>   if ((uh->uh_dport == sc->sc_dstport) &&
> Index: net/pf.c

I think we are approaching point, which requires us to revisit
NET_ASSERT_LOCKED(). The assert currently tests caller is writer
on net_lock.

Since we are going to 'soften' the NET_LOCK() to R-lock for
some tasks/threads the NET_ASSERT_LOCKED() will become invalid
(false positive) assertion for functions, which are being grabbed
occasionally as readers and other times as writers (?typically in
local outbound path?). I've seen such smoke already in my diffs
I'm working on currently.

So I'd like to know, what's the plan for NET_ASSERT_LOCKED() macro?

a) are we going to relax it, so it will test if the net_lock is
locked?

b) are we going to keep it, but a new 'soft' assert will be introduced
e.g. NET_ASSERT_ALOCKED() (A for any lock)?

c) add parameter to current NET_ASSERT_LOCKED() stating desired
lock level: 0 - unlocked, 1 - reader, 2 - writer

I'm leaning towards option b) introduce a new assertion for cases
where we require lock, but we don't care if it is reader/writer.

As I've said the patch looks good to me as-is and should go in. I just
would like to hear some greater plan for NET_ASSERT_LOCKED(). Perhaps
we should go for it before we will be further playing with parallel
softnet.

thanks a lot
regards
sasha