Re: bridge(4): BRIDGE_MPSAFE by default and applying psref(9)

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

On Thu, Apr 14, 2016 at 11:56 PM, Christos Zoulas <christos%astron.com@localhost> wrote:
> In article <CAKrYomh23MGVWK0USFTU_+LCbY3Yooh1M8R2QV3dA2EKS6N+_Q%mail.gmail.com@localhost>,
> Ryota Ozaki  <ozaki-r%netbsd.org@localhost> wrote:
>>Hi,
>>
>>I prepared two patches for bridge(4):
>>  http://www.netbsd.org/~ozaki-r/remove-BRIDGE_MPSAFE.diff
>>  http://www.netbsd.org/~ozaki-r/psref-bridge.diff
>>
>>The former removes BRIDGE_MPSAFE switch and enables the
>>MP-safe code by default. After introducing softint-based
>>if_input, bridge can run in parallel even without NET_MPSAFE,
>>so I think we need to always enable it.
>>
>>The latter applies psref(9) to bridge. I confirmed the new
>>implementation survives load test, which includes repeating
>>bridge creations/deletions and member interface
>>additions/removals, over several hours.
>>
>>Note that I notice that we need to tweak shmif of the
>>rump kernel because it seems that the interrupt handler
>>of shmif can migration between CPUs and so the behavior
>>violates a contract of psref. We can fix it by applying
>>if_percpuq to shmif, but I'm not sure that's the way
>>to go. (I'll ask pooka about the issue.)
>>
>>Anyway any comments on the patches?
>
> Why bother? Just define them empty?
>
> +#define ACQUIRE_GLOBAL_LOCKS(s)        do { s = 0; } while (0)
> +#define RELEASE_GLOBAL_LOCKS(s)        do { (void)s; } while (0)

To avoid "error: statement with no effect [-Werror=unused-value]"
when NET_MPSAFE. An alternative solution may be:

  +#ifdef NET_MPSAFE
  +       int s __unused;
  +#else
          int s;
  +#endif

for each function. Is there another way to deal with the error?

>
> Nobody sets error here.
>
> +       sc->sc_iflist_psref.bip_psz = pserialize_create();
> +       if (sc->sc_iflist_psref.bip_psz == NULL)
> +               panic("%s: pserialize_create %d\n", __func__, error);

Sure. Fixed.

>
> Early return here:
>
>         if (bif != NULL) {
> -               if (bif->bif_waiting)
> -                       bif = NULL;
> -               else
> -                       atomic_inc_32(&bif->bif_refs);
> +               psref_acquire(psref, &bif->bif_psref,
> +                   sc->sc_iflist_psref.bip_class);
>
>         if (bif == NULL)
>                 return NULL;
>         psref_acquire(psref, &bif->bif_psref, sc->sc_iflist_psref.bip_class);
>         return bif;

OK. Fixed.

Thank you for reviewing!

  ozaki-r