Re: passive references

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi riastradh@n.o,

Thank you for your reviews.

In fist, I reflect your reviews and restructure patch series.
Here is git format-patch patch series.
    http://www.netbsd.org/~knakahara/fix-gif-softint-using-psref2/fix-gif-softint-using-psref2-2.tgz
And here is the unified patch, maybe unnecessary.
   http://www.netbsd.org/~knakahara/fix-gif-softint-using-psref2/unified-fix-gif-softint-using-psref2-2.patch


On 2016/03/01 3:07, Taylor R Campbell wrote:
>    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
snip
> No need for membar_sync here: the xc_broadcast/xc_wait after this has
> the same effect.

I see. I will remove the unnecessary membar_sync().

>    @@ -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?

Uh...sorry, it's my mistake. As you point out below, encap_attach()
check duplicated configuration. In contrast, encap_attach_func()
does not check. So, I erroneously assumed that only encap_attach()
is required to hold encap_lock. However, not only encap_attach()
but also encap_attach_func() is required to hold encap_lock to avoid
encap[46]_lookup() and encap_add() race.
I will add encap_lock() to the following functions.
    - stf_clone_create()@if_stf.c
    - add_vif()@ip_mroute.c
    - ipe4_attach()@xform_ipip.c.


> 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>.

Yes, right. :)

BTW, as the KERNEL_LOCK ranges are increased by the following
integration encaptab.lock and encap_lock, I make encap_lock_enter/exit
interruptable which you present the above mail. Thanks.

> 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?

I bundled the modifications which is required to fix panic caused
by the race between gif(4) packet processing and ioctl, but that
is a bad idea, sorry.
I will split gif(4) change and ip_encap change to the following two
patches.
    - 0001-let-gif-4-promise-softint-9-contract-1-2-gif-4-side.patch
    - 0002-let-gif-4-promise-softint-9-contract-2-2-ip_encap-si.patch


>    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.

Sorry about a lack of explanation. This patch does not relate with
softint. This patch fixed a race in radix tree. In my parallel test
which several tens of "ifconfig {tunnnel,deletetunnel}" and gif(4)
flood ping run, I encountered panic at rn_match()@radix.c. It must
be caused by parallel run between rn_match() and rn_delete().
This is caused by radix tree which is not MP-safe, however in this
gif(4) case, I think more serious problem is receiving packet
processing is not suspended nevertheless sending packet processing
is. In our design, I think, if ioctl(writing configuration) begin,
packet processing(reading configuration) should be suspended. So,
I added above patch.
# I think ip_encap may not use radix tree in the future.
# If encap[46]_lookup() can eliminate linear search, it does not
# need to use radix tree to fix many tunnels scaling issue.


> 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

I see. I will integrate encaptab.lock and encap_lock, and implement
above your design. The modification is the following patch.
    - 0010-reflect-reastradh-n.o-s-2016-03-02-review.patch


Could you comment these patches?


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>