Re: reference counts

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

On Mon, Apr 13, 2015 at 10:47 PM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Mon, 13 Apr 2015 19:13:23 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    On Mon, Apr 13, 2015 at 5:22 PM, Masao Uebayashi <uebayasi%gmail.com@localhost> wrote:
>    > ISTR that ozaki-r@ had a similar code for part of pserialized IFNET_FOREACH().
>
> Right, I had forgotten about that!  It looks a bit less flexible than
> my proposal, but I remember thinking last summer that that was
> something we ought to provide in the kernel generally.
>
>    Yup. And bridge(4) already has a similar facility. I think we can use
>    riastradh's one in bridge. I'll try the patch if it really satisfies
>    bridge's requirement and if it works or not.
>
> I looked at the bridge code, and I'm having a hard time proving to
> myself that it can't miss a wakeup: at the very least, I'm pretty sure
> bridge_release_member needs a membar_consumer (although maybe on x86
> it doesn't matter since atomic implies membar_sync).

Thank you for pointing out. My code may very depend on x86
because I tested it on only x86 machines. I'll fix or change
to use refcount(9) if possible.

>
> But the membars are unnecessary if the transition from referenced to
> unreferenced is always done under a mutex anyway, which makes
> everything easier to reason about.
>
> Here's how I think it would look with my refcount(9) proposal:
>
> bridge_release_member(sc, bif)
> {
>
>         refcount_dec_broadcast(&bif->bif_refcount, &sc->sc_iflist_intr_lock,
>             &sc->sc_iflist_cv);
> }
>
> bridge_delete_member(sc, bif)
> {
>
>         ...
>
>         mutex_enter(&sc->sc_iflist_intr_lock);
>         refcount_dec_drain(&bif->bif_refcount, &sc->sc_iflist_intr_lock,
>             &sc->sc_iflist_cv);
>         mutex_exit(&sc->sc_iflist_intr_lock);
>
>         kmem_free(bif, sizeof(*bif));
> }
>
> When iflist_lock and iflist_intr_lock merge later on, this can be
> merged into the BRIDGE_LOCK(sc)/BRIDGE_UNLOCK(sc) section earlier in
> bridge_delete_member too.

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

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

  ozaki-r