lacp fixes #399

rjarry · 2025-11-19T14:11:09Z

bond: fix init with lacp mode
port: silently ignore multicast mac filters
lacp: only use tcp/udp ports for non-fragmented packets

Summary by CodeRabbit

Bug Fixes
- Simplified bond initialization and cleanup by removing unnecessary LACP MAC configuration steps.
- Improved MAC handling to return errors for null inputs and gracefully ignore multicast addresses without changing filters.
- Enhanced IPv4 fragmented-packet handling so transport ports are considered only for non-fragmented packets.

coderabbitai · 2025-11-19T14:16:14Z

📝 Walkthrough

Walkthrough

This pull request updates three modules. In bond control, code that added or removed the LACP destination MAC on member interfaces during initialization and cleanup was removed. In port control, MAC validation was split: NULL MACs return EINVAL; multicast MACs return success without modifying filters. In bond output, IPv4 UDP/TCP transmit hashing ignores transport ports for fragmented packets (fragment_offset != 0).

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'lacp fixes' is vague and generic, lacking specificity about what aspect of LACP is being fixed or what the primary changes address.	Clarify the title to specify the main fix, such as 'Fix LACP initialization with multicast MAC handling' or reference the primary objective.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 465705d and d72047f.

📒 Files selected for processing (3)

modules/infra/control/bond.c (0 hunks)
modules/infra/control/port.c (2 hunks)
modules/infra/datapath/bond_output.c (1 hunks)

💤 Files with no reviewable changes (1)

modules/infra/control/bond.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 sets x = NULL. There is no risk of double free.

ec_node_*() functions consume their ec_node arguments. No leaks on error.

rte_node->ctx is 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/bond_output.c
modules/infra/control/port.c

🧠 Learnings (4)

📚 Learning: 2025-09-09T09:22:31.596Z

Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.

Applied to files:

modules/infra/datapath/bond_output.c

📚 Learning: 2025-09-09T06:02:47.703Z

Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: The DPDK function rte_ipv6_get_next_ext() only reads 2 bytes from the extension header, so a 2-byte precheck is sufficient before calling it.

Applied to files:

modules/infra/datapath/bond_output.c

📚 Learning: 2025-09-09T07:36:34.893Z

Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T07:36:34.893Z
Learning: In the SRv6 routing header structure, the flag and tag fields are used for per-flow metadata and Group-Based Policy use cases. Only 5 bytes are needed to access the basic SRv6 fields: next_hdr, hdr_len, type, segments_left, and last_entry.

Applied to files:

modules/infra/datapath/bond_output.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/datapath/bond_output.c

🧬 Code graph analysis (1)

modules/infra/control/port.c (1)

api/gr_errno.h (1)

errno_set (9-12)

🔇 Additional comments (3)

modules/infra/datapath/bond_output.c (2)

107-119: LGTM: Correct handling of UDP ports for IP fragments.

The fragment_offset check ensures that UDP ports are only used for non-fragmented packets, preventing fragments from being distributed to different bond members.

122-134: LGTM: Correct handling of TCP ports for IP fragments.

The fragment_offset check ensures that TCP ports are only used for non-fragmented packets, preventing fragments from being distributed to different bond members.

modules/infra/control/port.c (1)

487-490: LGTM: Correct separation of NULL and multicast MAC validation.

The split validation correctly distinguishes between invalid parameters (NULL → EINVAL) and valid no-ops (multicast → success, since ALLMULTI is always enabled).

Also applies to: 562-565

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

aharivel

Overall good just a nit-pick on the offset flag.
Also, in your commit log you say "Removes LACP multicast MAC filter operations " and
Fixes: f2b09727b0b8 because it adds LACP support right. But it should more fix the commit ec4cbdc7f18f that add the multicast filtering no ?

aharivel · 2025-11-19T14:49:52Z

modules/infra/datapath/bond_output.c

-				);
-				tuple.v4.sport = l4.udp->src_port;
-				tuple.v4.dport = l4.udp->dst_port;
+				if (l3.ip4->fragment_offset == 0) {


The fragment_offset field includes both flags (upper 3 bits) and offset (lower 13 bits). Maybe we should be more clearer on what we are testing by using something like:

l3.ip4->fragment_offset & RTE_BE16(RTE_IPV4_HDR_OFFSET_MASK | RTE_IPV4_HDR_MF_FLAG)) == 0

Yes, I want to consider these flags as well. The first fragment will have offset=0 but will have MF=1 so l3.ip4->fragment_offset != 0 and the ports will not be considered. Otherwise, it would mean subsequent fragments would be output on different bond members.

