Date: Wed, 17 Feb 2016 13:56:02 +0900 From: Kengo NAKAHARA <k-nakah...@iij.ad.jp>
I fix above bugs and rebase, here is "git format-patch" patch series. http://www.netbsd.org/~knakahara/fix-gif-softint-using-psref2/fix-gif-softint-using-psref2.tgz # I think this can be applied cleanly this time... And here is the unified patch. http://www.netbsd.org/~knakahara/fix-gif-softint-using-psref2/unified-fix-gif-softint-using-psref2.patch Could you comments this patch? Some comments on all the patches in that box: commit 739235b87a941f9fb0e7150e9d7c0f888a3fd30f Author: k-nakahara <k-nakah...@iij.ad.jp> Date: Fri Jan 15 13:10:10 2016 +0900 fix: let gif(4) promise softint(9) contract diff --git a/sys/net/if_gif.c b/sys/net/if_gif.c index 0310969..d381cc0 100644 @@ -776,6 +780,26 @@ gif_encap_detach(struct gif_softc *sc) ... +static void +gif_encap_pause(struct gif_softc *sc) +{ + struct ifnet *ifp = &sc->gif_if; + uint64_t where; + + ifp->if_flags &= ~IFF_RUNNING; + membar_sync(); No need for membar_sync here: the xc_broadcast/xc_wait after this has the same effect. @@ -908,24 +921,15 @@ void gif_delete_tunnel(struct ifnet *ifp) { struct gif_softc *sc = ifp->if_softc; - struct sockaddr *osrc, *odst; - void *osi; int s; s = splsoftnet(); + encap_lock_enter(); Why is this called encap_lock_enter/exit? It seems to me that this is specific to gif -- nothing else uses it outside if_gif.c, and nothing in ip_encap.c relies on it. Can you document the resources that encap_lock is supposed to serialize? It looks like this is what I referred to as the global lock for the system's gif(4) configuration, gif_lock, in my sketch at <https://mail-index.netbsd.org/tech-net/2016/01/11/msg005475.html>. In that case, it serializes changes to the systemwide set of gif(4) tunnels, and preserves the invariant that there are no overlapping gif(4) tunnels. But maybe it will *also* be needed to serialize changes to the set of ip_encap tunnels too -- in that case, the name is good, but it needs to be used in ip_encap as well. More on that below. It also looks like this change is just about gif(4) -- it is independent of the change to use an rwlock for ip_encap. Can you split this one into two changes? Or are there locking rules related to both encap_lock and the rwlock you added that I haven't guessed? commit 568b52fa26e6141251fc4e63264fa93920e468ff Author: k-nakahara <k-nakah...@iij.ad.jp> Date: Thu Feb 4 17:43:29 2016 +0900 add receive side stop code to gif_encap_pause() Can you explain in a little more detail why this change is necessary? My memory is fuzzy -- I recall I suggested something like it, but I suggested that because I thought the softint was scheduled by the receive side, not by the send side. The rest of the changes look pretty good except for one part: where you commented /* check if anyone have already attached with exactly same config */ in encap_attach, you cannot rely on that check to *continue* to be true after pserialize_read_exit: someone else might concurrently call encap_attach. You will need, e.g., encap_lock to cover *all* of encap_attach, so that there can be only one change to the set of ip_encap tunnels at any given time. So perhaps this is what you want: - Caller must do encap_lock_enter/encap_attach/encap_lock_exit. - encap_attach should kassert encap_lock_held. - No need for pserialize read section in encap_attach. - No need for *separate* encaptab.lock and encap_lock -- these locks could be one and the same. (Of course, if the caller of encap_attach holds encap_lock, it is not necessary for encap_add to acquire it.) This way, gif(4) can do 1. encap_lock_enter 2. check gif configurations 3. establish/disestablish softints 4. encap_attach 5. encap_lock_exit