Re: passive references

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

Hi,

On 2016/02/16 7:23, Taylor R Campbell wrote:
>    Date: Mon, 15 Feb 2016 19:26:44 +0900
>    From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
> 
>    First, I fix some bug which you point out. Here is the
>    "git format-patch" patch series.
>        http://www.netbsd.org/~knakahara/fix-gif-softint-using-psref1/fix-gif-softint-using-psref1.tgz
>    And here is the unified patch.
>        http://www.netbsd.org/~knakahara/fix-gif-softint-using-psref1/unified-fix-gif-softint-using-psref1.patch
> 
> Thanks!  I'm a little confused, though: that patch series doesn't
> apply cleanly when I extract it and pass it to `git am'.  Some of the
> patches conflict with each other.

Sorry, it includes some old patch. I updated it.


> I would still like to see the independent functional changes in
> separate patches -- especially USE_RADIX, because our radix tree code
> is not safe for pserialize(9).  But I'll just make a few notes on the
> unified patch for now.

Sorry, it is my lack of consideration. The radix trees(encap_head[])
and encaptab list must be updated atomically, so I thought it is
required a exclusion like rwlock(9) if defined USE_RADIX. rwlock(9)
is hated, so I considered alternate method, however it is being
suspended, sorry.


>    > I added a psref_held(target, class) operation, to be used only in
>    > assertions:
>    > 
>    > 	psref_acquire(r, t, c);
>    > 	...
>    > 	KASSERT(psref_held(t, c));
>    > 	...
>    > 	psref_release(r, t, c);
> 
>    As far as I see the implementation, it seems to be able to be used
>    after psref_release(and before pserialize_read_exit()). And I want
>    to use such way.
>    # see the following 0010-add-psref-KASSERT-to-encap-46-_lookup.patch
>    Is this wrong?
> 
> Hmm...  I think in this case it happens to be safe, but in general, I
> would advise against doing that.  As soon as you call psref_release,
> you generally cannot use the target any more.
> 
> Also, if you assert !psref_held, that means even the *caller* is not
> allowed to hold a passive reference to the target -- which makes the
> contract of your function more complex.
> 
> So I would advise not to use !psref_held -- it is even more
> questionable than !mutex_owned.

I see. I don't use !psref_held.
# I wanted !psref_held as I struggled against my psref_release()
# leak bug...


> In the two cases where you use it, I would just make sure that the
> very last action in the loop body is an unconditional psref_release.
> In fact, you seem to be missing a psref_release for ep in the case
> that 0 < prio < matchprio.

Oh..., you are right. Sorry about lack of my self-reviews.


>    > A new patch for psref is attached.  API changes:
>    > 
>    > - Removed psref_target_drain; moved logic into psref_target_destroy.
>    > - Made psref_acquire never fail.
>    > - Added psref_copy.
>    > - Added psref_held.
> 
>    Hum, it seems psref_copy is lost. I add it in 0006-add-psref_copy.patch.
>    If there are mistakes, please point out.
> 
> I must have forgotten to rerun cvs diff before sending my patch,
> sorry!  It looks functionally equivalent to what I had -- I had just
> added a couple more comments.  New psref2.patch attached.

Thank you for new patch. I use it with reading the comments.


> In your patch, you try to do a radix tree lookup in a pserialize read
> section.  It would be nice if that worked -- but I am pretty sure that
> our radix tree code is not going to be pserialize-safe, because it
> won't do membar_producer/membar_datadep_consumer.
> 
> However, since we can drop packets here, we can use a somewhat
> heavy-handed alternative: simply drop all packets while anyone is
> updating the radix tree.  Something like this:

Hmm..., it causes heavy penalty which all of the encap interfaces
not only gif(4) but also stf(4) and ipsec(through xform_ipip.c)
drop all packets. However, I thinks it would be acceptable as
attach/detach operations must be done less often.

BTW, I would design another(not radix tree) solution to fix the
scaling issue of many encap interfaces in the future work.
Does anyone have the solution to fix the scaling issue?


> Small comment: In encap4_lookup, you do the following:

Thank you, I fix it.


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?


Thanks,

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

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

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