Page MenuHomeFreeBSD

bridge: epoch-ification
ClosedPublic

Authored by kp on Apr 1 2020, 2:26 PM.

Details

Summary

Run the bridge datapath under epoch, rather than under the
BRIDGE_LOCK().

We still take the BRIDGE_LOCK() whenever we insert or delete items in
the relevant lists, but we use epoch callbacks to free items so that
it's safe to iterate the lists without the BRIDGE_LOCK.

Tests on mercat5/6 shows this increases bridge throughput significantly,
from 3.7Mpps to 18.6Mpps.

Sponsored by: The FreeBSD Foundation

Test Plan

Bridge regression tests were executed, with WITNESS and INVARIANTS enabled.

Performance test shows an increase in throughput from 3.7Mpps to 18.6Mpps.
flame graph before the change: https://people.freebsd.org/~kp/if_bridge/unmodified.svg
flame graph after the change: https://people.freebsd.org/~kp/if_bridge/unicast.svg

This is also running happily on my home gateway.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kp set the repository for this revision to rS FreeBSD src repository - subversion.

Rebase after 24249 changes, and add missing NET_EPOCH_ENTER()

Additional assertions & fix a possible memory leak on bridge destruction

Barring objections I plan to commit this by next Saturday (April 25th).

I thought I had accepted this latest revision. Sorry about that.

These changes look good.

This revision is now accepted and ready to land.Apr 17 2020, 1:58 AM

I intend to look this over today.

sys/net/if_bridge.c
240 ↗(On Diff #70335)

Extra blank lines (here and elsewhere)?

607 ↗(On Diff #70335)

non-style(9) comment, but maybe can shorten it to just e.g. "callbacks may be using the UMA zone"?

778 ↗(On Diff #70335)

hrmph

LGTM

sys/net/if_bridge.c
202 ↗(On Diff #70335)

Are BRIDGE_LOCK2REF/ BRIDGE_UNREF still needed?

  • Whitespace & commit remarks
  • Remove BRIDGE_LOCK2REF/BRIDGE_UNREF As a result BRIDGE_XLOCK, BRIDGE_XDROP and the condition variable are also not required any more. These were used to ensure that member interfaces could not dissapear during processing. That's now provided by epoch, so the entire structure is not needed any more.
This revision now requires review to proceed.Apr 18 2020, 8:38 AM

When we wait for callbacks to finish before destroying the V_bridge_rtnode_zone
we need to use epoch_drain_callbacks(), not NET_EPOCH_WAIT().

emaste added inline comments.
sys/net/if_bridge.c
574 ↗(On Diff #70736)

Oops, my example should have used sentence case, /* Callbacks may use the UMA zone. */

This revision is now accepted and ready to land.Apr 20 2020, 8:52 PM
This revision now requires review to proceed.Apr 21 2020, 9:12 AM

Looks good to me!

sys/net/if_bridge.c
778 ↗(On Diff #70335)

Yeah. I don't really like the __containerof() either. It's the very definition of a layering violation. However, I don't think there's a way around it without substantially rewriting if_bridge.

Perhaps we can make this look a little less unsightly with a macro?

This revision is now accepted and ready to land.Apr 22 2020, 7:38 AM
sys/net/if_bridge.c
778 ↗(On Diff #70335)

It's how every NET_EPOCH_CALL user in the kernel does it.

We could extend struct epoch_context with a void* to pass this sort of thing, but it'd mean that struct bridge_softc would contain a struct epoch_context, which in turn would have a pointer to the struct bridge_softc.
That's possible, but it's an extra step we could make mistakes in.

We could also wrap it in a macro, but all that'd do is hide what's going on, and we'd still have to pass all of the same arguments, or have different macros for the different structures we use this for.

This revision was automatically updated to reflect the committed changes.