Re: passive references

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

Hi,

Thank you for comments and an updated patch.

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

On 2016/02/13 5:20, Taylor R Campbell wrote:
> 1. As I noted before, it makes no sense to separate psref_target_drain
> from psref_target_destroy.  The caller should just (a) prevent new
> references (e.g., by removing the target from a list and doing
> pserialize_perform), and then (b) drain/destroy.  So I will remove
> psref_target_drain and move its logic into psref_target_destroy.

I fixed ip_encap.c only and did not remove psref_target_drain().

> 2. Since the caller of psref_target_destroy must prevent new
> references from being acquired, it makes no sense for psref_acquire to
> ever fail -- the target must not be draining.  So I will make it
> kassert, and never fail, and return void.

I see. With no fail, psref_acquire() become easier to use :)

>    BTW, I think psref utilities to use KASSERT(like mutex_owned) would be
>    useful for debug. However, I have no idea to implement it lightly...
> 
> 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?

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

> Now for some comments on your patch:
> 
> - You cannot re-acquire a psref after you have released it: it may be
> destroyed by now.  You have some code that does this in encap4_lookup:
//snip 
> When I write a man page, I will add a note about this rule.  It's a
> little subtle: the rule is not *generally* that a target can never be
> re-acquired after it has been released once -- that would be absurd!
> 
> But once you call psref_release, the object may be destroyed, unless
> you find it again using the same method by which you found it the
> first time around -- e.g., by doing a fresh pserialize-read lookup in
> a list.

I fix it. I agree, please note it man...

> - As I noted in a recent message to Ozaki-san, queue(3) does not issue
> the necessary memory barriers.  In particular, LIST_INSERT_HEAD must
> issue a membar_producer somewhere in the middle -- but it doesn't.
> 
> This is already a bug in bridge(4).  So I think I will revive my
> proposal to add some variants to queue(3) operations that do issue the
> necessary memory barriers.

I implement LIST_INSERT_HEAD_PSZ copied from the following mail.
    https://mail-index.netbsd.org/tech-kern/2014/11/21/msg018055.html

I think it seems too ad-hoc. I want *_PSZ macros in queue.h, however
rmind@n.o objected it, hum...

> - It appears that your patch does several functional things:
> (a) restores USE_RADIX in ip_encap.c,
> (b) converts ip_encap.c to use psref, and
> (c) teaches in_gif to pause before disestablish.
> Can you split it into a series of patches for more incremental review?
> It looks like you're using Git to do this, so I'd be happy with `git
> format-patch' output.

I'm sorry about too large patch. I upload 'git format-patch' patch
series to the beginning of this mail URL.

> - For the encaptab globals, you used the structure member prefix
> convention -- each member of `encaptab_struct' is named `est_...'.
> 
> Since there is exactly one instance of this structure, I think this
> convention mainly adds clutter to the code -- grep can already find
> all uses of the members, because they are always used directly by
> writing `encaptab_struct.est_...' verbatim.
> 
> I would also move the lock into the the structure and make it all
> cache-line-aligned, because the members will generally all be used
> together, like so:
> 
> static struct {
> 	kmutex_t		lock;
> 	LIST_HEAD(, encaptab)	list;
> 	struct psref_class	*psr_class;
> 	pserialize_t		psz;
> } encaptab __cacheline_aligned;
> 
> Then `encaptab_struct.ets_list' becomes a little less of a mouthful,
> just `encaptab.list'.  We have some other examples of this pattern in
> src, such as vcache in sys/kern/vfs_vnode.c.

I see, I apply this way.

Could you comment the above 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>