Re: passive references

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

   Date: Wed, 17 Feb 2016 13:56:02 +0900
   From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>

   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-nakahara%iij.ad.jp@localhost>
   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-nakahara%iij.ad.jp@localhost>
   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