Re: fix gif(4)'s softint(9) contract violation [was Re: RFC: gif(4) MP-ify]

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

Hi,

On 2016/01/23 3:31, Taylor R Campbell wrote:
>    Date: Fri, 22 Jan 2016 16:29:40 +0000
>    From: Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>
> 
>    Part (2) makes ip_encap scale well to many cores, but it stops
>    ip_encap from scaling well to many tunnels, because it removes the
>    radix tree.  Maybe that's OK -- maybe today, scaling to many cores is
>    more important than scaling to many tunnels.
> 
> Oops -- I didn't read encap4/6_lookup closely enough.  They *always*
> iterate over the entire list of tunnels and call every relevant
> function stored there to prioritize matches.  So the radix tree
> probably never did help (much) to scale to many tunnels after all, and
> my concern was entirely moot!

I think not entirely. The usage of radix tree reduces calculation amount
for each loop. As the results of my benchmarks, origin implementation
(with radix tree) keeps performance up to about 100 gifs, in contrast
my implementation (without radix tree) does not that far.


> If you want to skip the radix tree, I suggest committing essentially
> the reverse of ip_encap.c 1.45 first (remove old USE_RADIX case),
> separately from any MP issues.

Ok, I will do it when I ready to commit ip_encap mp-ify codes.


> That said, it is still not clear to me how you guarantee the tunnel
> will continue to exist in encap4/6_input, after pserialize_read_exit
> and during the call to psw->pr_input.

Sorry, encap_detach() is required in gif_encap_pause() to prevent deletion
during in{,6}_gif_input() after gifp NULL check.

In the aspect of encap[46]_lookup, the psw is statically allocated as
in_gif_protosw, in6_gif_protosw and in_stf_protosw. So, once the address
is dereferenced, it is not required to protect tunnel itself. In addition,
the protection of gifp (as m_tag set by encap_fillarg()) is responsibility
of not ip_encap codes but gif(4) codes.
I think the above, but there might be an oversight...


> I also wonder whether it is OK to call ep->func inside a pserialize
> read section: it enters a lot of nested callbacks in the network stack
> which don't look safe to me.  For example, gif_encapcheck4 calls
> gif_validate4 calls rtalloc1 which doesn't look very safe for
> pserialize read to me.

I ignored it based on the premise of removing them, because
GIF_ENCAPCHECK is not defined by any architectures. However, I change
my mind, there is a possibility that new interface which use memory
allocation in ep->func will added in the future. So, I will implement
encap_change_enter()/encap_change_exit() as you point out the other mail.


Thanks,

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

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

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