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 -0000 1.63
> > +++ net/if_vxlan.c 8 Nov 2017 14:49:58 -0000
> > @@ -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, &vxlan_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?