Hum: I may disagree this completely closes #397.
While we will have all fragments of a packet on the same link, all packets from the same flow may not be sent on the same link as non-fragmented and fragmented packets will have a different hash.
1st packet: < MTU: we use L4 info to hash --> we will use the info.
2nd packet fragmented --> all packets will have a different hash, ending in a different hash. This packet may be sent on another link.
Did I miss something ?

I don't think this is critical, but may be worth creating a bug. WDYT ?

The issue is that there is no way to guarantee these small and fragmented packets will go through the same output member.

The hash computation is completely stateless:

First packet is transmitted, we compute the hash on a non-fragment. We can use TCP/UDP ports. We get output port 2.

Second packet is from the same flow, but too big so it gets fragmented. First fragment gets transmitted, we compute the hash but since it is a fragment, we ignore the TCP/UDP ports. Hash is different: maybe we get a different output port (no guarantee).

Second fragment is transmitted, it gets the same hash as the first fragment. It gets outputted on the same port as the first fragment.

The only way to solve this issue would be to reassemble but that defeats the purpose of fragmenting the packet in the first place

Honnestly, I don't think it is a critical issue and since it is not possible to fix it (unless I am mistaken), it may be just worth a comment to indicate that caveat.

What do you think?

rjarry · 2025-11-19T15:39:55Z

Also, in your commit log you say "Removes LACP multicast MAC filter operations " and Fixes: f2b09727b0b8 because it adds LACP support right. But it should more fix the commit ec4cbdc7f18f that add the multicast filtering no ?

Well, the LACP commit came after, so it is the one which introduced the issue.

aharivel

Ok LGTM

Since commit ec4cbdc ("port: always enable allmulti and simplify MAC filtering"), filtering multicast addresses is no longer necessary. Allmulti is always enabled. Calling iface_add_eth_addr() with a non-unicast address returns an error. The lacp test fails when doing so: + grcli interface add bond bond0 mode lacp member p0 member p1 member p2 DEBUG: GROUT: bond_init_new_members: adding p0 to bond bond0 ERR: GROUT: bond_init_new_members: iface_add_eth_addr(lacp): Invalid argument (22) error: command failed: Invalid argument (EINVAL) Do not add the LACP multicast address to the port filters. Fixes: f2b0972 ("bond: add lacp support") Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Anthony Harivel <[email protected]>

Since commit ec4cbdc ("port: always enable allmulti and simplify MAC filtering"), ALLMULTI is always on and adding a multicast address to the mac filter of port causes a hard error. This is a bit excessive. When a multicast address is added/removed to/from a port filter, simply ignore it. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Anthony Harivel <[email protected]>

If packets are fragmented, we cannot reliably read the TCP/UDP ports from the layer 4 header, even though ip->proto is TCP/UDP. We would read garbage. If fragment_offset is not 0, ignore the L4 header and assume the ports are 0 like for all other L4 protocols than TCP/UDP. Closes: DPDK#397 Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Anthony Harivel <[email protected]>

rjarry force-pushed the lacp-fixes branch from 7997e73 to 465705d Compare November 19, 2025 14:14

rjarry requested a review from aharivel November 19, 2025 14:22

aharivel suggested changes Nov 19, 2025

View reviewed changes

aharivel self-requested a review November 19, 2025 15:41

aharivel approved these changes Nov 19, 2025

View reviewed changes

rjarry added 3 commits November 19, 2025 15:41

grout-bot force-pushed the lacp-fixes branch from 465705d to d72047f Compare November 19, 2025 15:41

rjarry merged commit e2953be into DPDK:main Nov 19, 2025
7 checks passed

rjarry deleted the lacp-fixes branch November 19, 2025 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

lacp fixes #399

lacp fixes #399

Uh oh!

rjarry commented Nov 19, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 19, 2025 •

edited

Loading

Walkthrough

Uh oh!

aharivel left a comment

Uh oh!

aharivel Nov 19, 2025

Uh oh!

rjarry Nov 19, 2025

Uh oh!

christophefontaine Nov 19, 2025

Uh oh!

rjarry Nov 19, 2025

Uh oh!

rjarry commented Nov 19, 2025

Uh oh!

aharivel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lacp fixes #399

lacp fixes #399

Uh oh!

Conversation

rjarry commented Nov 19, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Pre-merge checks

Uh oh!

aharivel left a comment

Choose a reason for hiding this comment

Uh oh!

aharivel Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

rjarry Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

christophefontaine Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

rjarry Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

rjarry commented Nov 19, 2025

Uh oh!

aharivel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rjarry commented Nov 19, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 19, 2025 •

edited

Loading