Page MenuHomeFreeBSD

bridge: Try to make the GRAB_OUR_PACKETS macro a bit more readable
ClosedPublic

Authored by markj on Feb 6 2023, 9:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jun 5, 6:37 PM
Unknown Object (File)
Wed, Jun 5, 6:37 PM
Unknown Object (File)
Mon, Jun 3, 10:15 AM
Unknown Object (File)
Mon, Jun 3, 10:01 AM
Unknown Object (File)
Sat, May 25, 10:29 PM
Unknown Object (File)
Mon, May 20, 11:07 AM
Unknown Object (File)
Apr 9 2024, 5:32 AM
Unknown Object (File)
Mar 31 2024, 12:59 PM

Details

Summary
  • Let the compiler use constant folding to eliminate conditionals.
  • Fix some inconsistent whitespace.

No functional change intended.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 49578
Build 46468: arc lint + arc unit

Event Timeline

markj requested review of this revision.Feb 6 2023, 9:31 PM
zlei added a subscriber: zlei.

Looks good to me, and I like this change ;)

This revision is now accepted and ready to land.Feb 7 2023, 7:44 AM
sys/net/if_bridge.c
2570

It's much better than it was before.

Any thoughts on making it an inline function to improve readability further?
There are 8 arguments that are used in the macro: iface, eh, bif, m, ifp, bifp, vlan and sc.
It seems that vlan and sc are only used when the interface is in learning mode (e.g. short period of time), so these can be derived from m and ifp, leaving 6 arguments, which can be doable.
For example, something like
bool grab_our_packet(target_ifp, source_ifp, source_bifp, source_bif, &m, eh) {} returning true if the packet is consumed can be an option.

what do you think?

sys/net/if_bridge.c
2570

It seems that vlan and sc are only used when the interface is in learning mode (e.g. short period of time)

Are you thinking of STP learning? This is a different type of learning, and by default is always enabled. I suspect that it is useful to cache at least vlan, in any case.

I tried implementing your suggestion, but I don't know if it's much better. The problem is that there are three possible return values: 1) continue processing in ether_input(), 2) drop the packet, 3) try the next interface. So to make the control flow nice, I ended up introducing a couple of helpers. See https://reviews.freebsd.org/D38425

sys/net/if_bridge.c
2570

Are you thinking of STP learning? This is a different type of learning, and by default is always enabled. I suspect that it is useful to cache at least vlan, in any case.

Yep, that's datapath learning and I was wrong.

I tried implementing your suggestion, but I don't know if it's much better. The problem is that there are three possible return values: 1) continue processing in ether_input(), 2) drop the packet, 3) try the next interface.

I'm not advocating for my approach, more trying to discuss a reasonable approach to eliminate the huge #defined function.

So to make the control flow nice, I ended up introducing a couple of helpers. See https://reviews.freebsd.org/D38425

Thank you for doing that!
From my point of view, the new code is now readable - and it looks much easier to reason about/improve.

sys/net/if_bridge.c
2570

Btw, do you mind adding kp / network to the new review so they can comment/compare as well?