port: always enable allmulti and simplify MAC filtering #386
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
IPv6 requires receiving multicast traffic for neighbor discovery and other essential protocols. Additionally, we will soon need to receive all multicast ethernet packets to allow Linux daemons running in network namespaces to subscribe to multicast groups independently. Rather than track multicast addresses in hardware filters (which are subject to the same capacity limits as unicast filtering), unconditionally enable allmulti mode on all ports. Simplify the MAC filter structure to only handle unicast addresses. Merge the separate ucast_filter and mcast_filter into a single filter array. Combine each MAC address with its reference count in a port_mac structure rather than maintaining parallel arrays. Remove the MAC_FILTER_F_ALL flag since only promiscuous mode (for unicast overflow) needs to be tracked. Replace GR_IFACE_S_ALLMULTI_FIXED with GR_IFACE_S_ALLMULTI as it is no longer a temporary state that can be disabled. Remove allmulti from user-configurable interface flags since it is now always enabled for all ports. Update IPv6 neighbor discovery to no longer install multicast MAC filters when joining solicited-node multicast groups, as allmulti mode already ensures their reception. Signed-off-by: Robin Jarry <[email protected]>
When typing interface set port foo promisc <tab> there is no completion because the node is a regular expression. Change it to an or node with deterministic values. Signed-off-by: Robin Jarry <[email protected]>
📝 WalkthroughWalkthroughThis pull request migrates allmulticast (ALLMULTI) handling from a configurable flag to an internal state-only mechanism and consolidates MAC filtering infrastructure. ALLMULTI is removed from public APIs, CLI configuration, and enums—no longer user-controllable. MAC filtering is refactored by merging separate unicast and multicast filter structures into a unified filter model with a port_mac array. Port reconfig now automatically enables allmulticast state. IPv6 multicast address management no longer manipulates ethernet filters directly. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frr/if_grout.c(1 hunks)modules/infra/api/gr_infra.h(1 hunks)modules/infra/cli/gr_cli_iface.h(1 hunks)modules/infra/cli/iface.c(2 hunks)modules/infra/control/gr_iface.h(0 hunks)modules/infra/control/gr_port.h(2 hunks)modules/infra/control/iface.c(0 hunks)modules/infra/control/port.c(3 hunks)modules/infra/control/port_test.c(2 hunks)modules/ip6/control/address.c(1 hunks)
💤 Files with no reviewable changes (2)
- modules/infra/control/iface.c
- modules/infra/control/gr_iface.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/ip6/control/address.cmodules/infra/control/gr_port.hfrr/if_grout.cmodules/infra/cli/gr_cli_iface.hmodules/infra/control/port_test.cmodules/infra/control/port.cmodules/infra/api/gr_infra.hmodules/infra/cli/iface.c
🧠 Learnings (4)
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/address.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/ip6/control/address.c
📚 Learning: 2025-11-05T13:55:26.169Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.169Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/api/gr_infra.h
📚 Learning: 2025-09-21T10:48:42.539Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/srv6/api/gr_srv6.h:18-21
Timestamp: 2025-09-21T10:48:42.539Z
Learning: In the Grout codebase, fixed-width enum syntax like `typedef enum : uint8_t { ... } type_t;` is valid and correct since the project uses `-std=gnu2x` which supports C23 features including fixed-width enums.
Applied to files:
modules/infra/api/gr_infra.h
🧬 Code graph analysis (3)
modules/infra/control/port_test.c (1)
modules/infra/control/port.c (1)
port_mac_add(482-551)
modules/infra/control/port.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)api/gr_errno.h (1)
errno_set(9-12)
modules/infra/cli/iface.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
Comment on lines
+350
to
+354
| if ((ret = rte_eth_allmulticast_enable(p->port_id)) < 0) | ||
| return errno_log(-ret, "rte_eth_allmulticast_enable"); | ||
|
|
||
| iface->state |= GR_IFACE_S_ALLMULTI; | ||
|
|
There was a problem hiding this comment.
Handle unsupported allmulti fallback.
Ports on NICs that return -EOPNOTSUPP/-ENOSYS from rte_eth_allmulticast_enable() will hit this return and fail to come up. We still need multicast delivery on such hardware, so we should fall back to forcing promisc instead of aborting the reconfig.
- if ((ret = rte_eth_allmulticast_enable(p->port_id)) < 0)
- return errno_log(-ret, "rte_eth_allmulticast_enable");
-
- iface->state |= GR_IFACE_S_ALLMULTI;
+ ret = rte_eth_allmulticast_enable(p->port_id);
+ if (ret < 0) {
+ if (ret == -ENOSYS || ret == -EOPNOTSUPP) {
+ LOG(NOTICE,
+ "%s: %s, forcing promisc",
+ iface->name,
+ rte_strerror(-ret));
+ ret = rte_eth_promiscuous_enable(p->port_id);
+ if (ret < 0)
+ return errno_log(-ret, "rte_eth_promiscuous_enable");
+ iface->state |= GR_IFACE_S_PROMISC_FIXED | GR_IFACE_S_ALLMULTI;
+ } else {
+ return errno_log(-ret, "rte_eth_allmulticast_enable");
+ }
+ } else {
+ iface->state |= GR_IFACE_S_ALLMULTI;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ((ret = rte_eth_allmulticast_enable(p->port_id)) < 0) | |
| return errno_log(-ret, "rte_eth_allmulticast_enable"); | |
| iface->state |= GR_IFACE_S_ALLMULTI; | |
| ret = rte_eth_allmulticast_enable(p->port_id); | |
| if (ret < 0) { | |
| if (ret == -ENOSYS || ret == -EOPNOTSUPP) { | |
| LOG(NOTICE, | |
| "%s: %s, forcing promisc", | |
| iface->name, | |
| rte_strerror(-ret)); | |
| ret = rte_eth_promiscuous_enable(p->port_id); | |
| if (ret < 0) | |
| return errno_log(-ret, "rte_eth_promiscuous_enable"); | |
| iface->state |= GR_IFACE_S_PROMISC_FIXED | GR_IFACE_S_ALLMULTI; | |
| } else { | |
| return errno_log(-ret, "rte_eth_allmulticast_enable"); | |
| } | |
| } else { | |
| iface->state |= GR_IFACE_S_ALLMULTI; | |
| } |
🤖 Prompt for AI Agents
In modules/infra/control/port.c around lines 350-354, handle -EOPNOTSUPP/-ENOSYS
from rte_eth_allmulticast_enable by falling back to enabling promiscuous mode
instead of returning: call rte_eth_allmulticast_enable and if it returns < 0
check if ret is -EOPNOTSUPP or -ENOSYS; in that case log a warning and call
rte_eth_promiscuous_enable(p->port_id), set iface->state |= GR_IFACE_S_PROMISC
(and still set GR_IFACE_S_ALLMULTI semantics as needed), and only return an
error if the promiscuous enable call fails; for any other negative return from
rte_eth_allmulticast_enable keep existing error handling and return.
christophefontaine
approved these changes
Nov 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IPv6 requires receiving multicast traffic for neighbor discovery and other essential protocols. Additionally, we will soon need to receive all multicast ethernet packets to allow Linux daemons running in network namespaces to subscribe to multicast groups independently.
Rather than track multicast addresses in hardware filters (which are subject to the same capacity limits as unicast filtering), unconditionally enable allmulti mode on all ports.
Simplify the MAC filter structure to only handle unicast addresses. Merge the separate ucast_filter and mcast_filter into a single filter array. Combine each MAC address with its reference count in a port_mac structure rather than maintaining parallel arrays.
Remove the MAC_FILTER_F_ALL flag since only promiscuous mode (for unicast overflow) needs to be tracked. Replace GR_IFACE_S_ALLMULTI_FIXED with GR_IFACE_S_ALLMULTI as it is no longer a temporary state that can be disabled.
Remove allmulti from user-configurable interface flags since it is now always enabled for all ports.
Update IPv6 neighbor discovery to no longer install multicast MAC filters when joining solicited-node multicast groups, as allmulti mode already ensures their reception.
Summary by CodeRabbit
Refactor
Bug Fixes