Hi, On 2016/02/16 7:23, Taylor R Campbell wrote: > Date: Mon, 15 Feb 2016 19:26:44 +0900 > From: Kengo NAKAHARA <k-nakah...@iij.ad.jp> > > 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-nakah...@iij.ad.jp>