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)
Jan 10 2024, 8:36 PM
Unknown Object (File)
Jan 10 2024, 8:23 PM
Unknown Object (File)
Dec 26 2023, 6:08 AM
Unknown Object (File)
Dec 20 2023, 5:20 AM
Unknown Object (File)
Dec 12 2023, 4:23 PM
Unknown Object (File)
Dec 10 2023, 8:06 PM
Unknown Object (File)
Oct 28 2023, 8:06 PM
Unknown Object (File)
Oct 27 2023, 10:19 AM

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 Not Applicable
Unit
Tests Not Applicable

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
2558–2563

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
2558–2563

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
2558–2563

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
2558–2563

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