Re: reference counts

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Wed, Apr 15, 2015 at 12:34 PM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Wed, 15 Apr 2015 11:27:16 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    I'm trying refcount(9) with bridge. It works well for now.
>    (I got kernel freeze once but I'm not sure it's due to refcount(9)
>    because my testing VM sometimes freezes around uvm :-/)
>
>    The diff is here: http://www.netbsd.org/~ozaki-r/bridge-refcount.diff
>
> Looks nice!  I spotted one race which was there to begin with: nothing
> prevents multiple concurrent bridge_delete_member on the same member.
> I think bridge_delete_member should require the caller to hold the
> lock, and drop it to wait for users to drain:
>
> bridge_delete_member(sc, bif)
> {
>
>         KASSERT(BRIDGE_LOCKED(sc));
>         ...
>         LIST_REMOVE(bif, bif_next);
>         BRIDGE_PSZ_PERFORM(sc);
>
>         BRIDGE_UNLOCK(sc);
>         mutex_enter(sc->sc_iflist_intr_lock);
>         refcount_dec_drain(&bif->bif_refcount, &sc->sc_iflist_cv,
>             &sc->sc_iflist_intr_lock);
>         mutex_exit(sc->sc_iflist_intr_lock);
>         ...
>         BRIDGE_LOCK(sc);
> }
>
> bridge_ioctl_del(sc, arg)
> {
>
>         BRIDGE_LOCK(sc);
>         LIST_FOREACH(bif, &sc->sc_iflist, bif_next)
>                 ...
>         bridge_delete_member(sc, bif);
>         BRIDGE_UNLOCK(sc);
>         ...
> }
>
> bridge_clone_destroy(sc)
> {
>
>         ...
>         BRIDGE_LOCK(sc);
>         while ((bif = LIST_FIRST(&sc->sc_iflist)) != NULL)
>                 bridge_delete_member(sc, bif);
>         BRIDGE_UNLOCK(sc);
>         ...
> }

Hmm, right. It protected a member between bridge_ioctl_del,
but didn't between bridge_ioctl_del and bridge_clone_destroy.
Your fix makes sense and works for me.

>
>    Two comments to refcount(9) from me:
>    - Can we have cv in struct refcount? I think we don't need to share
>      it with outside refcount unlike mutex.
>
> I'd rather not do that because:
>
> (a) It doesn't hurt to share the condvar for most users -- all
> condvars share sleepqs with some other condvars anyway, and everyone
> must be prepared for spurious wakeups.
>
> (b) Not all users need the condvar, so it would waste space for them.
> Consider, e.g., kauth_cred_t, which would use refcount_dec_local
> instead of refcount_dec_{signal,broadcast}.

I see. I was thinking only my cases.

>
>    - It seems that the API assumes that refcount_inc is not called
>      after refcount_dec_drain as KASSERT(0 < old) in refcount_inc
>      implies. Don't we need to guarantee the assumption somehow?
>      Or users have to guarantee that by themselves? (e.g., by using
>      pserialize like bridge.) If so, we should document about that
>      in refcount.9.
>
> Users should remove the last reference in a table, or mark it dying,
> so that nobody else can get to it, before calling refcount_dec_drain.
> I added a note to the man page -- new draft attached.

LGTM

>
> This also enables a little optimization in refcount_dec_drain: if the
> reference count is 1, it can avoid the atomic.  This means, e.g.,
> emptying a large table (say, the vnode cache) need not issue atomic
> operations for every entry, only for those that are currently in use.

+       /*
+        * Caller guarantees if our reference is exclusive, nobody can
+        * acquire new references.  (If there are other references
+        * already, then they may turn into new references.)
+        */
+       if (refcount->rc_value == 1)
+               refcount->rc_value = 0;

                ^ We have to return here?

+
        if (0 < atomic_dec_uint_nv(&refcount->rc_value))
                do cv_wait(cv, interlock); while (0 < refcount->rc_value);

                ^ otherwise, we accidentally come here and wait forever

  ozaki-r