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

[Ryota Ozaki]

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?

Thanks,
ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Ryota Ozaki
Hi,
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)

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);

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;

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ryota Ozaki]

Post by Christos Zoulas

Post by Ryota Ozaki
Hi,
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?

Post by Christos Zoulas
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.

Post by Christos Zoulas
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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de