Add GARP and unsolicited NDP support #350
Conversation
📝 WalkthroughWalkthroughIntroduces NH_LOCAL_ADDR_FLAGS and replaces local/static nexthop flag compositions and checks across modules. Adds interface-up event handling to trigger ARP/NDP solicit/advertise actions. Distinguishes GARP behavior in ARP solicit logic (skips timing/probe updates and forces broadcast). Refactors NDP NA output to use a new public nh6_advertise API and moves ndp_na_output work onto a control-posted handler. Splits port reconfiguration into per-attribute updates with a needs_configure path that stops, reconfigures, and restarts ports as needed. Changes several datapath logging calls from LOG to RTE_LOG with adjusted formatting. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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: 3
🧹 Nitpick comments (7)
modules/ip6/datapath/gr_ip6_datapath.h (1)
35-35: Document nh6_advertise() contract (remote may be NULL).Impl asserts only on local; remote isn’t asserted and can be NULL for unsolicited NA. Clarify in the header to prevent misuse.
Apply this comment above the prototype:
+// Advertise an IPv6 Neighbor Advertisement for a local address. +// - local: required, GR_NH_T_L3 +// - remote: optional (may be NULL for unsolicited NA) int nh6_advertise(const struct nexthop *local, const struct nexthop *remote);modules/infra/control/gr_nh_control.h (1)
129-130: Good centralization; add a tiny helper to avoid mask repeats.Macro LGTM. Consider a helper to standardize checks and cut typos.
#define NH_LOCAL_ADDR_FLAGS (GR_NH_F_LOCAL | GR_NH_F_LINK | GR_NH_F_STATIC) +static inline bool nh_is_local_addr(const struct nexthop_info_l3 *l3) { + return (l3->flags & NH_LOCAL_ADDR_FLAGS) == NH_LOCAL_ADDR_FLAGS; +}modules/infra/datapath/trace.c (2)
269-271: Fix NA format string wording.“is at (solicited=%u)” reads odd since MAC is printed later via options. Drop “is at”.
- SAFE_BUF(snprintf, len, "neigh advert %s is at (solicited=%u)", dst, na->solicited); + SAFE_BUF(snprintf, len, "neigh advert %s (solicited=%u)", dst, na->solicited);
502-506: Unify logging macro use.You switched to RTE_LOG for the normal path; mirror it on error for consistency.
- LOG(ERR, "[%s %s] snprintf failed: %s", node, iface, strerror(errno)); + RTE_LOG(ERR, GROUT, "[%s %s] snprintf failed: %s\n", node, iface, strerror(errno));modules/ip/datapath/arp_output_request.c (1)
112-118: Correct ARP encoding for GARPs; add explicit header include.
- THA=00:00:00:00:00:00 for GARPs and broadcast L2 dst — correct.
- Minor: include the header that defines NH_LOCAL_ADDR_FLAGS to avoid transitive include surprises.
#include <gr_ip4_datapath.h> +#include <gr_nh_control.h>Also applies to: 126-127
modules/ip/control/address.c (2)
98-100: Flags via NH_LOCAL_ADDR_FLAGS — good; make include explicit.Avoid relying on indirect includes for the macro.
#include <gr_ip4_datapath.h> +#include <gr_nh_control.h>
207-216: GARP on interface up — correct trigger.Iterating local nexthops and calling arp_output_request_solicit() is the right place. Consider a small delay/backoff if certain drivers flap UP quickly.
Do we see GR_EVENT_IFACE_STATUS_UP flapping on any target NICs? If yes, I can add a simple debounce.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
modules/infra/api/nexthop.c(1 hunks)modules/infra/control/gr_nh_control.h(1 hunks)modules/infra/control/port.c(1 hunks)modules/infra/datapath/drop.c(1 hunks)modules/infra/datapath/trace.c(3 hunks)modules/ip/control/address.c(5 hunks)modules/ip/datapath/arp_output_request.c(5 hunks)modules/ip6/control/address.c(6 hunks)modules/ip6/control/nexthop.c(1 hunks)modules/ip6/datapath/gr_ip6_datapath.h(1 hunks)modules/ip6/datapath/ndp_na_output.c(4 hunks)
🧰 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/infra/datapath/trace.cmodules/infra/datapath/drop.cmodules/ip6/datapath/gr_ip6_datapath.hmodules/ip/datapath/arp_output_request.cmodules/infra/control/port.cmodules/infra/api/nexthop.cmodules/ip6/control/nexthop.cmodules/ip6/control/address.cmodules/ip/control/address.cmodules/infra/control/gr_nh_control.hmodules/ip6/datapath/ndp_na_output.c
🧬 Code graph analysis (7)
modules/ip6/datapath/gr_ip6_datapath.h (1)
modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-41)
modules/ip/datapath/arp_output_request.c (2)
api/gr_clock.h (1)
gr_clock_us(17-20)modules/ip/control/address.c (1)
addr4_get_preferred(48-63)
modules/infra/control/port.c (1)
modules/infra/control/worker.c (1)
port_unplug(145-182)
modules/ip6/control/nexthop.c (2)
modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-41)modules/ip6/control/gr_ip6_control.h (1)
nh6_lookup(18-21)
modules/ip6/control/address.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-41)
modules/ip/control/address.c (2)
modules/ip/datapath/arp_output_request.c (1)
arp_output_request_solicit(33-51)main/event.c (1)
gr_event_subscribe(14-16)
modules/ip6/datapath/ndp_na_output.c (3)
api/gr_errno.h (1)
errno_set(9-12)modules/infra/datapath/control_input.c (2)
post_to_stack(45-54)gr_control_input_register_handler(36-43)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). (8)
- 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-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- 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: rpm
- GitHub Check: deb
🔇 Additional comments (18)
modules/infra/datapath/drop.c (1)
14-14: LGTM! Logging standardization looks good.The migration to
RTE_LOGwith the GROUT domain is correct. The "drop" prefix and trailing newline improve log clarity and follow standard practices.modules/infra/api/nexthop.c (1)
86-88: LGTM — consistent flag check via NH_LOCAL_ADDR_FLAGS.modules/infra/datapath/trace.c (1)
113-115: GARP announce formatting: nice touch.Detecting tip==sip and printing an announce line improves readability. LGTM.
modules/ip/datapath/arp_output_request.c (2)
37-48: GARP path shouldn’t bump probe counters — good.Skipping last_request/ucast/bcast for local-address GARPs is correct. LGTM.
84-93: Local nh selection for GARP vs. regular ARP is sound.Using the address itself for GARPs and addr4_get_preferred otherwise matches expected behavior.
modules/ip/control/address.c (2)
9-9: Include addition is appropriate.Needed for arp_output_request_solicit decl. LGTM.
255-259: Event subscription wiring — looks good.Subscription registered during constructor; callback signature matches. LGTM.
Also applies to: 272-272
modules/infra/control/port.c (2)
313-316: Early exit mask — fine.Guarding on relevant attrs avoids needless work. LGTM.
317-317: Review identifies legitimate risk: MAC setting may fail on running port for some PMDs.Current code unconditionally unplugs workers, then sets MAC on a potentially running port. Many PMD drivers require port stopped before
rte_eth_dev_default_mac_addr_set(). The suggested refactor correctly defers unplug to queue-reconfig-only path and wraps MAC changes with conditional stop/start.Risk confirmed by code inspection: when only
GR_PORT_SET_MACis set,needs_configurestays false, so port isn't stopped before MAC operation. Unplugging workers doesn't stop the DPDK port itself.modules/ip6/control/address.c (4)
9-9: LGTM: Include added fornh6_advertisedeclaration.
224-224: Good: UsingNH_LOCAL_ADDR_FLAGSconstant for consistency.
389-389: Good: Variable hoisted to function scope for reuse.
474-478: LGTM: Event subscription correctly updated.modules/ip6/datapath/ndp_na_output.c (5)
22-26: LGTM: Context structure for bundling advertisement parameters.
28-28: LGTM: Control input handle stored at file scope.
63-67: LGTM: Context properly extracted and freed.
115-117: LGTM: Registration callback properly implemented.
132-132: LGTM: Register callback hooked up.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modules/ip6/control/address.c (1)
422-428: Missing break; fall‑through into default.Case GR_EVENT_IFACE_STATUS_UP falls through to default. Add break; after the loop.
case GR_EVENT_IFACE_STATUS_UP: addrs = &iface_addrs[iface->id]; gr_vec_foreach (nh, addrs->nh) { if (nh6_advertise(nh, NULL) < 0) LOG(WARNING, "nh6_advertise: %s", strerror(errno)); } + break; default: break;modules/ip6/control/nexthop.c (1)
213-214: Guard nh6_lookup result before calling nh6_advertise.nh6_lookup may return NULL; passing NULL trips the assert in nh6_advertise.
- if (nh6_advertise(nh6_lookup(iface->vrf_id, iface->id, local), nh) < 0) { - LOG(ERR, "nh6_advertise: %s", strerror(errno)); + struct nexthop *local_nh = nh6_lookup(iface->vrf_id, iface->id, local); + if (local_nh == NULL) { + LOG(ERR, "nh6_lookup failed for local address"); + goto free; + } + if (nh6_advertise(local_nh, nh) < 0) { + LOG(ERR, "nh6_advertise: %s", strerror(errno)); goto free; }
🧹 Nitpick comments (2)
modules/infra/control/port.c (1)
364-372: Consider making unplug conditional on reconfiguration needs.MAC-only changes currently trigger
port_unplug(line 317) andport_plug(line 372), even thoughneeds_configureis false and no actual reconfiguration occurs. Sinceport_unplugdisables queues and reloads the worker graph (perworker.c), this adds unnecessary overhead for MAC updates.Consider moving the
port_unplugcall to only execute whenneeds_configureis true, aligning with the PR objective "only unplug/stop if necessary".Apply this diff to optimize the unplug/plug cycle:
if (!(set_attrs & (GR_PORT_SET_N_RXQS | GR_PORT_SET_N_TXQS | GR_PORT_SET_Q_SIZE | GR_PORT_SET_MAC))) return 0; - if ((ret = port_unplug(p)) < 0) - return ret; - if (set_attrs & GR_PORT_SET_N_RXQS) { p->n_rxq = api->n_rxq; needs_configure = true; } if (set_attrs & GR_PORT_SET_N_TXQS) { p->n_txq = api->n_txq; needs_configure = true; } if (set_attrs & GR_PORT_SET_Q_SIZE) { p->rxq_size = api->rxq_size; p->txq_size = api->txq_size; needs_configure = true; } + if (needs_configure && (ret = port_unplug(p)) < 0) + return ret; + if (p->started && needs_configure) { if ((ret = rte_eth_dev_stop(p->port_id)) < 0) return errno_log(-ret, "rte_eth_dev_stop");Then update the final plug to be conditional:
if (!p->started && (ret = rte_eth_dev_start(p->port_id)) < 0) return errno_log(-ret, "rte_eth_dev_start"); p->started = true; - return port_plug(p); + if (needs_configure) + return port_plug(p); + return 0;modules/ip/datapath/arp_output_request.c (1)
84-93: ARP fields for GARPs/non‑GARPs look right; consider a helper for the flag test.
- For GARPs: local=nh, THA zero, L2 dst broadcast — correct.
- For non‑GARPs: prefer local via addr4_get_preferred, unicast when we have THA else broadcast — reasonable.
- Optional: extract “is_garp(local)” into a static inline to centralize the NH_LOCAL_ADDR_FLAGS test.
Also applies to: 112-118, 126-130
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
modules/infra/api/nexthop.c(1 hunks)modules/infra/control/gr_nh_control.h(1 hunks)modules/infra/control/port.c(1 hunks)modules/infra/datapath/drop.c(1 hunks)modules/infra/datapath/trace.c(3 hunks)modules/ip/control/address.c(5 hunks)modules/ip/datapath/arp_output_request.c(5 hunks)modules/ip6/control/address.c(6 hunks)modules/ip6/control/nexthop.c(1 hunks)modules/ip6/datapath/gr_ip6_datapath.h(1 hunks)modules/ip6/datapath/ndp_na_output.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/infra/control/gr_nh_control.h
- modules/ip6/datapath/gr_ip6_datapath.h
- modules/infra/api/nexthop.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/infra/datapath/trace.cmodules/ip/control/address.cmodules/ip6/control/nexthop.cmodules/infra/datapath/drop.cmodules/ip/datapath/arp_output_request.cmodules/ip6/control/address.cmodules/infra/control/port.cmodules/ip6/datapath/ndp_na_output.c
🧬 Code graph analysis (6)
modules/ip/control/address.c (2)
modules/ip/datapath/arp_output_request.c (1)
arp_output_request_solicit(33-51)main/event.c (1)
gr_event_subscribe(14-16)
modules/ip6/control/nexthop.c (2)
modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-44)modules/ip6/control/gr_ip6_control.h (1)
nh6_lookup(18-21)
modules/ip/datapath/arp_output_request.c (2)
api/gr_clock.h (1)
gr_clock_us(17-20)modules/ip/control/address.c (1)
addr4_get_preferred(48-63)
modules/ip6/control/address.c (2)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-44)
modules/infra/control/port.c (1)
modules/infra/control/worker.c (1)
port_unplug(145-182)
modules/ip6/datapath/ndp_na_output.c (2)
api/gr_errno.h (1)
errno_set(9-12)modules/infra/datapath/control_input.c (2)
post_to_stack(45-54)gr_control_input_register_handler(36-43)
⏰ 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). (8)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: rpm
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
🔇 Additional comments (15)
modules/infra/datapath/drop.c (1)
14-14: LGTM!Logging backend migration is correct and consistent with the project-wide changes.
modules/infra/datapath/trace.c (3)
113-114: LGTM!GARP detection using
tip == sipis correct and the formatting appropriately distinguishes gratuitous ARP from regular requests.
269-269: LGTM!Adding the solicited flag to neighbor advertisement traces improves diagnostics and aligns with the PR objectives.
502-502: LGTM!Logging backend migration is consistent with project-wide changes and correctly formatted.
modules/infra/control/port.c (4)
313-315: LGTM: Clean early exit.The guard condition correctly skips processing when no relevant attributes are set.
320-332: LGTM: Clear per-attribute handling.The granular updates with
needs_configuretracking are well-structured and align with the PR's goal of selective reconfiguration.
334-338: LGTM: Appropriate conditional stop.Port is stopped only when running and reconfiguration is required, matching the PR objective.
340-362: LGTM: Robust reconfiguration flow.The conditional reconfiguration correctly handles both port updates and initial creation, properly rebuilding the port list and redistributing queues.
modules/ip6/control/address.c (2)
224-224: Good: unify local IPv6 address flags via NH_LOCAL_ADDR_FLAGS.Consistent with new macro; reduces duplication.
474-479: Subscription updated to include IFACE_STATUS_UP.ev_count and ev_types aligned; looks correct.
modules/ip/datapath/arp_output_request.c (1)
38-48: GARP path correctly avoids probe/timing updates.Nice separation; avoids polluting ARP state with gratuitous requests.
modules/ip/control/address.c (2)
98-98: Good: use NH_LOCAL_ADDR_FLAGS for local IPv4 addresses.Keeps semantics consistent with IPv6 changes.
255-259: Interface‑up subscription wiring looks correct.Callback, ev_count, and registration are consistent.
Also applies to: 272-273
modules/ip6/datapath/ndp_na_output.c (2)
30-44: Memory ownership on post_to_stack failure handled.Freeing ctx on error prevents leaks. LGTM.
64-71: Control‑input plumbing looks correct.ctx is consumed and freed; handler registration sets na_output before use.
Also applies to: 118-120, 135-136
The combination GR_NH_F_LOCAL | GR_NH_F_LINK | GR_NH_F_STATIC is used to identify nexthops that correspond to local IP addresses assigned to interfaces. This flag combination is duplicated in three different places with no clear indication of its purpose. Define NH_LOCAL_ADDR_FLAGS to make the code more explicit and ensure consistency. Signed-off-by: Robin Jarry <[email protected]>
When a network interface transitions to the UP state, broadcast gratuitous ARP requests for all IP addresses configured on that interface. This allows the network to immediately update forwarding tables with the correct MAC address for these IP addresses, which is critical in bonding scenarios where the active member may change. A gratuitous ARP request has the source and target IP addresses set to the same value (the interface's own IP) and the target hardware address set to all zeros. These packets are always broadcast. Extend arp_output_request_solicit() to detect local address nexthops using NH_LOCAL_ADDR_FLAGS and skip updating probe counters and timestamps for gratuitous requests, since these are announcements rather than actual neighbor discovery attempts. Signed-off-by: Robin Jarry <[email protected]>
Introduce nh6_advertise() as a public API to send NDP neighbor advertisements from the control plane. This function allocates context data on the heap and posts it to the ndp_na_output datapath node, which constructs and transmits the NDP NA packet. Previously, NDP NA replies were only sent in response to neighbor solicitations by reusing the received mbuf and storing nexthop pointers in mbuf private data. This approach was limited to the solicitation handling path and required the mbuf to remain valid. The new design allows sending unsolicited neighbor advertisements from any control plane context. This is required to announce IPv6 addresses when an interface transitions to the UP state, particularly in bonding scenarios where the active member may change. Move the ndp_na_output control input handler registration to a node register callback since it is no longer needed during module init. Signed-off-by: Robin Jarry <[email protected]>
When a network interface transitions to the UP state, send unsolicited neighbor advertisements for all IPv6 addresses configured on that interface. This allows the network to immediately update forwarding tables with the correct MAC address, which is critical in bonding scenarios where the active member may change. Unsolicited neighbor advertisements are sent by calling nh6_advertise() with the local address nexthop and NULL for the remote parameter. These packets are sent to the all-nodes multicast address (ff02::1) and have the solicited flag cleared. This mirrors the gratuitous ARP functionality implemented for IPv4 addresses. Signed-off-by: Robin Jarry <[email protected]>
When reconfiguring an interface port, port_unplug() was called unconditionally even when the requested changes did not require it. Add an early return if none of the port-specific attributes are being modified (number of RX/TX queues, queue sizes, or MAC address). This allows changing interface flags and MTU without unplugging the port. Signed-off-by: Robin Jarry <[email protected]>
The LOG() macro automatically prepends the function name to log messages. Switch to RTE_LOG() in datapath packet logging to avoid cluttering the output with function names that add no useful context. Signed-off-by: Robin Jarry <[email protected]>
Gratuitous ARP requests have the source and target IP addresses set to the same value. Detect this condition and format these packets as "announce X is at MAC" instead of "request who has X? tell X" to make the trace output clearer. Signed-off-by: Robin Jarry <[email protected]>
Include the solicited flag value in NDP neighbor advertisement trace output. This flag distinguishes between advertisements sent in response to neighbor solicitations (solicited=1) and unsolicited announcements broadcast when an interface comes up (solicited=0). Signed-off-by: Robin Jarry <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/ip6/datapath/gr_ip6_datapath.h (1)
21-29: ip6_local_mbuf_data missingifacefield used by NA output.ndp_na_output.c assigns d->iface, but the struct here has no such member, risking memory corruption.
Apply in this header:
GR_MBUF_PRIV_DATA_TYPE(ip6_local_mbuf_data, { + const struct iface *iface; struct rte_ipv6_addr src; struct rte_ipv6_addr dst; uint16_t len; uint16_t ext_offset; uint8_t hop_limit; uint8_t proto; });Then keep ndp_na_output.c writing d->iface as it does now. Based on learnings
modules/ip6/datapath/ndp_na_output.c (1)
92-95: ip6_local_mbuf_data write uses nonexistentifacemember.This will smash adjacent fields unless ip6_local_mbuf_data includes
const struct iface *iface;. Add that field in gr_ip6_datapath.h.Apply header change:
GR_MBUF_PRIV_DATA_TYPE(ip6_local_mbuf_data, { + const struct iface *iface; struct rte_ipv6_addr src; struct rte_ipv6_addr dst; uint16_t len; uint16_t ext_offset; uint8_t hop_limit; uint8_t proto; });No code change needed here after the header update.
♻️ Duplicate comments (3)
modules/ip6/control/address.c (1)
422-429: Iface-up NA advertise handler looks good (break fixed).Iterates and logs failures; the missing break noted earlier is now present.
modules/ip6/datapath/ndp_na_output.c (1)
30-44: Failure-path free added — good catch.ctx is freed when post_to_stack fails; leak resolved.
modules/ip/control/address.c (1)
207-216: Fix gr_vec_foreach variable declaration (won’t compile as-is).Declare the loop variable separately; pass only the identifier to gr_vec_foreach.
Apply:
static void iface_up_cb(uint32_t /*event*/, const void *obj) { const struct iface *iface = obj; struct hoplist *ifaddrs = addr4_get_all(iface->id); if (ifaddrs == NULL) return; - gr_vec_foreach (struct nexthop *nh, ifaddrs->nh) - arp_output_request_solicit(nh); + struct nexthop *nh; + gr_vec_foreach (nh, ifaddrs->nh) + arp_output_request_solicit(nh); }
🧹 Nitpick comments (3)
modules/ip/control/address.c (1)
207-216: Log ARP solicit failures on iface up.Avoid silent failures when posting GARP/solicit.
Apply:
- gr_vec_foreach (nh, ifaddrs->nh) - arp_output_request_solicit(nh); + gr_vec_foreach (nh, ifaddrs->nh) { + int r = arp_output_request_solicit(nh); + if (r < 0) + LOG(WARNING, "arp_output_request_solicit: %s", strerror(errno)); + }modules/ip/datapath/arp_output_request.c (2)
38-48: GARP gating is fine; invert the branch to remove the empty then-block.Small clarity tweak: update probes/timestamps only for non‑GARP to avoid an empty GARP block.
- if ((l3->flags & NH_LOCAL_ADDR_FLAGS) == NH_LOCAL_ADDR_FLAGS) { - // GARP request - } else { + if (!((l3->flags & NH_LOCAL_ADDR_FLAGS) == NH_LOCAL_ADDR_FLAGS)) { // This function is called by the control plane main thread. // It is OK to modify the nexthop here. l3->last_request = gr_clock_us(); if (l3->ucast_probes < nh_conf.max_ucast_probes) l3->ucast_probes++; else l3->bcast_probes++; + } else { + // GARP: skip probe/timing accounting }
112-118: Decouple ARP THA from L2 destination; keep THA zero unless unicast‑probe.Current code sets THA to ff:ff:ff:ff:ff:ff as a placeholder when unknown, then reuses it as the L2 destination. That works but isn’t ideal for ARP requests (THA should be 0 when unknown). Suggest: keep THA zero unless we have a cached MAC; choose L2 dst based on last_reply/probe state directly.
- if (is_garp) - memset(&arp->arp_data.arp_tha, 0, sizeof(arp->arp_data.arp_tha)); - else if (l3->last_reply != 0) - arp->arp_data.arp_tha = l3->mac; - else - memset(&arp->arp_data.arp_tha, 0xff, sizeof(arp->arp_data.arp_tha)); + if (is_garp) { + memset(&arp->arp_data.arp_tha, 0, sizeof(arp->arp_data.arp_tha)); + } else if (l3->last_reply != 0) { + // Unicast ARP probe: reflect the known target MAC in THA. + arp->arp_data.arp_tha = l3->mac; + } else { + // Unknown target MAC: keep THA zero as per common ARP request practice. + memset(&arp->arp_data.arp_tha, 0, sizeof(arp->arp_data.arp_tha)); + }- if (l3->bcast_probes == 0 && !is_garp) - eth_data->dst = arp->arp_data.arp_tha; + if (!is_garp && l3->bcast_probes == 0 && l3->last_reply != 0) + eth_data->dst = l3->mac; // first: unicast probe to cached MAC else memset(ð_data->dst, 0xff, sizeof(eth_data->dst));Also applies to: 126-129
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
modules/infra/api/nexthop.c(1 hunks)modules/infra/control/gr_nh_control.h(1 hunks)modules/infra/control/port.c(1 hunks)modules/infra/datapath/drop.c(1 hunks)modules/infra/datapath/trace.c(3 hunks)modules/ip/control/address.c(5 hunks)modules/ip/datapath/arp_output_request.c(5 hunks)modules/ip6/control/address.c(6 hunks)modules/ip6/control/nexthop.c(1 hunks)modules/ip6/datapath/gr_ip6_datapath.h(1 hunks)modules/ip6/datapath/ndp_na_output.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/infra/control/gr_nh_control.h
- modules/ip6/control/nexthop.c
- modules/infra/datapath/drop.c
- modules/infra/datapath/trace.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/ip6/datapath/gr_ip6_datapath.hmodules/ip/control/address.cmodules/ip6/datapath/ndp_na_output.cmodules/ip/datapath/arp_output_request.cmodules/ip6/control/address.cmodules/infra/control/port.cmodules/infra/api/nexthop.c
🧬 Code graph analysis (6)
modules/ip6/datapath/gr_ip6_datapath.h (1)
modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-44)
modules/ip/control/address.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-22)modules/ip/datapath/arp_output_request.c (1)
arp_output_request_solicit(33-51)main/event.c (1)
gr_event_subscribe(14-16)
modules/ip6/datapath/ndp_na_output.c (2)
api/gr_errno.h (1)
errno_set(9-12)modules/infra/datapath/control_input.c (2)
post_to_stack(45-54)gr_control_input_register_handler(36-43)
modules/ip/datapath/arp_output_request.c (2)
api/gr_clock.h (1)
gr_clock_us(17-20)modules/ip/control/address.c (1)
addr4_get_preferred(48-63)
modules/ip6/control/address.c (1)
modules/ip6/datapath/ndp_na_output.c (1)
nh6_advertise(30-44)
modules/infra/control/port.c (1)
modules/infra/control/worker.c (1)
port_unplug(145-182)
⏰ 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). (8)
- GitHub Check: rpm
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: deb
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
🔇 Additional comments (17)
modules/infra/control/port.c (5)
313-315: LGTM: Early exit optimization.Efficiently avoids unnecessary operations when none of the relevant port attributes are being modified.
320-332: LGTM: Clean separation with needs_configure flag.The needs_configure flag clearly distinguishes queue reconfigurations (which require stop/restart) from simple attribute updates. Well-structured improvement.
364-370: Similar concern: MAC set failure after port stop.If the port was stopped for reconfiguration (line 335) and
port_mac_setfails (line 365), the port remains stopped. This is consistent with the reconfiguration failure behavior but should be verified as intended.
317-318: LGTM: Balanced unplug/plug operations.The
port_unplugat line 317 andport_plugat line 372 properly bracket the reconfiguration work, ensuring worker graphs are updated correctly.Also applies to: 372-372
334-362: Error handling is appropriate; port remains stopped by design.On
port_configure()orworker_queue_distribute()failure, the function returns early with the port stopped. This is intentional—the stopped state signals the failure, and the caller handles cleanup viaiface_port_fini()(line 465). In other contexts like worker.c, ports are restarted on reconfiguration failure, but here the initialization/reconfiguration path properly delegates cleanup to the caller. No issue found.modules/infra/api/nexthop.c (1)
86-88: Standardized local-addr flag check looks good.Switch to NH_LOCAL_ADDR_FLAGS is correct and keeps semantics intact.
modules/ip/control/address.c (4)
9-9: Include addition is appropriate.gr_ip4_datapath.h is relevant for RCU sync usage below.
98-100: Consistent flag composition.Using NH_LOCAL_ADDR_FLAGS improves consistency across modules.
255-259: Event subscription addition is correct.GR_EVENT_IFACE_STATUS_UP hook is registered properly.
272-272: Constructor subscribes iface-up event.Good to register during init alongside existing subscriptions.
modules/ip6/datapath/gr_ip6_datapath.h (1)
35-36: Public nh6_advertise API declaration is fine.Matches usage in control plane; no concerns here.
modules/ip6/control/address.c (3)
9-9: Include addition is appropriate.Required for nh6_advertise and related types.
224-226: Consistent flag composition.NH_LOCAL_ADDR_FLAGS keeps semantics aligned with IPv4.
475-480: Subscription updated correctly.ev_count and ev_types reflect the new STATUS_UP event.
modules/ip6/datapath/ndp_na_output.c (1)
118-121: Registration pathway looks correct.Handler is registered via register_callback before node usage; keep it as-is.
Please confirm nh6_advertise is not called before ndp_na_output_register runs at startup (e.g., via early events). If unsure, I can script a repo scan for early calls.
Also applies to: 135-136
modules/ip/datapath/arp_output_request.c (2)
66-66: LGTM: local is_garp flag introduction.Keeps the process path simple and avoids repeated flag checks.
84-93: Confirm whether silent fallback to first address is acceptable for ARP SPA selection.Your concern is valid:
addr4_get_preferred()(modules/ip/control/address.c:62) returnsaddrs->nh[0]when no same-subnet address exists, not NULL. The check at line 89 won't catch this fallback—the returned address may not match the target subnet.Decide if this behavior aligns with your ARP policy. If not, add a
ip4_addr_same_subnet()validation after line 88 and treat mismatches as ERROR.
1 participant
Summary by CodeRabbit
Bug Fixes
Performance Improvements
Refactor