infra: expose link speed attribute for all interface types #401
Conversation
📝 WalkthroughWalkthroughThe pull request moves interface speed tracking into the base interface type by adding Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
modules/infra/api/gr_infra.h (1)
61-74: Centralizing link_speed in __gr_iface_base looks goodAdding
link_speedto__gr_iface_baseand dropping it from port‑specific info cleanly makes speed available for all iface types and matches how the rest of the PR uses it (ports, bonds, VLANs, loopback, IPIP, CLI).As a small optional improvement, consider defining a common sentinel here (e.g.
#define GR_IFACE_SPEED_UNKNOWN UINT32_MAX) and documenting thatlink_speedis either a value in Mb/s orGR_IFACE_SPEED_UNKNOWN, so callers don’t have to hard‑codeUINT32_MAX.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
modules/infra/api/gr_infra.h(1 hunks)modules/infra/cli/bond.c(1 hunks)modules/infra/cli/iface.c(1 hunks)modules/infra/cli/port.c(0 hunks)modules/infra/control/bond.c(4 hunks)modules/infra/control/iface.c(3 hunks)modules/infra/control/loopback.c(2 hunks)modules/infra/control/port.c(1 hunks)modules/infra/control/vlan.c(1 hunks)modules/ipip/control.c(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/cli/port.c
🧰 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/ipip/control.cmodules/infra/api/gr_infra.hmodules/infra/cli/iface.cmodules/infra/cli/bond.cmodules/infra/control/bond.cmodules/infra/control/port.cmodules/infra/control/vlan.cmodules/infra/control/iface.cmodules/infra/control/loopback.c
🧠 Learnings (5)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
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.hmodules/infra/control/iface.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 326
File: modules/infra/control/graph.c:360-364
Timestamp: 2025-10-02T07:42:42.135Z
Learning: In the grout codebase, `gr_vec_free()` is a macro defined in `main/gr_vec.h` that automatically sets the vector pointer to NULL after freeing the memory, preventing double-free issues. The macro definition is: `#define gr_vec_free(v) ((v) ? free(__gr_vec_hdr(v)) : (void)0, (v) = NULL)`.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/infra/control/loopback.c
🧬 Code graph analysis (5)
modules/ipip/control.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/bond.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/iface.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: rpm
🔇 Additional comments (14)
modules/infra/control/loopback.c (2)
17-17: Include for link speed macro is appropriateAdding
#include <rte_ethdev.h>here is correct to support the speed macro used below and fits with the surrounding DPDK includes.Please confirm your minimum DPDK version provides
RTE_ETH_SPEED_NUM_10Ginrte_ethdev.h.
262-265: Loopback link_speed initialization is consistentSetting
iface->link_speedto 10G during loopback init gives a deterministic, non‑unknown speed for this logical interface and aligns with the new link_speed semantics.modules/infra/cli/iface.c (1)
467-470: Speed display logic matches link_speed semanticsUsing
UINT32_MAXfor "unknown" and otherwise printing<value> Mb/sis consistent with the newlink_speedfield and its documented units.modules/ipip/control.c (1)
98-101: IPIP link_speed initialization is appropriateAssigning a fixed 10G
link_speedfor IPIP interfaces during init gives them a consistent logical speed and integrates cleanly with the unified iface speed handling.modules/infra/control/vlan.c (1)
80-88: Propagating parent link_speed to VLAN iface is correctMirroring
state,mtu, and nowlink_speedfromnext_parentkeeps VLAN interfaces consistent with their parent’s characteristics on (re)attach.modules/infra/control/port.c (1)
670-670: LGTM!The assignment of link speed to the base interface structure is correct and aligns with the PR's objective.
modules/infra/control/iface.c (3)
16-16: LGTM!The include is necessary for the
RTE_ETH_SPEED_NUM_UNKNOWNconstant used in this file.
111-111: LGTM!Initializing link_speed to UNKNOWN for newly created interfaces is correct.
516-528: LGTM!The propagation logic correctly handles link speed for subinterfaces: inheriting the parent's speed on STATUS_UP and resetting to UNKNOWN on STATUS_DOWN.
modules/infra/control/bond.c (4)
237-237: LGTM!The speed accumulator variable is correctly initialized for tracking aggregate bond speed.
240-262: LGTM!Active-backup mode correctly captures the active member's link speed.
263-302: LGTM!LACP mode correctly:
- Uses link speed for the aggregation key (line 276)
- Accumulates speeds only for active members with known speeds (lines 298-299)
305-309: LGTM!The final assignment correctly sets the bond's link_speed: using the accumulated value when available, or UNKNOWN when no valid speed was determined.
modules/infra/cli/bond.c (1)
39-42: Code is correct; no changes needed.The hardcoded
UINT32_MAXin cli/bond.c is the only viable option. TheRTE_ETH_SPEED_NUM_UNKNOWNconstant is unavailable in the CLI module's include chain—CLI intentionally lacks direct DPDK header access to maintain API separation. The control modules (control/iface.c, control/bond.c) can use the constant because they haverte_ethdev.hincluded. Adding DPDK includes to CLI would violate this architectural boundary.The current hardcoded value is semantically correct (UINT32_MAX = RTE_ETH_SPEED_NUM_UNKNOWN) and appropriate for the CLI module's context.
Likely an incorrect or invalid review comment.
The link speed was previously only accessible for port interfaces via the port-specific info structure. This made it impossible to query the aggregated speed of bond interfaces or to propagate speed information to VLAN subinterfaces. Move the link_speed field from the port-specific structure to the base interface structure. Port interfaces fetch the physical link speed from DPDK as before. Bond interfaces aggregate member speeds (active member in active-backup mode, sum of active members in LACP mode). VLAN subinterfaces inherit their parent speed. Loopback and IPIP tunnels report a fixed 10 Gb/s speed. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
Use frr_global_options to apply logging configuration to all FRR daemons instead of configuring each daemon separately. This ensures that any daemon started by FRR (not just zebra and bgpd) will have logging enabled, making it easier to debug issues in smoke tests. Move the common options (-A 127.0.0.1 --log file:$flog) to frr_global_options and keep only daemon-specific options in zebra_options (socket size and dplane module). Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/cli/bond.c (1)
35-43: Use consistent sentinel value for unknown speed across control and CLIThe control plane sets
iface->speed = RTE_ETH_SPEED_NUM_UNKNOWN(which is 0 in DPDK), but the CLI checksmember->speed == UINT32_MAX. These values don't match, so the check always fails and unknown speeds display as "0 Mb/s" instead of "unknown".Define a shared
GR_IFACE_SPEED_UNKNOWNconstant inmodules/infra/api/gr_infra.hand use it in bothmodules/infra/control/iface.candmodules/infra/cli/bond.c(line 39).
🧹 Nitpick comments (1)
modules/infra/control/iface.c (1)
16-16: Speed init and subinterface propagation look good; consider speed-only change handlingInitializing
iface->speedto an explicit UNKNOWN value and mirroring parent speed to subinterfaces on STATUS_UP / clearing it on STATUS_DOWN is coherent with the rest of the infra. One thing to be aware of: if a parent’s speed changes while it remains RUNNING (e.g., bond LACP member set changes but at least one member stays active), subinterfaces’s->speedwon’t be updated here because no new STATUS_UP/DOWN event is emitted. If you care about subinterfaces always reflecting the current aggregate speed, you may want a dedicated “speed changed” signal or to reuse an existing event that’s guaranteed to fire on speed changes.Also applies to: 110-112, 515-528
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
modules/infra/api/gr_infra.h(1 hunks)modules/infra/cli/bond.c(1 hunks)modules/infra/cli/iface.c(1 hunks)modules/infra/cli/port.c(0 hunks)modules/infra/control/bond.c(4 hunks)modules/infra/control/iface.c(3 hunks)modules/infra/control/loopback.c(2 hunks)modules/infra/control/port.c(1 hunks)modules/infra/control/vlan.c(1 hunks)modules/ipip/control.c(1 hunks)smoke/_init.sh(1 hunks)smoke/_init_frr.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/cli/port.c
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/infra/control/port.c
- modules/infra/control/vlan.c
- modules/ipip/control.c
- smoke/_init_frr.sh
- modules/infra/api/gr_infra.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.sh
**/*.{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/infra/cli/bond.cmodules/infra/cli/iface.cmodules/infra/control/bond.cmodules/infra/control/loopback.cmodules/infra/control/iface.c
🧠 Learnings (5)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
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:
smoke/_init.shmodules/infra/control/iface.c
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/infra/control/loopback.c
📚 Learning: 2025-10-02T07:42:42.135Z
Learnt from: rjarry
Repo: DPDK/grout PR: 326
File: modules/infra/control/graph.c:360-364
Timestamp: 2025-10-02T07:42:42.135Z
Learning: In the grout codebase, `gr_vec_free()` is a macro defined in `main/gr_vec.h` that automatically sets the vector pointer to NULL after freeing the memory, preventing double-free issues. The macro definition is: `#define gr_vec_free(v) ((v) ? free(__gr_vec_hdr(v)) : (void)0, (v) = NULL)`.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-10-08T21:22:45.922Z
Learnt from: rjarry
Repo: DPDK/grout PR: 334
File: modules/infra/control/worker.c:278-281
Timestamp: 2025-10-08T21:22:45.922Z
Learning: In the codebase, `gr_vec_add` is a macro that does not return any value and cannot fail. Do not suggest checking its return value or adding error handling around it.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-10-21T15:42:43.874Z
Learnt from: rjarry
Repo: DPDK/grout PR: 350
File: modules/ip/control/address.c:214-216
Timestamp: 2025-10-21T15:42:43.874Z
Learning: In C code compiled with `-std=gnu2x`, the gr_vec_foreach macro supports inline variable declarations (e.g., `gr_vec_foreach (struct nexthop *nh, vector)`). This is valid C2x syntax and does not require pre-declaring the loop variable.
Applied to files:
modules/infra/control/iface.c
🧬 Code graph analysis (1)
modules/infra/control/loopback.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
🔇 Additional comments (3)
modules/infra/control/loopback.c (1)
17-17: Loopback fixed‑speed initialization looks correctSetting loopback interfaces to a fixed 10 Gb/s at init time aligns with the PR’s semantics and ensures iface->speed is populated before events/CLI consumers see the iface; no extra handling seems needed here.
Also applies to: 262-265
modules/infra/control/bond.c (1)
233-309: Bond speed aggregation logic matches the intended semanticsThe updated
bond_update_active_memberscorrectly sets bond speed to the active member’s speed in active‑backup mode and to the sum of active members’ speeds in LACP mode (ignoring UNKNOWN members and falling back to UNKNOWN when the aggregate is zero). Updatingiface->speedbefore emitting STATUS_UP/STATUS_DOWN ensures subscribers (e.g., VLAN subinterfaces) see a consistent speed value.modules/infra/cli/iface.c (1)
467-470: No change needed.RTE_ETH_SPEED_NUM_UNKNOWN is defined as UINT32_MAX, so the CLI check at line 467 correctly matches the control plane sentinel. The code is sound as-is.
Likely an incorrect or invalid review comment.
| frr_global_options="-A 127.0.0.1 --log file:$flog" | ||
| zebra_options="-s 90000000 -M dplane_grout" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify frrinit.sh integration with frr_global_options.
The new frr_global_options variable is non-standard for FRR daemons files and requires that frrinit.sh explicitly handles this variable when launching daemons. Confirm that the initialization script has been updated to apply these global options correctly.
🏁 Script executed:
#!/bin/bash
# Verify that frrinit.sh recognizes and applies frr_global_options
# Search for frr_global_options handling in frrinit.sh
rg -n 'frr_global_options' --type shLength of output: 152
🏁 Script executed:
#!/bin/bash
# Locate frrinit.sh and verify it handles frr_global_options from daemons config
# Find frrinit.sh
fd 'frrinit\.sh' -t f
# Search for how daemons config is sourced/used
rg -n 'source.*daemons|\..*daemons|frr_global_options' --type sh -C 2Length of output: 775
🏁 Script executed:
#!/bin/bash
# Search for how daemons config is actually used
# Look for references to the daemons file or FRR startup
rg -n 'daemons' --type sh -C 1 | head -50
# Search for how daemon options are applied
rg -n 'zebra_options|bgpd_options' --type sh -C 2Length of output: 1415
🏁 Script executed:
#!/bin/bash
# Check if there's any FRR documentation or scripts that handle frr_global_options
# and look for any sourcing of the daemons file
rg -n 'source|^\.' --type sh | grep -E 'daemons|frr' | head -20
# Look for any references to how options are applied to daemons
rg -n 'frr.*start|daemon.*start|exec.*frr' --type sh -C 2Length of output: 1488
🏁 Script executed:
#!/bin/bash
# Check FRR version/requirements and whether frr_global_options is documented
fd 'frr' -t f | grep -iE 'readme|doc|version|require' | head -10
# Look for any FRR documentation in the repo
find . -type f \( -name '*frr*' -o -name 'README*' \) | head -20Length of output: 389
🏁 Script executed:
#!/bin/bash
# Check FRR version and configuration documentation
cat subprojects/frr.wrap 2>/dev/null || echo "No frr.wrap found"
# Check if frr_global_options is mentioned in documentation
rg -n 'frr_global_options|global_options' . --type md --type txt
# Look for patches or FRR modifications
find . -type f -name '*.patch' | xargs grep -l 'global_options' 2>/dev/null || echo "No patches found"Length of output: 188
FRR 10.4.1 does not support frr_global_options in the daemons file format.
Standard FRR only recognizes per-daemon options (e.g., zebra_options, bgpd_options). The frr_global_options variable written to the daemons config will be silently ignored unless:
- A custom patch to FRR is applied (not found in this repo)
- Per-daemon options are used instead
Verify this configuration actually applies the intended global flags to FRR daemons, or refactor to use standard FRR option syntax.
🤖 Prompt for AI Agents
In smoke/_init.sh around lines 227-228, the script sets frr_global_options which
FRR 10.4.1 does not recognize; remove the unsupported frr_global_options
assignment and instead apply the intended flags (e.g., -A 127.0.0.1 and --log
file:$flog) to each daemon’s options (append them to zebra_options,
bgpd_options, ospfd_options, etc. as needed), then verify the resulting
per-daemon variables are written to the daemons file so the flags actually take
effect.
2 participants
The link speed was previously only accessible for port interfaces via the port-specific info structure. This made it impossible to query the aggregated speed of bond interfaces or to propagate speed information to VLAN subinterfaces.
Move the link_speed field from the port-specific structure to the base interface structure. Port interfaces fetch the physical link speed from DPDK as before. Bond interfaces aggregate member speeds (active member in active-backup mode, sum of active members in LACP mode). VLAN subinterfaces inherit their parent speed. Loopback and IPIP tunnels report a fixed 10 Gb/s speed.
Summary by CodeRabbit
New Features
Changes
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.