Details

Reviewers

emaste
philip
melifaro

Group Reviewers

network

Commits

rS360345: bridge: epoch-ification

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.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kp created this revision.Apr 1 2020, 2:26 PM

Herald added subscribers: melifaro, ae. · View Herald TranscriptApr 1 2020, 2:26 PM

Harbormaster completed remote builds in B30229: Diff 70091.Apr 1 2020, 2:26 PM

kp added a parent revision: D24249: bridge: Change lists to CK_LIST as a peparation for epochification.Apr 1 2020, 2:27 PM

kp added a child revision: D24251: bridge tests: Test for #216510.

kp edited the test plan for this revision. (Show Details)Apr 1 2020, 2:30 PM

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

Herald added a subscriber: imp. · View Herald TranscriptApr 1 2020, 2:30 PM

afedorov added a subscriber: afedorov.Apr 1 2020, 4:35 PM

Rebase after 24249 changes, and add missing NET_EPOCH_ENTER()

Harbormaster completed remote builds in B30240: Diff 70110.Apr 2 2020, 8:27 AM

Fix rebase, hopefully

Harbormaster completed remote builds in B30241: Diff 70111.Apr 2 2020, 8:30 AM

Fix rebase

Harbormaster completed remote builds in B30242: Diff 70113.Apr 2 2020, 9:27 AM

crest_bultmann.eu added a subscriber: crest_bultmann.eu.Apr 2 2020, 10:02 AM

mateusz_serveraptor.com added a subscriber: mateusz_serveraptor.com.Apr 2 2020, 10:26 AM

wanpengqian_gmail.com added a subscriber: wanpengqian_gmail.com.Apr 8 2020, 12:51 AM

Additional assertions & fix a possible memory leak on bridge destruction

Harbormaster completed remote builds in B30359: Diff 70335.Apr 8 2020, 11:33 AM

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.

emaste added inline comments.Apr 17 2020, 4:18 PM

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

Harbormaster completed remote builds in B30581: Diff 70731.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().

Harbormaster completed remote builds in B30584: Diff 70736.Apr 18 2020, 1:59 PM

emaste accepted this revision.Apr 20 2020, 8:52 PM

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

Capitalise

This revision now requires review to proceed.Apr 21 2020, 9:12 AM

Harbormaster completed remote builds in B30645: Diff 70829.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

kp added inline comments.Apr 22 2020, 7:46 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.