[PWCI] "netdev-dpdk, vswitchd: Add speed/duplex/autoneg config for DPDK."#8
Open
[PWCI] "netdev-dpdk, vswitchd: Add speed/duplex/autoneg config for DPDK."#8
Conversation
For vport devices hardware offload counters were being incremented on the physical device but decremented on the vport, causing incorrect statistics that didn't return to zero when flows are removed. Fix by using physdev instead of netdev for counter decrements to match the increment behavior. Fixes: 0e6366c ("netdev-offload-dpdk: Implement hw-offload statistics read.") Signed-off-by: Linda Wang <linda.wang@jaguarmicro.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
The error paths should release any allocated resources that are not added to the patterns. Found by Coverity. Fixes: c1a5d0e ("netdev-offload-dpdk: Support IPv4 fragmentation types.") Fixes: 1318929 ("netdev-offload-dpdk: Support IPv6 fragmentation types.") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> Reviewed-by: David Marchand <david.marchand@redhat.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Coverity reported an issue where the oftable->vacancy_up/down members were read without holding the ofproto_mutex. This patch ensures the mutex is acquired before accessing these members and adds OVS_REQUIRES(ofproto_mutex) to the affected function to enforce proper locking. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Fixes: bab8601 ("Implement Vacancy Events for OFPMP_TABLE_DESC.")
This patch resolves a Coverity-reported issue where a dereference could occur after a null check in ovsdb_monitor_row_update_type(). An additional condition has been added to ensure a safe early exit in ovsdb_monitor_compose_row_update(). Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity reported a possible null dereference of t->reply when handling errors in the "committing" state for "transact" requests. In practice, t->reply is always non-NULL at this point, but static analysis cannot infer this guarantee. Adding ovs_assert(t->reply) documents the invariant for developers and ensures debug builds will catch any misuse. No change in runtime behavior; this only improves code safety and satisfies static analysis. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity reported a possible null dereference of ctx->db inside the event processing loop in ovsdb_relay_run(). In practice, ctx->db is guaranteed to be non-NULL for valid relay_ctx structures. Adding ovs_assert(ctx->db) documents this invariant and ensures debug builds catch any misuse. No change in runtime behavior; this only improves code safety and satisfies static analysis. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity flagged a potential null dereference of txnid when calling raft_next_entry() in ovsdb_storage_read(). To prevent this, raft_next_entry() now checks whether txnid is non-NULL before attempting to write to it. This preserves existing behavior when txnid is valid, while safely handling the case where it is NULL. No change to normal operation; this only prevents potential crashes if a NULL txnid is passed. Acked-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Coverity reported a possible null dereference of datum->values when handling value-type weak references in find_and_add_weak_refs(). In practice, datum->values must be non-NULL for value-type weak references. Adding ovs_assert(datum->values) documents this invariant and ensures that we catch any unexpected misuse. No change to runtime behavior in production builds; this only improves safety and satisfies static analysis. Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
We're not installing anything from FreeBSD-kmods or any other non-main
repository, but it seems to have mismatching packages with the latest
Google Cloud image that appears to not be updated yet:
# pkg update -f
Updating FreeBSD repository catalogue...
Fetching meta.conf: . done
Fetching data.pkg: .......... done
Processing entries: .......... done
FreeBSD repository update completed. 36502 packages processed.
Updating FreeBSD-kmods repository catalogue...
Fetching meta.conf: . done
Fetching data.pkg: .. done
Processing entries:
Newer FreeBSD version for package parallels-tools:
To ignore this error set IGNORE_OSVERSION=yes
- package: 1403506
- running userland: 1403505
Ignore the mismatch and continue? [y/N]:
pkg: repository FreeBSD-kmods contains packages for wrong OS version:
FreeBSD:14:amd64
Processing entries...
Unable to update repository FreeBSD-kmods
Error updating repositories!
Not updating FreeBSD-kmods should avoid this issue. In the worst case
the next 'pkg install' will fail, but it shouldn't really ever happen
as we do not install any kernel modules or packages that depend on
any kernel modules.
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
When adjusting bond parameters, any adjustment is considered sufficient for triggering a rebalance. This is a very simplistic config update scheme that triggers a complete rebalance even if the time adjustment would move the next expiration out beyond the last calculated expiration. For the interval parameter only, we can simply recalculate the expiry deadline and let the next bond_run() event do the rebalance if needed. Even if the recalculation would cause the deadline to have occurred in the past, it should execute on the next bond_run() anyway. This is still okay, as the rebalance interval timeout may not result in a full rebalance anyway. Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2025-April/053588.html Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Aaron Conole <aconole@redhat.com>
This patch adds OVSDB schema and netdev-dpdk support for configuring link speed, duplex mode, and autonegotiation on DPDK interfaces. Users can now set these options in the Interface table, and the values are propagated to the underlying DPDK device. Signed-off-by: resa, Renata Saiakhova <Renata.Saiakhova@ekinops.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideThis PR extends the DPDK netdev backend by introducing user‐configurable link speed, duplex, and autoneg parameters—adding state in netdev_dpdk, conversion and update routines, and wiring these into the port configuration and reconfigure paths—while also exposing the options in the vswitchd XML schema. It rounds out the feature with validation and reconfigure triggers. In addition, the PR delivers targeted bugfixes and cleanups: memory‐leak prevention in the DPDK offload parser, refined bond rebalance logic, lock annotations in ofproto, updated CI flags, and strengthened assertions for safer operation. Sequence diagram for DPDK link configuration update and reconfigure triggersequenceDiagram
participant User
participant "vswitchd XML/options"
participant netdev_dpdk
participant DPDK
User->>"vswitchd XML/options": set dpdk-link-speed, dpdk-autoneg, dpdk-duplex
"vswitchd XML/options"->>netdev_dpdk: pass config via dpdk_set_link_config()
netdev_dpdk->>netdev_dpdk: validate config
netdev_dpdk->>netdev_dpdk: update requested_link_speed, requested_autoneg, requested_duplex
netdev_dpdk->>netdev_dpdk: trigger netdev_request_reconfigure()
netdev_dpdk->>DPDK: apply new link config on reconfigure
ER diagram for new DPDK link configuration options in vswitchd XMLerDiagram
INTERFACE ||--o{ OPTIONS : has
OPTIONS {
integer dpdk-link-speed
boolean dpdk-autoneg
string dpdk-duplex
}
INTERFACE {
string type
}
OPTIONS }|--|{ DPDK : applies_to
DPDK {
string type
}
Class diagram for updated netdev_dpdk structure and link config logicclassDiagram
class netdev_dpdk {
+int requested_link_speed
+bool requested_autoneg
+dpdk_link_duplex_t requested_duplex
+int link_speed
+bool autoneg
+dpdk_link_duplex_t duplex
+void netdev_dpdk_update_link_fields_from_hw()
+void dpdk_set_link_config()
}
class dpdk_link_duplex_t {
<<enum>>
FULL_DUPLEX
HALF_DUPLEX
}
netdev_dpdk --> dpdk_link_duplex_t : uses
netdev_dpdk : +mutex
netdev_dpdk : +link
netdev_dpdk : +up
netdev_dpdk : +port_id
netdev_dpdk : +devargs
netdev_dpdk : +started
netdev_dpdk : +link_reset_cnt
netdev_dpdk : +vid
netdev_dpdk : +vhost_reconfigured
netdev_dpdk : +virtio_features_state
netdev_dpdk : +requested_socket_id
netdev_dpdk : +rxq_size
netdev_dpdk : +txq_size
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds configurable DPDK link speed/duplex/autoneg with new fields, conversion/validation, reconfiguration, and hardware state syncing. Updates schema and docs accordingly. Introduces concurrency guards in ofproto table desc path. Tweaks bond rebalance timing logic. Fixes leaks and improves robustness in DPDK offload and several OVSDB components. Minor CI and AUTHORS updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin (OVSDB client)
participant OVSDB as OVSDB (vswitch.xml)
participant vsd as vswitchd
participant nd as netdev-dpdk
participant dpdk as DPDK PMD/Hardware
Admin->>OVSDB: Set Interface.options { dpdk-link-speed, dpdk-autoneg, dpdk-duplex }
OVSDB-->>vsd: Notifies config change
vsd->>nd: dpdk_set_link_config(opts)
nd->>nd: Validate combo / convert to link_speeds
nd-->>vsd: Config changed?
alt changed
vsd->>nd: netdev_dpdk_reconfigure()
nd->>dpdk: dpdk_eth_dev_port_config(link_speeds)
nd->>nd: Update requested_* and actual fields
else no change
nd-->>vsd: No-op
end
rect rgba(230,245,255,0.6)
note over nd,dpdk: Link status polling/interrupt
dpdk-->>nd: Report link up/down, speed, duplex, autoneg
nd->>nd: netdev_dpdk_update_link_fields_from_hw()
end
sequenceDiagram
participant Caller
participant ofp as ofproto.c
participant M as ofproto_mutex
Caller->>ofp: query_tables_desc()
ofp->>M: lock()
ofp->>ofp: build table_desc, query per-table
ofp->>M: unlock()
ofp-->>Caller: table descriptions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Avoid the 'ato*()' family of functions. Their use can lead to undefined behavior, integer overflows, and lack of appropriate error handling. Instead prefer the 'strtol*()' family of functions. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/netdev-dpdk.c:2532` </location>
<code_context>
}
}
+static bool
+is_valid_link_combo(bool autoneg, int speed, int duplex)
+{
+ VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s",
+ autoneg ? "true" : "false",
+ speed,
+ duplex == FULL_DUPLEX ? "full" : "half");
+ if (autoneg) {
+ /* Autoneg ON: speed/duplex should not be forced. */
+ return true;
+ } else {
+ /* Autoneg OFF: must set valid speed/duplex. */
+ if (speed == 100 && duplex == HALF_DUPLEX) {
+ return true;
+ }
+ if ((speed == 100 || speed == 1000 || speed == 10000) &&
+ duplex == FULL_DUPLEX) {
+ return true;
+ }
+ return false;
+ }
+}
</code_context>
<issue_to_address>
is_valid_link_combo may not cover all valid DPDK speed/duplex combinations.
Please verify whether DPDK supports additional speed or duplex combinations and update the function logic accordingly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
static bool
is_valid_link_combo(bool autoneg, int speed, int duplex)
{
VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s",
autoneg ? "true" : "false",
speed,
duplex == FULL_DUPLEX ? "full" : "half");
if (autoneg) {
/* Autoneg ON: speed/duplex should not be forced. */
return true;
} else {
/* Autoneg OFF: must set valid speed/duplex. */
if (speed == 100 && duplex == HALF_DUPLEX) {
return true;
}
if ((speed == 100 || speed == 1000 || speed == 10000) &&
duplex == FULL_DUPLEX) {
return true;
}
return false;
}
}
=======
/*
* Returns true if the speed/duplex combination is valid for DPDK.
* DPDK supports the following speeds (in Mbps) and duplex modes:
* - 10 Mbps: HALF/FULL
* - 100 Mbps: HALF/FULL
* - 1000 Mbps: FULL
* - 2500 Mbps: FULL
* - 5000 Mbps: FULL
* - 10000 Mbps: FULL
* - 20000 Mbps: FULL
* - 25000 Mbps: FULL
* - 40000 Mbps: FULL
* - 50000 Mbps: FULL
* - 100000 Mbps: FULL
*/
static bool
is_valid_link_combo(bool autoneg, int speed, int duplex)
{
VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s",
autoneg ? "true" : "false",
speed,
duplex == FULL_DUPLEX ? "full" : "half");
if (autoneg) {
/* Autoneg ON: speed/duplex should not be forced. */
return true;
} else {
/* Autoneg OFF: must set valid speed/duplex. */
switch (speed) {
case 10:
case 100:
if (duplex == HALF_DUPLEX || duplex == FULL_DUPLEX) {
return true;
}
break;
case 1000:
case 2500:
case 5000:
case 10000:
case 20000:
case 25000:
case 40000:
case 50000:
case 100000:
if (duplex == FULL_DUPLEX) {
return true;
}
break;
default:
break;
}
return false;
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `lib/netdev-dpdk.c:1027` </location>
<code_context>
+ if (link->link_status) {
</code_context>
<issue_to_address>
Link fields are only updated when link is up; consider resetting all fields when down.
Currently, only link_speed is reset when the link is down. Resetting duplex and autoneg to default or unknown values would help prevent reporting outdated information.
Suggested implementation:
```c
if (link->link_status) {
/* Link is up - update actual speed, duplex, autoneg from hardware */
switch (link->link_speed) {
case RTE_ETH_SPEED_NUM_10M:
dev->link_speed = 10;
break;
case RTE_ETH_SPEED_NUM_100M:
dev->link_speed = 100;
break;
case RTE_ETH_SPEED_NUM_1G:
dev->link_speed = 1000;
/* ... rest of link up code ... */
} else {
/* Link is down - reset fields to default/unknown values */
dev->link_speed = 0;
dev->duplex = NETDEV_DUPLEX_UNKNOWN;
dev->autoneg = false;
}
```
If `NETDEV_DUPLEX_UNKNOWN` is not defined, you should define it in the appropriate header (likely as 0 or -1).
If `dev->autoneg` is not a boolean, set it to the appropriate "unknown" or "default" value as per your codebase conventions.
</issue_to_address>
### Comment 3
<location> `ovsdb/trigger.c:401` </location>
<code_context>
/* Permanent error. Transition to "completed" state to report
* it. */
if (!strcmp(t->request->method, "transact")) {
+ ovs_assert(t->reply);
json_array_add(t->reply->result,
ovsdb_error_to_json_free(error));
</code_context>
<issue_to_address>
The assertion on t->reply may mask underlying issues.
If t->reply may be null, handle this case explicitly or log an error to avoid masking failures and improve robustness.
</issue_to_address>
### Comment 4
<location> `ofproto/bond.c:462` </location>
<code_context>
}
if (bond->rebalance_interval != s->rebalance_interval) {
+ if (s->rebalance_interval && bond->rebalance_interval) {
+ /* Recompute the next rebalance interval by moving the
+ * next_rebalance to be offset by the new interval. In this
+ * case, if all that was updated is the rebalance interval,
+ * we can skip triggering the rest of the port reconfiguration. */
+ if (bond->next_rebalance) {
+ long long int old_start_time =
+ bond->next_rebalance - s->rebalance_interval;
+ bond->next_rebalance =
+ old_start_time + bond->rebalance_interval;
+ }
+ } else {
+ /* When the bond is doing a disable/enable of the rebalance
+ * interval, trigger the revalidation. */
</code_context>
<issue_to_address>
The logic for rebalance interval changes may not cover all edge cases.
If next_rebalance is zero, the code does not update it. Please confirm if this is intentional, or if next_rebalance should be set in this scenario.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (s->rebalance_interval && bond->rebalance_interval) {
/* Recompute the next rebalance interval by moving the
* next_rebalance to be offset by the new interval. In this
* case, if all that was updated is the rebalance interval,
* we can skip triggering the rest of the port reconfiguration. */
if (bond->next_rebalance) {
long long int old_start_time =
bond->next_rebalance - s->rebalance_interval;
bond->next_rebalance =
old_start_time + bond->rebalance_interval;
}
} else {
=======
if (s->rebalance_interval && bond->rebalance_interval) {
/* Recompute the next rebalance interval by moving the
* next_rebalance to be offset by the new interval. In this
* case, if all that was updated is the rebalance interval,
* we can skip triggering the rest of the port reconfiguration. */
if (bond->next_rebalance) {
long long int old_start_time =
bond->next_rebalance - s->rebalance_interval;
bond->next_rebalance =
old_start_time + bond->rebalance_interval;
} else {
/* If next_rebalance is zero, set it to current time + new interval. */
bond->next_rebalance = time_msec() + bond->rebalance_interval;
}
} else {
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `lib/netdev-offload-dpdk.c:1529` </location>
<code_context>
} else {
VLOG_WARN_RL(&rl, "Unknown IPv4 frag (0x%x/0x%x)",
match->flow.nw_frag, match->wc.masks.nw_frag);
+ free(spec);
+ free(mask);
return -1;
}
consumed_masks->nw_frag = 0;
</code_context>
<issue_to_address>
Memory is freed before returning on error, but check for double free risk.
Verify that spec and mask are not freed elsewhere after this error path to prevent double free. Apply the same check for frag_spec and frag_mask in the IPv6 case.
Suggested implementation:
```c
/* Free spec and mask only here on error. Ensure not freed elsewhere. */
free(spec);
spec = NULL;
free(mask);
mask = NULL;
return -1;
```
```c
/* Free frag_spec and frag_mask only here on error. Ensure not freed elsewhere. */
free(frag_spec);
frag_spec = NULL;
free(frag_mask);
frag_mask = NULL;
return -1;
```
- Review the rest of the function and file to ensure that `spec`, `mask`, `frag_spec`, and `frag_mask` are not freed elsewhere after these error paths. If they are, remove those frees or guard them with a NULL check.
- If these pointers are used after the error return, ensure that the calling code checks for NULL before freeing.
- Consider documenting the ownership and lifecycle of these pointers in the function header or comments for future maintainers.
</issue_to_address>
## Security Issues
### Issue 1
<location> `lib/netdev-dpdk.c:2564` </location>
<issue_to_address>
**security (c.lang.correctness.incorrect-use-ato-fn):** Avoid the 'ato*()' family of functions. Their use can lead to undefined behavior, integer overflows, and lack of appropriate error handling. Instead prefer the 'strtol*()' family of functions.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+2532
to
+2553
| static bool | ||
| is_valid_link_combo(bool autoneg, int speed, int duplex) | ||
| { | ||
| VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s", | ||
| autoneg ? "true" : "false", | ||
| speed, | ||
| duplex == FULL_DUPLEX ? "full" : "half"); | ||
| if (autoneg) { | ||
| /* Autoneg ON: speed/duplex should not be forced. */ | ||
| return true; | ||
| } else { | ||
| /* Autoneg OFF: must set valid speed/duplex. */ | ||
| if (speed == 100 && duplex == HALF_DUPLEX) { | ||
| return true; | ||
| } | ||
| if ((speed == 100 || speed == 1000 || speed == 10000) && | ||
| duplex == FULL_DUPLEX) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: is_valid_link_combo may not cover all valid DPDK speed/duplex combinations.
Please verify whether DPDK supports additional speed or duplex combinations and update the function logic accordingly.
Suggested change
| static bool | |
| is_valid_link_combo(bool autoneg, int speed, int duplex) | |
| { | |
| VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s", | |
| autoneg ? "true" : "false", | |
| speed, | |
| duplex == FULL_DUPLEX ? "full" : "half"); | |
| if (autoneg) { | |
| /* Autoneg ON: speed/duplex should not be forced. */ | |
| return true; | |
| } else { | |
| /* Autoneg OFF: must set valid speed/duplex. */ | |
| if (speed == 100 && duplex == HALF_DUPLEX) { | |
| return true; | |
| } | |
| if ((speed == 100 || speed == 1000 || speed == 10000) && | |
| duplex == FULL_DUPLEX) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| } | |
| /* | |
| * Returns true if the speed/duplex combination is valid for DPDK. | |
| * DPDK supports the following speeds (in Mbps) and duplex modes: | |
| * - 10 Mbps: HALF/FULL | |
| * - 100 Mbps: HALF/FULL | |
| * - 1000 Mbps: FULL | |
| * - 2500 Mbps: FULL | |
| * - 5000 Mbps: FULL | |
| * - 10000 Mbps: FULL | |
| * - 20000 Mbps: FULL | |
| * - 25000 Mbps: FULL | |
| * - 40000 Mbps: FULL | |
| * - 50000 Mbps: FULL | |
| * - 100000 Mbps: FULL | |
| */ | |
| static bool | |
| is_valid_link_combo(bool autoneg, int speed, int duplex) | |
| { | |
| VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s", | |
| autoneg ? "true" : "false", | |
| speed, | |
| duplex == FULL_DUPLEX ? "full" : "half"); | |
| if (autoneg) { | |
| /* Autoneg ON: speed/duplex should not be forced. */ | |
| return true; | |
| } else { | |
| /* Autoneg OFF: must set valid speed/duplex. */ | |
| switch (speed) { | |
| case 10: | |
| case 100: | |
| if (duplex == HALF_DUPLEX || duplex == FULL_DUPLEX) { | |
| return true; | |
| } | |
| break; | |
| case 1000: | |
| case 2500: | |
| case 5000: | |
| case 10000: | |
| case 20000: | |
| case 25000: | |
| case 40000: | |
| case 50000: | |
| case 100000: | |
| if (duplex == FULL_DUPLEX) { | |
| return true; | |
| } | |
| break; | |
| default: | |
| break; | |
| } | |
| return false; | |
| } | |
| } |
| @@ -1004,8 +1018,75 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev) | |||
| return ret; | |||
| } | |||
There was a problem hiding this comment.
suggestion: Link fields are only updated when link is up; consider resetting all fields when down.
Currently, only link_speed is reset when the link is down. Resetting duplex and autoneg to default or unknown values would help prevent reporting outdated information.
Suggested implementation:
if (link->link_status) {
/* Link is up - update actual speed, duplex, autoneg from hardware */
switch (link->link_speed) {
case RTE_ETH_SPEED_NUM_10M:
dev->link_speed = 10;
break;
case RTE_ETH_SPEED_NUM_100M:
dev->link_speed = 100;
break;
case RTE_ETH_SPEED_NUM_1G:
dev->link_speed = 1000;
/* ... rest of link up code ... */
} else {
/* Link is down - reset fields to default/unknown values */
dev->link_speed = 0;
dev->duplex = NETDEV_DUPLEX_UNKNOWN;
dev->autoneg = false;
}If NETDEV_DUPLEX_UNKNOWN is not defined, you should define it in the appropriate header (likely as 0 or -1).
If dev->autoneg is not a boolean, set it to the appropriate "unknown" or "default" value as per your codebase conventions.
| /* Permanent error. Transition to "completed" state to report | ||
| * it. */ | ||
| if (!strcmp(t->request->method, "transact")) { | ||
| ovs_assert(t->reply); |
There was a problem hiding this comment.
suggestion (bug_risk): The assertion on t->reply may mask underlying issues.
If t->reply may be null, handle this case explicitly or log an error to avoid masking failures and improve robustness.
Comment on lines
+462
to
+473
| if (s->rebalance_interval && bond->rebalance_interval) { | ||
| /* Recompute the next rebalance interval by moving the | ||
| * next_rebalance to be offset by the new interval. In this | ||
| * case, if all that was updated is the rebalance interval, | ||
| * we can skip triggering the rest of the port reconfiguration. */ | ||
| if (bond->next_rebalance) { | ||
| long long int old_start_time = | ||
| bond->next_rebalance - s->rebalance_interval; | ||
| bond->next_rebalance = | ||
| old_start_time + bond->rebalance_interval; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
suggestion: The logic for rebalance interval changes may not cover all edge cases.
If next_rebalance is zero, the code does not update it. Please confirm if this is intentional, or if next_rebalance should be set in this scenario.
Suggested change
| if (s->rebalance_interval && bond->rebalance_interval) { | |
| /* Recompute the next rebalance interval by moving the | |
| * next_rebalance to be offset by the new interval. In this | |
| * case, if all that was updated is the rebalance interval, | |
| * we can skip triggering the rest of the port reconfiguration. */ | |
| if (bond->next_rebalance) { | |
| long long int old_start_time = | |
| bond->next_rebalance - s->rebalance_interval; | |
| bond->next_rebalance = | |
| old_start_time + bond->rebalance_interval; | |
| } | |
| } else { | |
| if (s->rebalance_interval && bond->rebalance_interval) { | |
| /* Recompute the next rebalance interval by moving the | |
| * next_rebalance to be offset by the new interval. In this | |
| * case, if all that was updated is the rebalance interval, | |
| * we can skip triggering the rest of the port reconfiguration. */ | |
| if (bond->next_rebalance) { | |
| long long int old_start_time = | |
| bond->next_rebalance - s->rebalance_interval; | |
| bond->next_rebalance = | |
| old_start_time + bond->rebalance_interval; | |
| } else { | |
| /* If next_rebalance is zero, set it to current time + new interval. */ | |
| bond->next_rebalance = time_msec() + bond->rebalance_interval; | |
| } | |
| } else { |
Comment on lines
+1529
to
1531
| free(spec); | ||
| free(mask); | ||
| return -1; |
There was a problem hiding this comment.
suggestion (bug_risk): Memory is freed before returning on error, but check for double free risk.
Verify that spec and mask are not freed elsewhere after this error path to prevent double free. Apply the same check for frag_spec and frag_mask in the IPv6 case.
Suggested implementation:
/* Free spec and mask only here on error. Ensure not freed elsewhere. */
free(spec);
spec = NULL;
free(mask);
mask = NULL;
return -1; /* Free frag_spec and frag_mask only here on error. Ensure not freed elsewhere. */
free(frag_spec);
frag_spec = NULL;
free(frag_mask);
frag_mask = NULL;
return -1;- Review the rest of the function and file to ensure that
spec,mask,frag_spec, andfrag_maskare not freed elsewhere after these error paths. If they are, remove those frees or guard them with a NULL check. - If these pointers are used after the error return, ensure that the calling code checks for NULL before freeing.
- Consider documenting the ownership and lifecycle of these pointers in the function header or comments for future maintainers.
| const char *autoneg_str = smap_get(args, "dpdk-autoneg"); | ||
| const char *duplex_str = smap_get(args, "dpdk-duplex"); | ||
|
|
||
| int requested_link_speed = link_speed_str ? atoi(link_speed_str) : 0; |
There was a problem hiding this comment.
security (c.lang.correctness.incorrect-use-ato-fn): Avoid the 'ato*()' family of functions. Their use can lead to undefined behavior, integer overflows, and lack of appropriate error handling. Instead prefer the 'strtol*()' family of functions.
Source: opengrep
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/netdev-offload-dpdk.c (1)
2378-2384: Add null-check for offload_data before decrement
Wrap therte_flow_counters[tid]--in a guard to avoid NPE during teardown:data = (struct netdev_offload_dpdk_data *) ovsrcu_get(void *, &physdev->hw_info.offload_data); - data->rte_flow_counters[tid]--; + if (data && data->rte_flow_counters) { + data->rte_flow_counters[tid]--; + } else { + VLOG_WARN_RL(&rl, "%s: offload_data missing while destroying flow", + netdev_get_name(physdev)); + }lib/netdev-dpdk.c (1)
1088-1119: Update on any link change, not only status flips.Currently you only refresh when link_status toggles. Speed/duplex/autoneg can change while link stays up; those changes won’t be reflected.
Apply this diff:
- if (dev->link.link_status != link.link_status) { - netdev_change_seq_changed(&dev->up); - dev->link_reset_cnt++; - dev->link = link; - /* Update individual link fields from hardware state */ - netdev_dpdk_update_link_fields_from_hw(dev); + if (dev->link.link_status != link.link_status || + dev->link.link_speed != link.link_speed || + dev->link.link_duplex != link.link_duplex || + dev->link.link_autoneg != link.link_autoneg) { + bool status_changed = dev->link.link_status != link.link_status; + if (status_changed) { + netdev_change_seq_changed(&dev->up); + dev->link_reset_cnt++; + } + dev->link = link; + /* Keep derived fields in sync with hardware. */ + netdev_dpdk_update_link_fields_from_hw(dev);
🧹 Nitpick comments (11)
.cirrus.yml (1)
19-19: Guard pkg update against repo-alias mismatchesExplicitly selecting the “FreeBSD” repo may fail if the alias differs on some images. Consider a safe fallback.
Apply this diff:
- - pkg update -f -r FreeBSD + - pkg update -f -r FreeBSD || pkg update -fovsdb/monitor.c (1)
681-688: Initial-state filtering change: confirm no regressions and document intent.Switching initial=true mapping to return OJMS_NONE when 'new' is NULL will drop such rows from initial updates. This looks correct given how init sets only 'new' for live rows, but please:
- Verify no code path builds initial change-sets with new==NULL (e.g., corner cases during replication or condition diffs).
- Add a brief comment here clarifying the intended behavior for initial snapshots.
Would you like me to add a short comment at this site summarizing the initial/none semantics?
vswitchd/vswitch.xml (2)
3592-3599: dpdk-link-speed: add schema bounds and clarify autoneg interaction/defaults
- Prevent invalid values (e.g., negatives) at the schema layer.
- Make it explicit this is ignored when autoneg is enabled and document “0/unset” behavior.
Apply this diff:
- <column name="options" key="dpdk-link-speed" type='{"type": "integer"}'> + <column name="options" key="dpdk-link-speed" type='{"type": "integer", "minInteger": 0}'> <p> Specifies the fixed link speed in Mbps for this DPDK interface (e.g., 1000, 10000). - Only applies to interfaces with type <code>dpdk</code>. + Only applies to interfaces with type <code>dpdk</code>. + Only effective when <code>options:dpdk-autoneg=false</code>; when autoneg is <code>true</code> this setting is ignored. + If unset or set to <code>0</code>, the driver default/autonegotiated speed is used. + Changing this value may trigger a brief device reconfiguration and link flap. </p> </column>
3600-3606: dpdk-autoneg: document default and operational impactState the default and that changing it can momentarily flap the link.
Apply this diff:
<column name="options" key="dpdk-autoneg" type='{"type": "boolean"}'> <p> Set to <code>true</code> to enable autonegotiation, <code>false</code> to disable. Only applies to interfaces with type <code>dpdk</code>. + Default is <code>true</code>. Changing this value may reconfigure the device and briefly flap link state. </p> </column>ofproto/ofproto.c (2)
4287-4299: Guard the whole snapshot under ofproto_mutex; annotate to prevent accidental double-lockThe internal lock here is correct and yields a consistent table-desc snapshot. To prevent future accidental calls while holding ofproto_mutex (which would deadlock), annotate the function as OVS_EXCLUDED(ofproto_mutex).
Apply:
static void -query_tables_desc(struct ofproto *ofproto, struct ofputil_table_desc **descp) +query_tables_desc(struct ofproto *ofproto, struct ofputil_table_desc **descp) + OVS_EXCLUDED(ofproto_mutex) { struct ofputil_table_desc *table_desc; size_t i; table_desc = *descp = xcalloc(ofproto->n_tables, sizeof *table_desc); - ovs_mutex_lock(&ofproto_mutex); + ovs_mutex_lock(&ofproto_mutex); for (i = 0; i < ofproto->n_tables; i++) { struct ofputil_table_desc *td = &table_desc[i]; query_table_desc__(td, ofproto, i); } - ovs_mutex_unlock(&ofproto_mutex); + ovs_mutex_unlock(&ofproto_mutex); }
4330-4356: Nit: comment and comparisons disagree on threshold inclusivityThe comment says “less than or equal” / “greater or equal” but code uses strict < and >. Either adjust the comment or make the comparisons inclusive. Given existing behavior, update the comment to avoid confusion.
Apply:
-/* This function determines and sends the vacancy event, based on the value - * of current vacancy and threshold vacancy. If the current vacancy is less - * than or equal to vacancy_down, vacancy up events must be enabled, and when - * the current vacancy is greater or equal to vacancy_up, vacancy down events - * must be enabled. */ +/* This function determines and sends the vacancy event, based on the value + * of current vacancy and threshold vacancy. If the current vacancy is less + * than vacancy_down, vacancy up events must be enabled, and when + * the current vacancy is greater than vacancy_up, vacancy down events + * must be enabled. */lib/netdev-dpdk.c (5)
553-561: Avoid divergence between “requested_*” and “actual” link fields.Maintaining dev->link (rte_eth_link) plus separate dev->{link_speed,autoneg,duplex} risks drift unless always synced on every update.
- Treat dev->{link_speed,autoneg,duplex} as read-only snapshots derived from dev->link and refresh them immediately after every rte_eth_link_get* call (see fix below for check_link_status and dpdk_eth_dev_init), or drop these duplicates and always read from dev->link.
1021-1085: Good helper; also clear stale state on link down (optional).Function correctly maps DPDK speeds. Consider also resetting autoneg/duplex on link down to avoid stale presentation if consumers read these without checking link_status.
} else { /* Link is down - set speed to unknown */ dev->link_speed = 0; + dev->autoneg = false; + dev->duplex = FULL_DUPLEX; /* or leave unchanged; choose one policy */ }
1186-1304: Validation is too permissive/opaque; consider checking device capabilities.For fixed speed, validate the chosen speed against info->speed_capa and warn/fallback if unsupported (prevents rte_eth_dev_configure failures and surprises).
- In dpdk_eth_dev_port_config(), if (conf.link_speeds & RTE_ETH_LINK_SPEED_FIXED) ensure the selected bit is present in info->speed_capa; otherwise log and switch to AUTONEG or return -ENOTSUP.
1315-1326: OK to set link_speeds; add capability check.As above, masking with info->speed_capa before configure would make this more robust.
1407-1408: Remove noisy pointer log.Printing the address of a stack-local rte_eth_conf adds no value and is noisy.
- VLOG_INFO("Configuring device with conf=%p", &conf); + VLOG_DBG("Configuring device '%s' (n_rxq=%d, n_txq=%d)", dev->up.name, n_rxq, n_txq);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.cirrus.yml(1 hunks)AUTHORS.rst(1 hunks)lib/netdev-dpdk.c(14 hunks)lib/netdev-offload-dpdk.c(3 hunks)ofproto/bond.c(1 hunks)ofproto/ofproto.c(3 hunks)ovsdb/monitor.c(1 hunks)ovsdb/raft.c(1 hunks)ovsdb/relay.c(1 hunks)ovsdb/transaction.c(1 hunks)ovsdb/trigger.c(1 hunks)vswitchd/vswitch.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ofproto/bond.c (1)
ofproto/ofproto-dpif-upcall.c (1)
revalidate(2809-2989)
🔇 Additional comments (12)
AUTHORS.rst (1)
298-298: Approve addition of Linda Wang entry
Commit b1e295d confirms both author and Signed-off-by; entry is correctly placed after Linda Sun and aligned with existing formatting.ovsdb/relay.c (1)
383-385: Assert before use: LGTMRuntime assert for ctx->db/ctx->cs prevents UB later in the loop. Good defensive check.
ovsdb/trigger.c (1)
401-405: Permanent-error path hardened: LGTMAsserting t->reply before appending error avoids a NULL deref on regressions.
ovsdb/raft.c (1)
3625-3627: Fix: avoid NULL out-param deref in raft_next_entryOptional eid now respected; no behavior change otherwise.
ovsdb/transaction.c (1)
649-651: Invariant assert for map values: LGTMEnsures datum->values present when n>0 for value-weak-ref path; debug-only, safe.
lib/netdev-offload-dpdk.c (2)
1527-1532: Good catch: free IPv4 spec/mask on error.The early frees on the unknown IPv4 frag path prevent a leak. LGTM.
1612-1617: Good catch: free IPv6 frag spec/mask on error.Same here for IPv6 frag; leak avoided. LGTM.
vswitchd/vswitch.xml (1)
3608-3616: Clarify dpdk-duplex default and constraints
- Default is full duplex; half-duplex only applies at 10/100 Mbps and unsupported settings will fallback or fail per driver.
Apply this diff:
<column name="options" key="dpdk-duplex" type='{"type": "string", "enum": ["set", ["full", "half"]]}'> <p> Specifies the duplex mode for this DPDK interface. Valid values are <code>full</code> or <code>half</code>. Only applies to interfaces with type <code>dpdk</code>. - Only applies if autonegotiation is disabled. + Only applies if autonegotiation is disabled. Default is <code>full</code>. + Half-duplex is typically only applicable at 10/100 Mbps and may not be supported by many devices or higher link speeds; unsupported requests will be ignored or fail depending on the driver. </p> </column>ofproto/ofproto.c (1)
4266-4283: Good: add OVS_REQUIRES(ofproto_mutex) to query_table_desc__This makes the function’s contract explicit and matches its data access pattern. Callers (query_tables_desc() and send_table_status()) now satisfy the requirement.
lib/netdev-dpdk.c (3)
453-457: Duplex enum addition looks fine.Local, self-explanatory, and avoids reusing DPDK enums. No issues.
1771-1777: Defaults look reasonable.Autoneg on, speed 0 (auto), full duplex default is sensible.
2636-2637: OK to wire link-config parsing into set_config.This is the right place to capture user intent. No issues.
Comment on lines
+1688
to
+1691
| dev->link_speed = dev->requested_link_speed; | ||
| dev->autoneg = dev->requested_autoneg; | ||
| dev->duplex = dev->requested_duplex; | ||
|
|
There was a problem hiding this comment.
Don’t assume requested == applied; read actual from hardware.
Setting dev->{link_speed,autoneg,duplex} to requested values can misreport reality if the PMD rejects or adjusts the config.
Apply this diff:
- dev->link_speed = dev->requested_link_speed;
- dev->autoneg = dev->requested_autoneg;
- dev->duplex = dev->requested_duplex;And immediately after retrieving dev->link, sync derived fields:
if (rte_eth_link_get_nowait(dev->port_id, &dev->link) < 0) {
memset(&dev->link, 0, sizeof dev->link);
}
+ /* Reflect actual hardware state in derived fields. */
+ netdev_dpdk_update_link_fields_from_hw(dev);📝 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
| dev->link_speed = dev->requested_link_speed; | |
| dev->autoneg = dev->requested_autoneg; | |
| dev->duplex = dev->requested_duplex; | |
| /* After (re)configuring the PMD, query the real hardware link state. */ | |
| if (rte_eth_link_get_nowait(dev->port_id, &dev->link) < 0) { | |
| memset(&dev->link, 0, sizeof dev->link); | |
| } | |
| /* Reflect actual hardware state in derived fields. */ | |
| netdev_dpdk_update_link_fields_from_hw(dev); |
🤖 Prompt for AI Agents
In lib/netdev-dpdk.c around lines 1688-1691, don’t set dev->link_speed,
dev->autoneg, and dev->duplex to the requested_* values; instead, after you
retrieve and populate dev->link from the hardware, copy the actual hardware
values into the device fields (e.g., set dev->link_speed from dev->link's speed
field, dev->autoneg from dev->link's autoneg field, and dev->duplex from
dev->link's duplex field) so the struct reflects what the PMD actually applied
rather than what was requested.
Comment on lines
+2532
to
+2554
| static bool | ||
| is_valid_link_combo(bool autoneg, int speed, int duplex) | ||
| { | ||
| VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s", | ||
| autoneg ? "true" : "false", | ||
| speed, | ||
| duplex == FULL_DUPLEX ? "full" : "half"); | ||
| if (autoneg) { | ||
| /* Autoneg ON: speed/duplex should not be forced. */ | ||
| return true; | ||
| } else { | ||
| /* Autoneg OFF: must set valid speed/duplex. */ | ||
| if (speed == 100 && duplex == HALF_DUPLEX) { | ||
| return true; | ||
| } | ||
| if ((speed == 100 || speed == 1000 || speed == 10000) && | ||
| duplex == FULL_DUPLEX) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Validation rejects many valid speeds/duplex combinations.
This allows only 100/HD and 100/1000/10000 FD; it rejects 10M FD/HD, 2.5G, 5G, 20G, 25G, 40G, 50G, 56G, 100G, 200G, 400G full-duplex — all commonly supported.
Use a comprehensive whitelist:
-static bool
-is_valid_link_combo(bool autoneg, int speed, int duplex)
+static bool
+is_valid_link_combo(bool autoneg, int speed, int duplex)
{
- VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s",
- autoneg ? "true" : "false",
- speed,
- duplex == FULL_DUPLEX ? "full" : "half");
+ VLOG_DBG("is_valid_link_combo: autoneg=%s, speed=%d, duplex=%s",
+ autoneg ? "true" : "false", speed,
+ duplex == FULL_DUPLEX ? "full" : "half");
if (autoneg) {
/* Autoneg ON: speed/duplex should not be forced. */
return true;
} else {
- /* Autoneg OFF: must set valid speed/duplex. */
- if (speed == 100 && duplex == HALF_DUPLEX) {
- return true;
- }
- if ((speed == 100 || speed == 1000 || speed == 10000) &&
- duplex == FULL_DUPLEX) {
- return true;
- }
- return false;
+ switch (speed) {
+ case 0: return true; /* treat as auto */
+ case 10: return duplex == FULL_DUPLEX || duplex == HALF_DUPLEX;
+ case 100: return duplex == FULL_DUPLEX || duplex == HALF_DUPLEX;
+ case 1000: return duplex == FULL_DUPLEX; /* 1G HD is not supported */
+ case 2500:
+ case 5000:
+ case 10000:
+ case 20000:
+ case 25000:
+ case 40000:
+ case 50000:
+ case 56000:
+ case 100000:
+ case 200000:
+ case 400000:
+ return duplex == FULL_DUPLEX;
+ default:
+ return false;
+ }
}
}Optionally, further constrain against the device’s advertised capabilities (info->speed_capa) at configuration time.
Support the full range of speeds in is_valid_link_combo
In lib/netdev-dpdk.c (lines 2532–2554), manual-mode logic only permits 100 Mb/s half-duplex and 100/1000/10000 Mb/s full-duplex, rejecting valid combinations (10 Mb/s, 2.5 Gb/s, 5 Gb/s, 20 Gb/s, 25 Gb/s, 40 Gb/s, 50 Gb/s, 56 Gb/s, 100 Gb/s, 200 Gb/s, 400 Gb/s). Replace with a comprehensive switch/case whitelist (e.g. 0→auto; allow both duplexes at 10/100; full-duplex on higher rates) and optionally validate against info->speed_capa.
Comment on lines
+2555
to
+2611
| static void | ||
| dpdk_set_link_config(struct netdev_dpdk *dev, const struct smap *args, | ||
| char **errp) | ||
| OVS_REQUIRES(dev->mutex) | ||
| { | ||
| const char *link_speed_str = smap_get(args, "dpdk-link-speed"); | ||
| const char *autoneg_str = smap_get(args, "dpdk-autoneg"); | ||
| const char *duplex_str = smap_get(args, "dpdk-duplex"); | ||
|
|
||
| int requested_link_speed = link_speed_str ? atoi(link_speed_str) : 0; | ||
| bool requested_autoneg = autoneg_str ? !strcmp(autoneg_str, "true") : true; | ||
| int requested_duplex = duplex_str && !strcmp(duplex_str, "half") ? | ||
| HALF_DUPLEX : FULL_DUPLEX; | ||
|
|
||
| VLOG_INFO("dpdk_set_link_config: Requested DPDK link configuration: " | ||
| "speed=%d, autoneg=%s, duplex=%s", | ||
| requested_link_speed, | ||
| requested_autoneg ? "true" : "false", | ||
| requested_duplex == FULL_DUPLEX ? "full" : "half"); | ||
|
|
||
| /* Sanity check */ | ||
| if (!is_valid_link_combo(requested_autoneg, requested_link_speed, | ||
| requested_duplex)) { | ||
| VLOG_WARN_BUF(errp, "Invalid DPDK link configuration: " | ||
| "autoneg=%s, speed=%d, duplex=%s", | ||
| requested_autoneg ? "true" : "false", | ||
| requested_link_speed, | ||
| requested_duplex == FULL_DUPLEX ? "full" : "half"); | ||
| /* fallback to autoneg true */ | ||
| requested_autoneg = true; | ||
| requested_link_speed = 0; | ||
| requested_duplex = FULL_DUPLEX; | ||
| } | ||
|
|
||
| bool changed = false; | ||
| if (dev->requested_link_speed != requested_link_speed) { | ||
| dev->requested_link_speed = requested_link_speed; | ||
| changed = true; | ||
| } | ||
| if (dev->requested_autoneg != requested_autoneg) { | ||
| dev->requested_autoneg = requested_autoneg; | ||
| changed = true; | ||
| } | ||
| if (dev->requested_duplex != requested_duplex) { | ||
| dev->requested_duplex = requested_duplex; | ||
| changed = true; | ||
| } | ||
| if (changed) { | ||
| VLOG_INFO("dpdk_set_link_config: Before reconfigure: " | ||
| "Link configuration changed: speed=%d, autoneg=%s, duplex=%s", | ||
| dev->requested_link_speed, | ||
| dev->requested_autoneg ? "true" : "false", | ||
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); | ||
| netdev_request_reconfigure(&dev->up); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parse options robustly and scope to ETH devices; trigger reconfigure only on change.
- Use smap_get_int/smap_get_bool to parse values.
- Only accept dpdk-duplex when autoneg is false (warn otherwise).
- Only process these options for DPDK_DEV_ETH, not vhost.
- const char *link_speed_str = smap_get(args, "dpdk-link-speed");
- const char *autoneg_str = smap_get(args, "dpdk-autoneg");
- const char *duplex_str = smap_get(args, "dpdk-duplex");
-
- int requested_link_speed = link_speed_str ? atoi(link_speed_str) : 0;
- bool requested_autoneg = autoneg_str ? !strcmp(autoneg_str, "true") : true;
- int requested_duplex = duplex_str && !strcmp(duplex_str, "half") ?
- HALF_DUPLEX : FULL_DUPLEX;
+ if (dev->type != DPDK_DEV_ETH) {
+ return;
+ }
+ int requested_link_speed = smap_get_int(args, "dpdk-link-speed", 0);
+ bool requested_autoneg = smap_get_bool(args, "dpdk-autoneg", true);
+ const char *duplex_str = smap_get(args, "dpdk-duplex");
+ dpdk_link_duplex_t requested_duplex =
+ (duplex_str && !strcasecmp(duplex_str, "half")) ? HALF_DUPLEX
+ : FULL_DUPLEX;
+ if (requested_autoneg && duplex_str) {
+ VLOG_DBG("%s: ignoring dpdk-duplex because dpdk-autoneg=true",
+ dev->up.name);
+ }📝 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
| static void | |
| dpdk_set_link_config(struct netdev_dpdk *dev, const struct smap *args, | |
| char **errp) | |
| OVS_REQUIRES(dev->mutex) | |
| { | |
| const char *link_speed_str = smap_get(args, "dpdk-link-speed"); | |
| const char *autoneg_str = smap_get(args, "dpdk-autoneg"); | |
| const char *duplex_str = smap_get(args, "dpdk-duplex"); | |
| int requested_link_speed = link_speed_str ? atoi(link_speed_str) : 0; | |
| bool requested_autoneg = autoneg_str ? !strcmp(autoneg_str, "true") : true; | |
| int requested_duplex = duplex_str && !strcmp(duplex_str, "half") ? | |
| HALF_DUPLEX : FULL_DUPLEX; | |
| VLOG_INFO("dpdk_set_link_config: Requested DPDK link configuration: " | |
| "speed=%d, autoneg=%s, duplex=%s", | |
| requested_link_speed, | |
| requested_autoneg ? "true" : "false", | |
| requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| /* Sanity check */ | |
| if (!is_valid_link_combo(requested_autoneg, requested_link_speed, | |
| requested_duplex)) { | |
| VLOG_WARN_BUF(errp, "Invalid DPDK link configuration: " | |
| "autoneg=%s, speed=%d, duplex=%s", | |
| requested_autoneg ? "true" : "false", | |
| requested_link_speed, | |
| requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| /* fallback to autoneg true */ | |
| requested_autoneg = true; | |
| requested_link_speed = 0; | |
| requested_duplex = FULL_DUPLEX; | |
| } | |
| bool changed = false; | |
| if (dev->requested_link_speed != requested_link_speed) { | |
| dev->requested_link_speed = requested_link_speed; | |
| changed = true; | |
| } | |
| if (dev->requested_autoneg != requested_autoneg) { | |
| dev->requested_autoneg = requested_autoneg; | |
| changed = true; | |
| } | |
| if (dev->requested_duplex != requested_duplex) { | |
| dev->requested_duplex = requested_duplex; | |
| changed = true; | |
| } | |
| if (changed) { | |
| VLOG_INFO("dpdk_set_link_config: Before reconfigure: " | |
| "Link configuration changed: speed=%d, autoneg=%s, duplex=%s", | |
| dev->requested_link_speed, | |
| dev->requested_autoneg ? "true" : "false", | |
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| netdev_request_reconfigure(&dev->up); | |
| } | |
| } | |
| static void | |
| dpdk_set_link_config(struct netdev_dpdk *dev, const struct smap *args, | |
| char **errp) | |
| OVS_REQUIRES(dev->mutex) | |
| { | |
| /* Only apply link‐config settings for real Ethernet DPDK devices. */ | |
| if (dev->type != DPDK_DEV_ETH) { | |
| return; | |
| } | |
| /* Parse more robustly via smap helpers, with sane defaults. */ | |
| int requested_link_speed = smap_get_int(args, "dpdk-link-speed", 0); | |
| bool requested_autoneg = smap_get_bool(args, "dpdk-autoneg", true); | |
| const char *duplex_str = smap_get(args, "dpdk-duplex"); | |
| dpdk_link_duplex_t requested_duplex = | |
| (duplex_str && !strcasecmp(duplex_str, "half")) | |
| ? HALF_DUPLEX | |
| : FULL_DUPLEX; | |
| /* If autoneg is enabled, ignore any duplex hint. */ | |
| if (requested_autoneg && duplex_str) { | |
| VLOG_DBG("%s: ignoring dpdk-duplex because dpdk-autoneg=true", | |
| dev->up.name); | |
| } | |
| VLOG_INFO("dpdk_set_link_config: Requested DPDK link configuration: " | |
| "speed=%d, autoneg=%s, duplex=%s", | |
| requested_link_speed, | |
| requested_autoneg ? "true" : "false", | |
| requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| /* Sanity check */ | |
| if (!is_valid_link_combo(requested_autoneg, requested_link_speed, | |
| requested_duplex)) { | |
| VLOG_WARN_BUF(errp, "Invalid DPDK link configuration: " | |
| "autoneg=%s, speed=%d, duplex=%s", | |
| requested_autoneg ? "true" : "false", | |
| requested_link_speed, | |
| requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| /* fallback to autoneg true */ | |
| requested_autoneg = true; | |
| requested_link_speed = 0; | |
| requested_duplex = FULL_DUPLEX; | |
| } | |
| bool changed = false; | |
| if (dev->requested_link_speed != requested_link_speed) { | |
| dev->requested_link_speed = requested_link_speed; | |
| changed = true; | |
| } | |
| if (dev->requested_autoneg != requested_autoneg) { | |
| dev->requested_autoneg = requested_autoneg; | |
| changed = true; | |
| } | |
| if (dev->requested_duplex != requested_duplex) { | |
| dev->requested_duplex = requested_duplex; | |
| changed = true; | |
| } | |
| if (changed) { | |
| VLOG_INFO("dpdk_set_link_config: Before reconfigure: " | |
| "Link configuration changed: speed=%d, autoneg=%s, duplex=%s", | |
| dev->requested_link_speed, | |
| dev->requested_autoneg ? "true" : "false", | |
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| netdev_request_reconfigure(&dev->up); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/netdev-dpdk.c around lines 2555 to 2611: currently the function parses
dpdk link options using atoi/strcmp for all device types and accepts duplex even
when autoneg is enabled; update it to use smap_get_int for dpdk-link-speed and
smap_get_bool for dpdk-autoneg (with proper defaults), and use smap_get to read
dpdk-duplex but only accept/interpret it when autoneg is false (emit a
VLOG_WARN_BUF into errp and ignore duplex if autoneg is true); additionally,
guard the whole options handling so it only runs for DPDK_DEV_ETH devices (skip
for vhost), and ensure netdev_request_reconfigure(&dev->up) is invoked only when
one of dev->requested_* fields actually changes (preserve the existing changed
logic).
Comment on lines
+6498
to
+6503
| VLOG_DBG("netdev_dpdk_reconfigure: Before reconfigure: " | ||
| "Link configuration changed: speed=%d, autoneg=%s, duplex=%s", | ||
| dev->requested_link_speed, | ||
| dev->requested_autoneg ? "true" : "false", | ||
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); | ||
|
|
There was a problem hiding this comment.
Log reads dev fields without lock.
These VLOG reads race with concurrent writers of requested_* fields.
Move inside the mutex or snapshot under lock:
- VLOG_DBG("netdev_dpdk_reconfigure: Before reconfigure: "
- "Link configuration changed: speed=%d, autoneg=%s, duplex=%s",
- dev->requested_link_speed,
- dev->requested_autoneg ? "true" : "false",
- dev->requested_duplex == FULL_DUPLEX ? "full" : "half");
-
- ovs_mutex_lock(&dev->mutex);
+ ovs_mutex_lock(&dev->mutex);
+ VLOG_DBG("netdev_dpdk_reconfigure: requested link config: speed=%d, autoneg=%s, duplex=%s",
+ dev->requested_link_speed,
+ dev->requested_autoneg ? "true" : "false",
+ dev->requested_duplex == FULL_DUPLEX ? "full" : "half");📝 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
| VLOG_DBG("netdev_dpdk_reconfigure: Before reconfigure: " | |
| "Link configuration changed: speed=%d, autoneg=%s, duplex=%s", | |
| dev->requested_link_speed, | |
| dev->requested_autoneg ? "true" : "false", | |
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); | |
| ovs_mutex_lock(&dev->mutex); | |
| VLOG_DBG("netdev_dpdk_reconfigure: requested link config: speed=%d, autoneg=%s, duplex=%s", | |
| dev->requested_link_speed, | |
| dev->requested_autoneg ? "true" : "false", | |
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); |
🤖 Prompt for AI Agents
In lib/netdev-dpdk.c around lines 6498-6503, the VLOG_DBG call reads
dev->requested_link_speed, dev->requested_autoneg and dev->requested_duplex
without holding the device mutex, which races with concurrent writers; fix by
taking the appropriate mutex and either move the VLOG_DBG call inside that lock
or, if you prefer to keep logging outside, snapshot the requested_* fields into
local variables while holding the mutex then release the mutex and use the
locals in the VLOG_DBG call.
Comment on lines
+6581
to
+6589
| VLOG_DBG("netdev_dpdk_reconfigure: Applied link configuration: " | ||
| "speed=%d, autoneg=%s, duplex=%s", | ||
| dev->requested_link_speed, | ||
| dev->requested_autoneg ? "true" : "false", | ||
| dev->requested_duplex == FULL_DUPLEX ? "full" : "half"); | ||
| dev->link_speed = dev->requested_link_speed; | ||
| dev->autoneg = dev->requested_autoneg; | ||
| dev->duplex = dev->requested_duplex; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid forcing requested values into “actual” fields post-start.
You already captured and can sync from hardware. Keep these as snapshots of actual state only.
Apply the earlier dpdk_eth_dev_init change to remove these assignments and call netdev_dpdk_update_link_fields_from_hw() after reading dev->link.
🤖 Prompt for AI Agents
In lib/netdev-dpdk.c around lines 6581 to 6589, the code incorrectly overwrites
the actual link state by assigning requested_* fields into dev->link_speed,
dev->autoneg, and dev->duplex; remove those three assignments and instead call
netdev_dpdk_update_link_fields_from_hw() after the code path that reads
dev->link (apply the same dpdk_eth_dev_init change made earlier) so the dev->*
fields are populated from hardware snapshot only rather than forced from
requested values.
Comment on lines
461
to
479
| if (bond->rebalance_interval != s->rebalance_interval) { | ||
| if (s->rebalance_interval && bond->rebalance_interval) { | ||
| /* Recompute the next rebalance interval by moving the | ||
| * next_rebalance to be offset by the new interval. In this | ||
| * case, if all that was updated is the rebalance interval, | ||
| * we can skip triggering the rest of the port reconfiguration. */ | ||
| if (bond->next_rebalance) { | ||
| long long int old_start_time = | ||
| bond->next_rebalance - s->rebalance_interval; | ||
| bond->next_rebalance = | ||
| old_start_time + bond->rebalance_interval; | ||
| } | ||
| } else { | ||
| /* When the bond is doing a disable/enable of the rebalance | ||
| * interval, trigger the revalidation. */ | ||
| revalidate = true; | ||
| } | ||
| bond->rebalance_interval = s->rebalance_interval; | ||
| revalidate = true; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Fix rebalance schedule math: wrong interval used (old vs new).
When both intervals are non-zero, you subtract the new interval and add the old one, leaving next_rebalance effectively unchanged and drifting from the intended schedule. Use old_interval for subtraction and new_interval for addition.
Apply this diff:
- if (s->rebalance_interval && bond->rebalance_interval) {
+ if (s->rebalance_interval && bond->rebalance_interval) {
/* Recompute the next rebalance interval by moving the
* next_rebalance to be offset by the new interval. In this
* case, if all that was updated is the rebalance interval,
* we can skip triggering the rest of the port reconfiguration. */
if (bond->next_rebalance) {
- long long int old_start_time =
- bond->next_rebalance - s->rebalance_interval;
- bond->next_rebalance =
- old_start_time + bond->rebalance_interval;
+ long long int old_interval = bond->rebalance_interval;
+ long long int new_interval = s->rebalance_interval;
+ long long int old_start_time =
+ bond->next_rebalance - old_interval;
+ bond->next_rebalance = old_start_time + new_interval;
}
} else {Optional: if next_rebalance is 0 (not yet scheduled) and both intervals are non-zero, consider initializing it to time_msec() + s->rebalance_interval to avoid delaying the first rebalance indefinitely.
Correct rebalance interval adjustment
In the non-zero interval case, subtract the old interval and add the new one rather than vice versa:
- long long int old_start_time =
- bond->next_rebalance - s->rebalance_interval;
- bond->next_rebalance =
- old_start_time + bond->rebalance_interval;
+ long long int old_interval = bond->rebalance_interval;
+ long long int new_interval = s->rebalance_interval;
+ long long int old_start_time =
+ bond->next_rebalance - old_interval;
+ bond->next_rebalance = old_start_time + new_interval;Optional: if bond->next_rebalance is zero, consider setting it to time_msec() + s->rebalance_interval to schedule the first rebalance.
📝 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 (bond->rebalance_interval != s->rebalance_interval) { | |
| if (s->rebalance_interval && bond->rebalance_interval) { | |
| /* Recompute the next rebalance interval by moving the | |
| * next_rebalance to be offset by the new interval. In this | |
| * case, if all that was updated is the rebalance interval, | |
| * we can skip triggering the rest of the port reconfiguration. */ | |
| if (bond->next_rebalance) { | |
| long long int old_start_time = | |
| bond->next_rebalance - s->rebalance_interval; | |
| bond->next_rebalance = | |
| old_start_time + bond->rebalance_interval; | |
| } | |
| } else { | |
| /* When the bond is doing a disable/enable of the rebalance | |
| * interval, trigger the revalidation. */ | |
| revalidate = true; | |
| } | |
| bond->rebalance_interval = s->rebalance_interval; | |
| revalidate = true; | |
| } | |
| if (bond->rebalance_interval != s->rebalance_interval) { | |
| if (s->rebalance_interval && bond->rebalance_interval) { | |
| /* Recompute the next rebalance interval by moving the | |
| * next_rebalance to be offset by the new interval. In this | |
| * case, if all that was updated is the rebalance interval, | |
| * we can skip triggering the rest of the port reconfiguration. */ | |
| if (bond->next_rebalance) { | |
| long long int old_interval = bond->rebalance_interval; | |
| long long int new_interval = s->rebalance_interval; | |
| long long int old_start_time = | |
| bond->next_rebalance - old_interval; | |
| bond->next_rebalance = old_start_time + new_interval; | |
| } | |
| } else { | |
| /* When the bond is doing a disable/enable of the rebalance | |
| * interval, trigger the revalidation. */ | |
| revalidate = true; | |
| } | |
| bond->rebalance_interval = s->rebalance_interval; | |
| } |
🤖 Prompt for AI Agents
In ofproto/bond.c around lines 461 to 479, the rebalance interval adjustment
uses the wrong arithmetic when updating bond->next_rebalance: compute the old
start time as bond->next_rebalance minus bond->rebalance_interval (the previous
interval), then set bond->next_rebalance = old_start_time +
s->rebalance_interval (the new interval) instead of the current reversed
subtraction/addition; also, if bond->next_rebalance is zero, optionally
initialize it to time_msec() + s->rebalance_interval to schedule the first
rebalance.
ovsrobot
pushed a commit
that referenced
this pull request
Sep 19, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
ovsrobot
pushed a commit
that referenced
this pull request
Oct 9, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
Auto-submission for "http://patchwork.ozlabs.org/project/openvswitch/list/?series=472437"
Summary by Sourcery
Add support for configuring and tracking link speed, duplex, and autonegotiation on DPDK-backed interfaces and expose these options in vswitchd, along with various minor fixes across omitempty modules.
Bug Fixes:
Enhancements:
CI:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores