[PWCI] "fix NBL issues in DPDK 25.11-rc4" #491
Conversation
Enhance the documentation by adding information about limitations specific to the synchronous API when used with hardware steering. This update provides users with a more comprehensive understanding of the constraints in this configuration. Signed-off-by: Maayan Kashani <mkashani@nvidia.com> Signed-off-by: Bing Zhao <bingz@nvidia.com>
Add tested, and hence recommended, firmware and other versions for i40e and ice drivers. In the process remove version inform for obsolete DPDK releases. Signed-off-by: Hailin Xu <hailinx.xu@intel.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
There is the updated firmware providing the new capability bit "header_length_field_offset_mode". If this bit is set the length field offset in flex parser configuration should not be adjusted by the supported field mask left margin, and the bit "header_length_field_offset_mode" should be set in configuration command on the flex parser creation firmware call. Fixes: b04b06f ("net/mlx5: fix flex item header length field translation") Cc: stable@dpdk.org Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
The update of flow engine devargs introduced a regression with untrusted VFs/SFs. Using HWS engine caused a degradation in probing 1K SFs, because memzone segments exceeds maximum 2560. Fixes: 53fdc23 ("common/mlx5: read SW steering capability bits") Fixes: d1ac7b6 ("net/mlx5: update flow devargs handling for future HW") Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
Count flow action is supported on root table with HW Steering flow engine with upstream drivers/libraries and drivers/libraries packaged with recently released DOCA. This patch updates relevant docs with minimum DOCA version. Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com> Acked-by: Bing Zhao <bingz@nvidia.com>
Previously, the Tx burst size in l3fwd was fixed at 256, which could lead to suboptimal performance in certain scenarios. This patch introduces separate --rx-burst and --tx-burst options to explicitly configure Rx and Tx burst sizes. By default, the Tx burst size now matches the Rx burst size for better efficiency and pipeline balance. Fixes: d5c4897 ("examples/l3fwd: add option to set Rx burst size") Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> Tested-by: Venkat Kumar Ande <venkatkumar.ande@amd.com> Tested-by: Dengdui Huang <huangdengdui@huawei.com> Tested-by: Pavan Nikhilesh <pbhagavatula@marvell.com> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
Currently, l3fwd starts in auto-negotiation mode, but it may fail to link up when auto-negotiation is not supported. Therefore, it is necessary to support starting with a specified speed for port. Additionally, this patch does not support changing the duplex mode. So speeds like 10M, 100M are not configurable using this method. Signed-off-by: Dengdui Huang <huangdengdui@huawei.com> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
Currently, l3fwd-power starts in auto-negotiation mode, but it may fail to link up when auto-negotiation is not supported. Therefore, it is necessary to support starting with a specified speed for port. Additionally, this patch does not support changing the duplex mode. So speeds like 10M, 100M are not configurable using this method. Signed-off-by: Dengdui Huang <huangdengdui@huawei.com> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
If format-truncation is enabled, the compiler detects a string overflow
since the snprintf() is putting ethernet address and new line
in buffer only sized for the address.
Since get_rx_queue_name() is used to create a ring name.
The buffer should be sized to be a valid ring name.
This fixes some format-overflow warnings and also corrects
for future errors.
In file included from ../examples/server_node_efd/efd_server/main.c:31:
In function ‘get_printable_mac_addr’,
inlined from ‘do_stats_display’ at ../examples/server_node_efd/efd_server/main.c:136:3:
../lib/net/rte_ether.h:248:36: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
248 | #define RTE_ETHER_ADDR_PRT_FMT "%02X:%02X:%02X:%02X:%02X:%02X"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../examples/server_node_efd/efd_server/main.c:86:33: note: in expansion of macro ‘RTE_ETHER_ADDR_PRT_FMT’
86 | RTE_ETHER_ADDR_PRT_FMT "\n",
| ^~~~~~~~~~~~~~~~~~~~~~
../examples/server_node_efd/efd_server/main.c: In function ‘do_stats_display’:
../examples/server_node_efd/efd_server/main.c:86:59: note: format string is defined here
86 | RTE_ETHER_ADDR_PRT_FMT "\n",
|
Fixes: 39aad0e ("examples/flow_distributor: new example to demonstrate EFD")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
The ifname is limited to 128 characters, but it would allow up
to 128 characters as prefix then could overflow creating ifname.
Change to limit path prefix (iface) to 123 (128 - sizeof("1024"))
to avoid possible format overflow
../examples/vdpa/main.c:501:76: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
501 | snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
| ^
../examples/vdpa/main.c:501:25: note: ‘snprintf’ output between 2 and 139 bytes into a destination of size 128
501 | snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
502 | iface, devcnt);
|
Fixes: 38f8ab0 ("vhost: make vDPA framework bus agnostic")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
In theory, lcore and queue could be so large that mbuf pool name could overflow. But that can never happen since lcore and queue will be in range. Add a check so that static tools know that. Format overflow warnings seen on older versions of Gcc only. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
There were several hidden bugs in examples because format truncation warning was disabled. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
After removing callback checks from ethdev fast path, some drivers crashed because tx_pkt_prepare was set to NULL. Some drivers (hns3, ngbe, txgbe) need to use rte_eth_tx_pkt_prepare_dummy when configuring queues. Other drivers (ntnic, softnic) does not need to set tx_pkt_prepare as it was set by eth_dev_set_dummy_fops() called by rte_eth_dev_allocate(). Bugzilla ID: 1834 Fixes: 066f3d9 ("ethdev: remove callback checks from fast path") Reported-by: Jiawen Wu <jiawenwu@trustnetic.com> Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Reviewed-by: Morten Brørup <mb@smartsharesystems.com> Tested-by: Jiawen Wu <jiawenwu@trustnetic.com>
The Pollara 400 AI NIC is supported without any PMD changes. Add a link to the product brief. Cc: stable@dpdk.org Signed-off-by: Andrew Boyer <andrew.boyer@amd.com>
changes: 1. fix spelling from `SIENNA` to `SIENA` 2. add tuning guide for zen5 AMD Epyc 9005 3. add uncore power details 4. update on using AMD Solarflare X4 and X2 Signed-off-by: Vipin Varghese <vipin.varghese@amd.com> Tested-by: Thiyagarajan P <thiyagarajan.p@amd.com>
Updated some external links and removed a note which does not apply to MSVC builds anymore. Also reworded a sentence clarifying how to open a Visual Studio Developer Command Prompt from CMD and Powershell, as the original text could induce people to run VsDevCmd.bat from a Powershell prompt, which does not work. Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
The note about the prefix of the package pyelftools was not showed in the documentation output because the syntax was missing a colon, so it was considered as a simple comment in the source file. Fixes: 9e6f75e ("doc: update build section of FreeBSD guide") Cc: stable@dpdk.org Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
The version python used in current FreeBSD stable release 14.3 is now 3.11. Update documentation accordingly. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
KNI was removed from DPDK but the diagrams in the documentation were left behind. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Chengwen Feng <fengchengwen@huawei.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
This allows building and installing only the documentation, without recompiling the whole project, using: meson build -Denable_docs=true meson compile -C build doc meson install -C build --no-rebuild --tags doc In Debian/Ubuntu the documentation is built separately from the binaries, in a separate architecture-independent build job, so that it has to be done only once, instead of once per supported architecture. Signed-off-by: Luca Boccassi <bluca@debian.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Add tested platforms with NVIDIA NICs to the 25.11 release notes. Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
Add tested Intel platforms with Intel NICs to v25.11 release note. Signed-off-by: Yu Jiang <yux.jiang@intel.com>
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Queue statistics are being continuously updated in Rx/Tx burst routines while handling traffic. In addition to that, statistics can be reset (written with zeroes) on statistics reset in other threads, causing a race condition, which in turn could result in wrong stats. The patch provides an approach with reference values, allowing the actual counters to be writable within Rx/Tx burst threads only, and updating reference values on stats reset. Fixes: 661c0cc ("net/nbl: support statistics") Signed-off-by: Dimon Zhao <dimon.zhao@nebula-matrix.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideSplits and clarifies burst sizing in l3fwd, fixes NBL stats reset semantics, and updates multiple mlx5, example, and NIC drivers for new HW capabilities, safer buffer sizes, proper tx_pkt_prepare defaults, and documentation install tagging for the 25.11-rc4 release. Class diagram for NBL RX/TX ring stats and reset behaviorclassDiagram
class nbl_res_rx_ring {
+struct nbl_rxq_stats rxq_stats
+struct nbl_rxq_stats rxq_stats_reset
+uint16_t queue_id
+const struct rte_eth_dev *eth_dev
+struct nbl_common_info *common
}
class nbl_res_tx_ring {
+struct nbl_txq_stats txq_stats
+struct nbl_txq_stats txq_stats_reset
+uint16_t queue_id
+const struct rte_eth_dev *eth_dev
+struct nbl_common_info *common
}
class nbl_rxq_stats {
+uint64_t rx_packets
+uint64_t rx_bytes
+uint64_t rx_nombuf
+uint64_t rx_ierror
}
class nbl_txq_stats {
+uint64_t tx_packets
+uint64_t tx_bytes
+uint64_t tx_errors
}
class nbl_resource_mgt {
+struct rte_eth_dev *eth_dev
+int nbl_res_txrx_get_stats(void *priv, struct rte_eth_stats *rte_stats, struct rte_eth_xstat *xstats, struct rte_eth_xstat *qstats)
+int nbl_res_txrx_reset_stats(void *priv)
}
nbl_res_rx_ring --> nbl_rxq_stats : has
nbl_res_rx_ring --> nbl_rxq_stats : has_reset
nbl_res_tx_ring --> nbl_txq_stats : has
nbl_res_tx_ring --> nbl_txq_stats : has_reset
nbl_resource_mgt --> nbl_res_rx_ring : manages
nbl_resource_mgt --> nbl_res_tx_ring : manages
Class diagram for mlx5 flex parser attributes and node configurationclassDiagram
class mlx5_hca_flex_attr {
+uint8_t sample_tunnel_outer:1
+uint8_t sample_tunnel_inner:1
+uint8_t sample_tunnel_inner2:1
+uint8_t zero_size_supported:1
+uint8_t sample_id_in_out:1
+uint8_t header_length_field_mode_wa:1
+uint16_t max_base_header_length
+uint8_t max_sample_base_offset
+uint16_t max_next_header_offset
+uint8_t header_length_mask_width
}
class mlx5_devx_graph_node_attr {
+uint32_t header_length_mode:2
+uint32_t header_length_base_value:16
+uint32_t header_length_field_shift:4
+uint32_t header_length_field_offset:16
+uint32_t header_length_field_offset_mode:1
+uint32_t header_length_field_mask
+uint32_t next_header_field_offset:16
+uint32_t next_header_field_size:5
+struct mlx5_devx_match_sample_attr sample[MLX5_GRAPH_NODE_SAMPLE_NUM]
}
class mlx5_devx_cmd_query_hca_parse_graph_node_cap {
+int mlx5_devx_cmd_query_hca_parse_graph_node_cap(void *ctx, struct mlx5_hca_flex_attr *attr)
}
class mlx5_devx_cmd_create_flex_parser {
+void mlx5_devx_cmd_create_flex_parser(void *ctx, struct mlx5_devx_graph_node_attr *node)
}
class mlx5_flex_translate_length {
+int mlx5_flex_translate_length(struct mlx5_hca_flex_attr *attr, struct mlx5_devx_graph_node_attr *node, const struct rte_flow_item_flex *field, struct rte_flow_error *error)
}
mlx5_devx_cmd_query_hca_parse_graph_node_cap --> mlx5_hca_flex_attr : fills
mlx5_flex_translate_length --> mlx5_hca_flex_attr : reads
mlx5_flex_translate_length --> mlx5_devx_graph_node_attr : sets
mlx5_devx_cmd_create_flex_parser --> mlx5_devx_graph_node_attr : uses
note for mlx5_hca_flex_attr "header_length_field_mode_wa indicates legacy firmware workaround; affects offset calculations"
note for mlx5_devx_graph_node_attr "header_length_field_offset_mode mirrors hardware capability to use direct offset"
Flow diagram for NBL stats get and reset semanticsflowchart LR
subgraph Reset_path
A["nbl_res_txrx_reset_stats(priv)"] --> B["for each RX queue"]
B --> C{"rxq exists?"}
C -->|no| B
C -->|yes| D["rxq->rxq_stats_reset = rxq->rxq_stats"]
D --> B
A --> E["for each TX queue"]
E --> F{"txq exists?"}
F -->|no| E
F -->|yes| G["txq->txq_stats_reset = txq->txq_stats"]
G --> E
end
subgraph Get_path
H["nbl_res_txrx_get_stats(priv, rte_stats, qstats)"] --> I["for each RX queue"]
I --> J{"rxq exists?"}
J -->|no| I
J -->|yes| K["rxq_stats = &rxq->rxq_stats<br/>rxq_stats_reset = &rxq->rxq_stats_reset"]
K --> L["delta_pkts = rxq_stats->rx_packets - rxq_stats_reset->rx_packets"]
K --> M["delta_bytes = rxq_stats->rx_bytes - rxq_stats_reset->rx_bytes"]
K --> N["delta_nombuf = rxq_stats->rx_nombuf - rxq_stats_reset->rx_nombuf"]
K --> O["delta_ierror = rxq_stats->rx_ierror - rxq_stats_reset->rx_ierror"]
L --> P["add deltas into qstats and rte_stats"]
M --> P
N --> P
O --> P
P --> I
H --> Q["for each TX queue"]
Q --> R{"txq exists?"}
R -->|no| Q
R -->|yes| S["txq_stats = &txq->txq_stats<br/>txq_stats_reset = &txq->txq_stats_reset"]
S --> T["delta_pkts = txq_stats->tx_packets - txq_stats_reset->tx_packets"]
S --> U["delta_bytes = txq_stats->tx_bytes - txq_stats_reset->tx_bytes"]
S --> V["delta_errors = txq_stats->tx_errors - txq_stats_reset->tx_errors"]
T --> W["add deltas into qstats and rte_stats"]
U --> W
V --> W
W --> Q
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughVersion bumped to 25.11.0-rc4. Documentation expanded with new platform support (AMD TURIN, Pollara NIC) and updated API guidance. Build system adds documentation install tags. Drivers updated: MLX5 refactored for header length handling and capability parsing; multiple NICs now use dummy prepare functions. L3fwd example refactored for configurable burst sizes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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 found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `drivers/net/mlx5/mlx5.c:1083-1090` </location>
<code_context>
- node.header_length_field_offset = 0x8;
- if (attr->header_length_mask_width < 8)
- node.header_length_field_offset += 8 - attr->header_length_mask_width;
+ if (attr->header_length_field_mode_wa) {
+ /* Legacy firmware before ConnectX-8, we should provide offset WA. */
+ node.header_length_field_offset = 8;
+ if (attr->header_length_mask_width < 8)
+ node.header_length_field_offset += 8 - attr->header_length_mask_width;
+ } else {
+ /* The new firmware, we can specify the correct offset directly. */
+ node.header_length_field_offset = 12;
+ }
node.header_length_field_mask = 0xF;
</code_context>
<issue_to_address>
**issue (bug_risk):** New header_length_field_offset_mode bit is never set in this flex parser path, which can leave it uninitialized.
`header_length_field_offset_mode` is now sent to firmware via:
```c
MLX5_SET(parse_graph_flex, flex, header_length_field_offset_mode,
data->header_length_field_offset_mode);
```
In `mlx5_alloc_srh_flex_parser()`, you only set `node.header_length_field_offset` and never initialize `node.header_length_field_offset_mode` on this stack-allocated struct, so it may carry a garbage value into the command. Please either set `node.header_length_field_offset_mode` explicitly here (e.g. 0 for WA mode, 1 for direct offset mode as in `mlx5_flex_translate_length()`), or ensure `node` is zero-initialized before use so the mode is deterministic.
</issue_to_address>
### Comment 2
<location> `examples/ip_reassembly/main.c:887-888` </location>
<code_context>
nb_mbuf = RTE_MAX(nb_mbuf, (uint32_t)NB_MBUF);
+ /* Should never happen but check so that pool name won't be too long. */
+ if (lcore > RTE_MAX_LCORE || queue > RTE_MAX_QUEUES_PER_PORT) {
+ RTE_LOG(ERR, IP_RSMBL, "invalid lcore %u or queue %u",
+ lcore, queue);
</code_context>
<issue_to_address>
**issue (bug_risk):** Boundary checks on lcore/queue may be off-by-one depending on how RTE_MAX_* macros are defined.
The guard currently uses `>`:
```c
if (lcore > RTE_MAX_LCORE || queue > RTE_MAX_QUEUES_PER_PORT) {
...
}
```
If `RTE_MAX_LCORE` / `RTE_MAX_QUEUES_PER_PORT` are counts (valid indices `[0, MAX-1]`), this should be `>=` so that `lcore == RTE_MAX_LCORE` and `queue == RTE_MAX_QUEUES_PER_PORT` are rejected rather than treated as valid. Please confirm the macro semantics and switch to `>=` if they represent counts, to avoid off-by-one index issues.
</issue_to_address>
### Comment 3
<location> `doc/guides/rel_notes/release_25_11.rst:374-376` </location>
<code_context>
+ * Red Hat Enterprise Linux Server release 9.6
+ * Ubuntu 24.04 LTS
+ * Ubuntu 24.04.3 LTS
+ * Vmware ESXi 8.0.3
+
+ * NICs:
</code_context>
<issue_to_address>
**suggestion (typo):** Use the correct vendor spelling "VMware"
Also use the full product name “VMware ESXi” here to match official branding.
```suggestion
* Ubuntu 24.04 LTS
* Ubuntu 24.04.3 LTS
* VMware ESXi 8.0.3
```
</issue_to_address>
### Comment 4
<location> `doc/guides/nics/mlx5.rst:1443` </location>
<code_context>
+ If more significant bits are present in the `spec` than in the `mask`,
+ the rule will be created without any error
+ but the packets will not hit as expected.
+ Such limitation will be removed in future.
+
</code_context>
<issue_to_address>
**suggestion (typo):** Fix phrasing to "in the future" for grammatical correctness
Consider rephrasing to: "Such limitation will be removed in the future." or "This limitation will be removed in the future." for more natural wording.
Suggested implementation:
```
If more significant bits are present in the `spec` than in the `mask`,
the rule will be created without any error
but the packets will not hit as expected.
This limitation will be removed in the future.
```
If the exact surrounding context differs (for example, if there is an empty line between the last two sentences or slightly different spacing), adjust the SEARCH block to match the actual text in `doc/guides/nics/mlx5.rst` so the replacement applies cleanly. The key change is to replace the phrase `Such limitation will be removed in future.` with `This limitation will be removed in the future.` while preserving indentation and RST formatting.
</issue_to_address>
### Comment 5
<location> `doc/guides/linux_gsg/amd_platform.rst:102` </location>
<code_context>
+Uncore
+~~~~~~
+
+AMD EPYC SoC support UNCORE power functions via ``rte_power_uncore`` from `25.03` onwards.
+These are tested and validated on GENOA, SIENA and TURIN.
+Please refer the tuning guides to enable ``HSMP`` and DPDK power management guide.
</code_context>
<issue_to_address>
**issue (typo):** Correct subject-verb agreement in "AMD EPYC SoC support UNCORE"
Change to: "AMD EPYC SoC supports UNCORE power functions via ``rte_power_uncore`` from `25.03` onwards."
```suggestion
AMD EPYC SoC supports UNCORE power functions via ``rte_power_uncore`` from `25.03` onwards.
```
</issue_to_address>
### Comment 6
<location> `doc/guides/linux_gsg/amd_platform.rst:104` </location>
<code_context>
+
+AMD EPYC SoC support UNCORE power functions via ``rte_power_uncore`` from `25.03` onwards.
+These are tested and validated on GENOA, SIENA and TURIN.
+Please refer the tuning guides to enable ``HSMP`` and DPDK power management guide.
+
+
</code_context>
<issue_to_address>
**suggestion (typo):** Use "refer to" rather than "refer" alone for grammatical correctness
Suggested wording: "Please refer to the tuning guides to enable `HSMP` and to the DPDK power management guide."
```suggestion
Please refer to the tuning guides to enable ``HSMP`` and to the DPDK power management guide.
```
</issue_to_address>
### Comment 7
<location> `doc/guides/freebsd_gsg/build_dpdk.rst:23-24` </location>
<code_context>
- The specific package for pyelftools is dependent on the version of python in use,
- Python 3.8 being the version at type of writing, hence the ``py38`` prefix.
+ The specific package for pyelftools is dependent on the version of python in use,
+ Python 3.11 being the version at type of writing, hence the ``py311`` prefix.
These can be installed using (as root)::
</code_context>
<issue_to_address>
**issue (typo):** Fix typo "type of writing" to "time of writing"
Use "at the time of writing" instead of "at type of writing".
```suggestion
The specific package for pyelftools is dependent on the version of python in use,
Python 3.11 being the version at the time of writing, hence the ``py311`` prefix.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (attr->header_length_field_mode_wa) { | ||
| /* Legacy firmware before ConnectX-8, we should provide offset WA. */ | ||
| node.header_length_field_offset = 8; | ||
| if (attr->header_length_mask_width < 8) | ||
| node.header_length_field_offset += 8 - attr->header_length_mask_width; | ||
| } else { | ||
| /* The new firmware, we can specify the correct offset directly. */ | ||
| node.header_length_field_offset = 12; |
There was a problem hiding this comment.
issue (bug_risk): New header_length_field_offset_mode bit is never set in this flex parser path, which can leave it uninitialized.
header_length_field_offset_mode is now sent to firmware via:
MLX5_SET(parse_graph_flex, flex, header_length_field_offset_mode,
data->header_length_field_offset_mode);In mlx5_alloc_srh_flex_parser(), you only set node.header_length_field_offset and never initialize node.header_length_field_offset_mode on this stack-allocated struct, so it may carry a garbage value into the command. Please either set node.header_length_field_offset_mode explicitly here (e.g. 0 for WA mode, 1 for direct offset mode as in mlx5_flex_translate_length()), or ensure node is zero-initialized before use so the mode is deterministic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
drivers/net/mlx5/mlx5.c (1)
1083-1091: SRH flex parser offset WA looks consistent; consider guarding mask width edge casesThe new
header_length_field_mode_wabranch and the adjustment based onattr->header_length_mask_widthcorrectly separate legacy vs new firmware handling while keeping the non-WA path simple (offset = 12). One minor robustness suggestion: ifheader_length_field_mode_wais set butheader_length_mask_widthis 0 or otherwise unexpected,header_length_field_offsetwill silently drift (e.g., to 16). A defensive check (assert or debug log) whenheader_length_mask_width == 0in the WA path would make misreported capabilities easier to diagnose without changing normal behavior.doc/guides/freebsd_gsg/build_dpdk.rst (1)
19-24: FreeBSD pyelftools update is technically sound; consider future‑proofing noteSwitch to
py311-pyelftoolsand the Python 3.11 note are coherent and consistent with FreeBSD package naming; install snippet matches.If you want to reduce future doc churn, you could slightly reword the note to say that the package name follows the
pyXY-prefix for the active Python version, withpy311-pyelftoolsas the current example.Also applies to: 28-28
examples/ip_reassembly/main.c (1)
887-892: Tighten error path insetup_queue_tbl(leak + log formatting)The new guard on
lcore/queueis reasonable for bounding the mempool name, but:
- On the error path,
rxq->frag_tblhas already been created and is not freed, so this introduces a small leak in the (admittedly “should never happen”) case.- The log message is missing a trailing newline, unlike surrounding
RTE_LOGuses.You could avoid the leak and fix formatting like this:
-static int -setup_queue_tbl(struct rx_queue *rxq, uint32_t lcore, uint32_t queue) +static int +setup_queue_tbl(struct rx_queue *rxq, uint32_t lcore, uint32_t queue) { @@ - socket = rte_lcore_to_socket_id(lcore); - if (socket == SOCKET_ID_ANY) - socket = 0; - - /* Each table entry holds information about packet fragmentation. 8< */ + /* Should never happen but check so that pool name won't be too long. */ + if (lcore > RTE_MAX_LCORE || queue > RTE_MAX_QUEUES_PER_PORT) { + RTE_LOG(ERR, IP_RSMBL, "invalid lcore %u or queue %u\n", + lcore, queue); + return -1; + } + + socket = rte_lcore_to_socket_id(lcore); + if (socket == SOCKET_ID_ANY) + socket = 0; + + /* Each table entry holds information about packet fragmentation. 8< */ @@ - if ((rxq->frag_tbl = rte_ip_frag_table_create(max_flow_num, + if ((rxq->frag_tbl = rte_ip_frag_table_create(max_flow_num, IP_FRAG_TBL_BUCKET_ENTRIES, max_flow_num, frag_cycles, socket)) == NULL) { @@ - return -1; + return -1; }(or alternatively free
rxq->frag_tbljust before returning in the new error branch).examples/l3fwd-power/main.c (1)
1958-1970: Strengthen input validation for eth-link-speed parameter.The current validation has a potential weakness:
atoi()returns 0 for both invalid input (e.g., "abc") and the string "0". The checkspeed_num >= 0would pass for both cases, potentially leading to unexpected behavior.Consider using
strtol()with proper error checking to distinguish between valid zero, invalid input, and out-of-range values:- if (!strncmp(lgopts[option_index].name, - CMD_LINK_OPT_ETH_LINK_SPEED, - sizeof(CMD_LINK_OPT_ETH_LINK_SPEED))) { - speed_num = atoi(optarg); - if ((speed_num == RTE_ETH_SPEED_NUM_10M) || - (speed_num == RTE_ETH_SPEED_NUM_100M)) { - fprintf(stderr, "Unsupported fixed speed\n"); - print_usage(prgname); - return -1; - } - if (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0) - port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); - } + if (!strncmp(lgopts[option_index].name, + CMD_LINK_OPT_ETH_LINK_SPEED, + sizeof(CMD_LINK_OPT_ETH_LINK_SPEED))) { + char *endptr; + errno = 0; + long speed_val = strtol(optarg, &endptr, 10); + if (errno != 0 || endptr == optarg || *endptr != '\0' || + speed_val < 0 || speed_val > INT_MAX) { + fprintf(stderr, "Invalid eth-link-speed value\n"); + print_usage(prgname); + return -1; + } + speed_num = (int)speed_val; + if ((speed_num == RTE_ETH_SPEED_NUM_10M) || + (speed_num == RTE_ETH_SPEED_NUM_100M)) { + fprintf(stderr, "Unsupported fixed speed\n"); + print_usage(prgname); + return -1; + } + if (rte_eth_speed_bitflag(speed_num, 0) > 0) + port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); + else { + fprintf(stderr, "Invalid speed value\n"); + print_usage(prgname); + return -1; + } + }examples/vdpa/main.c (1)
25-25: Clarify the purpose of MAX_VDPA_STR_LEN.The macro
MAX_VDPA_STR_LENis defined assizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS)), which evaluates to the size of the string "1024" (5 bytes). However, the relationship between this value and the iface buffer size reduction is not immediately clear from the code.Could you add a comment explaining why the iface buffer needs to be reduced by this specific amount? For example:
/* Reserve space for port number suffix in socket path construction */ #define MAX_VDPA_STR_LEN sizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS))doc/guides/sample_app_ug/l3_forward.rst (1)
80-81: Docs correctly describe new--rx-burst/--tx-burstoptionsThe new options and default (32) match the implementation and
print_usage()inmain.c. If you want to be exhaustive, you could note that--rx-burst 0falls back to the PMD-recommended RX burst, but current wording is acceptable.Also applies to: 118-121
examples/l3fwd/l3fwd.h (1)
115-116: Consider usingtx_burst_sizefor TX flush threshold instead ofrx_burst_size
send_single_packet()currently flushes whenlen == rx_burst_sizeand sendsrx_burst_sizepackets, even though a separatetx_burst_sizeknob exists. This is safe (both are bounded byMAX_PKT_BURST), but semantically it ties TX behavior to the RX burst configuration.If the intent is for
--tx-burstto control TX behavior consistently, consider:-extern uint32_t rx_burst_size; +extern uint32_t rx_burst_size; +extern uint32_t tx_burst_size; @@ - /* enough pkts to be sent */ - if (unlikely(len == rx_burst_size)) { - send_burst(qconf, rx_burst_size, port); + /* enough pkts to be sent */ + if (unlikely(len == tx_burst_size)) { + send_burst(qconf, len, port); len = 0; }This keeps the buffer bound by
MAX_PKT_BURSTbut lets TX batching follow the TX-specific CLI setting.Also applies to: 139-157
examples/l3fwd/l3fwd_common.h (1)
28-30: TX burst threshold change is correct; consider minor header cleanupUsing
tx_burst_sizein thenum >= tx_burst_size && len == 0fast path correctly wires the new TX burst knob into the multi-packet send path, while still bounding buffered bursts withMAX_PKT_BURST.Two small cleanups you might consider:
- Move the
extern uint32_t rx_burst_size;/tx_burst_size;declarations outside#ifdef DO_RFC_1812_CHECKSso they are clearly available regardless of RFC 1812 checks being enabled.- Optionally, also key the buffered-flush condition (
len == MAX_PKT_BURST) offtx_burst_sizeif you want TX batching behavior to be fully controlled by the CLI knob instead of only the direct-send path.Functionally it's fine as-is; these are just consistency/clarity tweaks.
Also applies to: 77-79
examples/l3fwd/main.c (2)
703-750:parse_pkt_burst()safely constrains and interprets user burst sizesThe helper:
- Validates the CLI value, ignoring malformed input.
- Enforces
pkt_burst <= MAX_PKT_BURST.- Updates
*burst_szonly for positive values or valid PMD-recommended RX burst sizes.- Ensures we never end up with a zero burst size.
For RX, using port 0’s recommended burst as the basis is acceptable for this sample; if you ever want closer alignment with real deployments, you might later consider querying the first enabled port instead.
761-787: CLI wiring for eth-link-speed and RX/TX burst options looks correct
- New macros (
CMD_LINK_OPT_ETH_LINK_SPEED,CMD_LINE_OPT_PKT_RX_BURST,CMD_LINE_OPT_PKT_TX_BURST) and enum values are correctly mapped intolgopts.CMD_LINK_OPT_ETH_LINK_SPEED_NUMcase:
- Rejects 10M/100M explicitly.
- Uses
rte_eth_speed_bitflag(speed_num, 0)and setsport_conf.link_speedsonly for valid values, otherwise silently falls back to auto-negotiation.- RX/TX burst cases call
parse_pkt_burst(optarg, true, &rx_burst_size)andparse_pkt_burst(optarg, false, &tx_burst_size), respectively, matching the option names and semantics.You might later tighten numeric parsing (
strtolwith explicit end-pointer checks) foreth-link-speed, but for a sample app this is sufficient.Also applies to: 789-820, 822-851, 869-883, 930-955
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
doc/guides/prog_guide/img/kni_traffic_flow.pngis excluded by!**/*.pngdoc/guides/prog_guide/img/pkt_flow_kni.pngis excluded by!**/*.png
📒 Files selected for processing (38)
VERSION(1 hunks)doc/api/meson.build(3 hunks)doc/guides/cryptodevs/ionic.rst(2 hunks)doc/guides/freebsd_gsg/build_dpdk.rst(1 hunks)doc/guides/linux_gsg/amd_platform.rst(2 hunks)doc/guides/meson.build(1 hunks)doc/guides/nics/i40e.rst(2 hunks)doc/guides/nics/ice.rst(2 hunks)doc/guides/nics/ionic.rst(2 hunks)doc/guides/nics/mlx5.rst(11 hunks)doc/guides/rel_notes/release_25_11.rst(1 hunks)doc/guides/sample_app_ug/l3_forward.rst(2 hunks)doc/guides/windows_gsg/build_dpdk.rst(4 hunks)drivers/common/mlx5/mlx5_devx_cmds.c(2 hunks)drivers/common/mlx5/mlx5_devx_cmds.h(2 hunks)drivers/common/mlx5/mlx5_prm.h(4 hunks)drivers/net/hns3/hns3_rxtx.c(2 hunks)drivers/net/mlx5/mlx5.c(2 hunks)drivers/net/mlx5/mlx5_flow_flex.c(2 hunks)drivers/net/nbl/nbl_hw/nbl_resource.h(2 hunks)drivers/net/nbl/nbl_hw/nbl_txrx.c(3 hunks)drivers/net/ngbe/ngbe_rxtx.c(1 hunks)drivers/net/ntnic/ntnic_ethdev.c(0 hunks)drivers/net/softnic/rte_eth_softnic.c(0 hunks)drivers/net/txgbe/txgbe_rxtx.c(1 hunks)examples/ip_reassembly/main.c(1 hunks)examples/l3fwd-power/main.c(4 hunks)examples/l3fwd/l3fwd.h(2 hunks)examples/l3fwd/l3fwd_acl.c(1 hunks)examples/l3fwd/l3fwd_common.h(2 hunks)examples/l3fwd/l3fwd_em.c(1 hunks)examples/l3fwd/l3fwd_fib.c(1 hunks)examples/l3fwd/l3fwd_lpm.c(1 hunks)examples/l3fwd/main.c(15 hunks)examples/meson.build(1 hunks)examples/server_node_efd/efd_server/main.c(1 hunks)examples/server_node_efd/shared/common.h(1 hunks)examples/vdpa/main.c(3 hunks)
💤 Files with no reviewable changes (2)
- drivers/net/ntnic/ntnic_ethdev.c
- drivers/net/softnic/rte_eth_softnic.c
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (54)
doc/guides/linux_gsg/amd_platform.rst (5)
25-25: Verify spelling consistency for AMD EPYC generation name.The correction from "SIENNA" to "SIENA" aligns with AMD's official naming (e.g., EPYC 9004 series = "SIENA"). Ensure this spelling is consistent throughout the document and related guides.
27-27: LGTM – TURIN tuning guide entry added.The new TURIN tuning guide link follows the established format and provides the corresponding documentation resource.
91-92: Good addition ofamd_pstatepassive mode detail.Explicitly noting that
amd_pstateoperates in "passive" mode adds important clarity for users. The updated list now correctly includes SIENA and TURIN alongside earlier generations.
99-104: LGTM – Uncore power subsection properly introduced.New subsection clearly documents uncore power function support from 25.03 onwards, validated targets (GENOA, SIENA, TURIN), and points to relevant references (HSMP, power management guide).
107-130: Verify completeness of NIC section documentation.The PR summary mentions adding "Pollara NIC" platform support, but this section only documents Solarflare x4 and x2. Confirm whether Pollara NIC guidance is missing from this file or documented elsewhere.
The new NIC section itself is well-structured with clear prerequisites for each variant:
- Linux version requirement (6.13+)
- PCIe power management configuration
- Driver binding order
- PMD argument recommendations
These details are appropriate for a tuning guide.
doc/guides/nics/i40e.rst (2)
140-141: Verify firmware and driver version accuracy for X722 in 25.11-rc4.The entry is correctly positioned and formatted. Confirm that kernel driver version 2.28.13 and firmware version 6.51 are the validated versions for X722 devices in DPDK 25.11-rc4. Note the firmware version 6.51 differs from X710 series (9.55), which is expected for different device families.
107-108: Firmware and driver versions are accurate for DPDK 25.11.The verification confirms that kernel driver version 2.28.13 and firmware versions 9.55 (X710/XL710/XXV710) and 6.51 (X722) are the correct, officially supported versions for DPDK 25.11. The table entries follow proper RST formatting and are correctly positioned as the newest entries.
drivers/net/nbl/nbl_hw/nbl_resource.h (2)
144-144: LGTM! Well-placed reset statistics field.The
txq_stats_resetfield is correctly typed and logically positioned next to the correspondingtxq_statsfield, enabling delta-based statistics tracking.
186-186: LGTM! Well-placed reset statistics field.The
rxq_stats_resetfield is correctly typed and logically positioned next to the correspondingrxq_statsfield, enabling delta-based statistics tracking.drivers/net/nbl/nbl_hw/nbl_txrx.c (2)
724-775: LGTM! Correct delta-based statistics implementation.The delta calculation approach
(current - reset)is correctly applied to all per-queue statistics for both RX and TX paths. The reset pointers are properly initialized before use, and the implementation maintains consistency across all statistics fields.
777-804: LGTM! Clean reset implementation via snapshotting.The struct assignments on lines 792 and 800 correctly snapshot the current statistics state. This approach, combined with the delta calculations in
nbl_res_txrx_get_stats, properly implements statistics reset without zeroing the underlying counters.drivers/net/ngbe/ngbe_rxtx.c (1)
1920-1935: Simple TX path now safely initializestx_pkt_prepareAssigning
dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;in the simple/no-offload path is correct and aligns with the full-featured path, ensuring upper layers never see a NULLtx_pkt_preparewhile preserving existing behavior (no extra prep work is done). Change looks good.drivers/net/txgbe/txgbe_rxtx.c (1)
2340-2357: Consistent non-NULLtx_pkt_preparefor TXGBE simple pathUsing
rte_eth_tx_pkt_prepare_dummyin the simple/no-offload TX path ensuresdev->tx_pkt_prepareis always valid, matching the full-featured path’s contract and avoiding potential NULL calls from common TX code. This is consistent with the ngbe driver and looks correct.drivers/net/hns3/hns3_rxtx.c (2)
4561-4565: LGTM! Good defensive programming practice.Setting
tx_pkt_preparetorte_eth_tx_pkt_prepare_dummyinstead of NULL when the device is not started or resetting ensures a valid function pointer is always available. This avoids potential null pointer dereferences and aligns with the pattern of using dummy functions for all datapath operations in inactive states.
4917-4921: LGTM! Consistent with datapath stop semantics.Setting
tx_pkt_preparetorte_eth_tx_pkt_prepare_dummywhen stopping the TX datapath ensures uniform handling of TX operations in the stopped state. This complements the dummy burst function assignment and maintains consistency across the driver.examples/server_node_efd/shared/common.h (2)
61-63: Good improvement using standard ring name size constant.The change from a calculated buffer size to
RTE_RING_NAMESIZEis appropriate since this buffer stores ring names. This removes the arbitrary limitation on node IDs and aligns with DPDK's standard naming constraints.
55-67: Based on my verification, I can now provide the rewritten review comment.No thread-safety issue in this context.
The function
get_rx_queue_name()is only called during initialization—inefd_server/init.c:188(to create RX queues) andefd_node/node.c:331(to look up an RX queue)—not during concurrent packet forwarding operations. Additionally, each process has its own copy of the static buffer, eliminating any race condition between processes. While the static buffer pattern is generally non-reentrant, it is used safely here because there are no concurrent calls.examples/server_node_efd/efd_server/main.c (1)
71-71: Review comment is accurate and well-substantiated.Verification confirms all technical claims in the review:
- The old buffer size
sizeof(err_address)= 18 bytes- The
snprintfcall writes "XX:XX:XX:XX:XX:XX\n" (18 characters) + null terminator = 19 bytes needed- The old 18-byte buffer would truncate the trailing newline
- The new size
RTE_ETHER_ADDR_FMT_SIZE + 1= 19 bytes correctly accommodates the formatted outputRTE_ETHER_ADDR_FMT_SIZEis confirmed as 18 inlib/net/rte_ether.h:259The fix is a legitimate buffer overflow correction using proper DPDK idioms.
drivers/net/mlx5/mlx5.c (1)
1490-1498: Behavior change: DV flow and duplicate-pattern now enabled by defaultSetting
config->dv_flow_en = 1andconfig->allow_duplicate_pattern = 1as the baseline (before devargs and capability clamping) changes the default behavior when users do not specify these devargs. The later checks still correctly disable DV flow when unsupported, and probe‑again validation keeps shared contexts consistent, so the change is mechanically safe. Please double‑check that:
- This new default is intended for all supported NIC generations, and
- Documentation and tooling assume DV flow + duplicate‑pattern are effectively “on unless disabled”.
If that’s aligned with the broader series intent, the change looks fine.
drivers/common/mlx5/mlx5_prm.h (4)
1600-1602: Steering logic format constants now macros; values preservedExposing
MLX5_STEERING_LOGIC_FORMAT_CONNECTX_5/_CONNECTX_6DXas macros with explicit0x0/0x1keeps the values stable while decoupling them from enum layout. As long as external code doesn’t depend on the old enum type for type‑checking, this is ABI‑safe.
2111-2132: Parse-graph node caps: new offset-mode bit keeps total width and layout consistentReplacing
reserved_at_78[0x03]withreserved_at_78[0x01],header_length_field_offset_mode[0x1], andreserved_at_79[0x01]preserves the 3‑bit span while surfacing the new capability bit. This keeps the struct size and field ordering intact for existing uses, while enabling the flex‑parser WA logic. Just ensure that the code populatingmlx5_hca_flex_attr.header_length_field_mode_wais wired to this bit (and only this bit).
2351-2357: ESW flow-table caps reserved area widened; downstream fields unaffectedExpanding
reserved_at_0inmlx5_ifc_flow_table_esw_cap_bitsto0x800bits only changes padding at the front of the struct; theft_header_modify_esw_fdband subsequent fields retain their offsets as long as the PRM definition matches. That looks consistent with the accompanying spec changes and should be safe for existing users that rely on those later fields.
4979-4995: Parse-graph flex caps: new header_length_field_offset_mode bit matches node capsThe split of
reserved_at_178[0x3]intoreserved_at_178[0x1],header_length_field_offset_mode[0x1], andreserved_at_17a[0x1]mirrors the earlier change inmlx5_ifc_parse_graph_node_cap_bitsand keeps the total width unchanged. This is important for correctly reporting whether the firmware expects the legacy offset WA vs the new direct‑offset mode. Please confirm the devx capability parsing uses this field consistently when fillingmlx5_hca_flex_attr.header_length_field_mode_wa.drivers/common/mlx5/mlx5_devx_cmds.h (2)
108-127: New flex-cap bit header_length_field_mode_wa integrates cleanly with SRH WAAdding
header_length_field_mode_watomlx5_hca_flex_attrgives a clear, single source of truth for “legacy vs new” firmware behavior and is already consumed inmlx5_alloc_srh_flex_parser(). Since this struct is purely driver‑internal aggregation of PRM caps, there are no ABI concerns; just ensure the DevX capability parsing sets this flag solely based on the newheader_length_field_offset_modebits in the PRM so that the WA path is only taken where really needed.
647-653: Graph-node attr gains header_length_field_offset_mode; verify DevX usage and defaultsThe added
header_length_field_offset_modebit inmlx5_devx_graph_node_attrmatches the new PRM field and is needed to express which interpretation ofheader_length_field_offsetthe firmware expects. From the call sites (e.g. SRH/eCPRI flex parser setup) this field is not explicitly set and will default to 0; that’s fine as long asmlx5_devx_cmd_create_flex_parser()either:
- Ignores this field and derives the correct mode purely from HCA caps, or
- Interprets
0as “legacy WA mode” and1as “new direct-offset mode”, consistent with howheader_length_field_mode_wais filled.If the DevX layer instead expects callers to drive this bit, we should set it alongside
header_length_field_offsetinmlx5_alloc_srh_flex_parser().drivers/net/mlx5/mlx5_flow_flex.c (1)
557-575: LGTM - Conditional gate for firmware capability-based offset handling.The changes correctly implement a firmware capability-based workaround path:
- Line 557: The offset adjustment is now gated by
header_length_field_mode_wa, ensuring the workaround only applies on hardware that requires it.- Line 575: The
header_length_field_offset_modeis correctly set as the inverse of the WA flag, maintaining consistency with the capability query inmlx5_devx_cmd_query_hca_parse_graph_node_cap.The inverse relationship between the workaround flag and the offset mode is correctly maintained throughout the code path.
drivers/common/mlx5/mlx5_devx_cmds.c (2)
787-788: LGTM - Correctly propagates offset mode to DevX command.The new field
header_length_field_offset_modeis correctly written to the flex parser configuration, enabling the firmware to understand how offset handling should be performed.
916-917: LGTM - Correctly derives workaround flag from HCA capability.The inverse relationship between
header_length_field_offset_mode(firmware capability) andheader_length_field_mode_wa(workaround flag) is correctly implemented. When the firmware reports support for the new offset mode, the workaround is disabled, ensuring backward compatibility with older firmware while enabling optimized behavior on newer hardware.doc/guides/nics/mlx5.rst (11)
650-651: Clear and specific limitation note on tunnel offload.The addition properly documents the tunnel offload limitation for synchronous flow API with HW steering. The cross-reference to the HW steering section is correct and placement is appropriate within the
dv_xmeta_enparameter documentation.
699-700: Improved clarity on dv_flow_en default selection.The updated documentation now explicitly states the auto-selection behavior: defaults to 1 (DV flow steering) if supported, otherwise 0 (legacy Verbs). This clarifies the driver's fallback mechanism and is more informative than the previous version.
836-836: Verify intentional default behavior change for allow_duplicate_pattern.The default for
allow_duplicate_patternhas been changed from 0 to 1, which changes the behavior from preventing insertion of duplicate pattern rules to allowing them. While the documentation is updated correctly, this is a significant behavior change that may warrant migration notes or a changelog entry if not already documented elsewhere in the PR.Can you confirm this default change aligns with the PR objectives and that any necessary migration or upgrade notes are in place?
1366-1371: Clear guidance on rte_flow_configure() usage with HW steering.The added note properly distinguishes between the mandatory requirement for asynchronous flow API and the recommendation for synchronous API. Placement in the HW steering Runtime configuration section is appropriate, and the guidance is clear for application developers.
1410-1443: Comprehensive synchronous flow API limitations documentation.The new subsection thoroughly documents synchronous flow API limitations when using HW steering, including match field sizes, rule update semantics, default configuration behavior, and mask handling. The examples clarify the spec/mask interaction, and the note about future removal of limitations indicates active driver development. Guidance on
rte_flow_configure()is appropriately repeated here for context.
2396-2402: Clear limitations for synchronous flow API with HW steering on representor ports.The new limitation item properly documents the unsupported combinations of synchronous API with HW steering and port representor features. Alternatives are provided (using
RTE_FLOW_ITEM_TYPE_TX_QUEUEinstead), making the limitation actionable for developers.
2593-2595: Clear alternative for MARK item limitation with synchronous HW steering.The limitation properly documents that
RTE_FLOW_ITEM_TYPE_MARK(16-bit match) is unsupported with synchronous API and HW steering, and providesRTE_FLOW_ITEM_TYPE_META(32-bit match) as an alternative. The placement as item #1 is appropriate for a core matching capability limitation.
2771-2772: Clear version requirements for count action on HW steering root tables.The requirement note properly specifies version constraints for both upstream and NVIDIA-provided stacks (Linux 6.4+/rdma-core 60.0+ or DOCA 3.2.0+). This helps operators understand prerequisites for advanced features.
2812-2813: Clear documentation of aged flows reporting behavior with synchronous API.The limitation properly documents that aged flows are reported only once when using synchronous flow API with HW steering. This is important for applications expecting repeated aging notifications.
2947-2948: Clear action support distinction for metering with HW steering.The documentation properly clarifies that only
RTE_FLOW_ACTION_TYPE_METER_MARKis supported with HW steering, notRTE_FLOW_ACTION_TYPE_METER. This distinction is important for application developers migrating to HW steering.
3816-3820: Specific GRE field limitation for synchronous flow API with HW steering.The limitation properly documents that matching the
c_rsvd0_verfield is unsupported on root table (group 0) when using synchronous API with HW steering. The scope limitation (group 0 only) is clear and consistent with similar limitations elsewhere in the documentation.VERSION (1)
1-1: Version bump looks correctVersion string update to
25.11.0-rc4is straightforward and consistent with an RC bump.doc/guides/nics/ionic.rst (1)
2-2: Doc updates and Pollara NIC entry look consistentCopyright year extension and the added Pollara 400 AI NIC bullet (with PDF link) match existing style and structure.
Also applies to: 18-19
doc/guides/cryptodevs/ionic.rst (1)
2-2: Crypto guide updates are aligned with NIC docCopyright range and Pollara 400 AI NIC entry are correctly added and formatted.
Also applies to: 19-20
doc/api/meson.build (1)
30-30: Doc install tagging for API targets is consistentAdding
install_tag: 'doc'toexamples.dox,doxygen-html, anddoxygen-manaligns these targets with the guides docs and improves install tagging without affecting build logic.Also applies to: 80-80, 95-95
doc/guides/meson.build (1)
25-25: HTML guides install_tag matches API doc taggingThe new
install_tag: 'doc'onhtml_guidesis consistent with the API docs targets and should help downstream installers categorize documentation.examples/meson.build (1)
81-83: Simplifieddefault_cflags; check for format‑truncation warningsSetting
default_cflags = machine_argssimplifies the examples build, but removing the conditional-Wno-format-truncation(per summary) can re‑expose format‑truncation diagnostics on some compilers, especially if-Werroris in effect.As long as CI/builds are green on your supported compiler set, this simplification looks fine; otherwise, you may need to fix the underlying format issues or reintroduce a targeted suppression where needed.
doc/guides/rel_notes/release_25_11.rst (1)
349-600: LGTM: Comprehensive platform testing documentation.The extensive additions to the tested platforms section provide valuable details about supported hardware configurations, firmware versions, and driver compatibility. The documentation is well-structured with clear sections for Intel and NVIDIA platforms.
doc/guides/windows_gsg/build_dpdk.rst (1)
29-146: LGTM: Documentation improvements enhance clarity and security.The changes improve the Windows build documentation by:
- Updating to HTTPS for security
- Providing specific version links
- Adding detailed Developer Command Prompt guidance
The new VsDevCmd.bat example and PowerShell note are particularly helpful for users.
doc/guides/nics/ice.rst (1)
437-877: Excellent documentation for ICE driver enhancements.The additions provide comprehensive documentation for new ICE features:
- DCB/PFC Configuration: Clear examples for both testpmd CLI and C API usage, with explicit limitations and prerequisites
- Raw Pattern Filtering: Detailed explanation of protocol-agnostic filtering with practical examples, including the VPP tool reference for generating patterns
The documentation is well-structured with appropriate warnings about DDP package dependencies and hardware limitations. The code examples are complete and executable.
examples/l3fwd/l3fwd_fib.c (1)
241-242: LGTM: Consistent with burst size refactoring.The change from
nb_pkt_per_bursttorx_burst_sizeis consistent with the burst sizing refactoring across the l3fwd examples. Assumingrx_burst_sizeis properly initialized and validated (as requested in l3fwd_acl.c review), this change looks correct.examples/l3fwd/l3fwd_lpm.c (1)
207-208: LGTM: Completes consistent burst size refactoring.This change aligns with the identical modifications in
l3fwd_acl.candl3fwd_fib.c, completing the consistent refactoring of burst size handling across all l3fwd forwarding engines. The pattern is now uniform across ACL, FIB, and LPM modes.examples/l3fwd/l3fwd_acl.c (1)
1138-1139: I'll help you verify the concerns raised in this review comment aboutrx_burst_sizeinitialization and bounds checking. Let me start by examining the codebase.
<function_calls>
#!/bin/bashVerify rx_burst_size declaration and initialization
echo "=== Checking rx_burst_size declaration ==="
rg -n "extern.*rx_burst_size|^[^/]*rx_burst_size" examples/l3fwd/ -A 2 -B 2echo -e "\n=== Checking rx_burst_size initialization ==="
rg -n "rx_burst_size\s*=" examples/l3fwd/ -B 2 -A 2echo -e "\n=== Checking MAX_PKT_BURST definition ==="
rg -n "define.MAX_PKT_BURST|MAX_PKT_BURST\s=" examples/l3fwd/echo -e "\n=== Checking rte_eth_rx_burst calls in l3fwd_acl.c ==="
rg -n "rte_eth_rx_burst" examples/l3fwd/l3fwd_acl.c -B 2 -A 2
</function_calls><function_calls>
#!/bin/bashCheck l3fwd_common.h for rx_burst_size declaration
echo "=== Searching l3fwd_common.h ==="
find examples/l3fwd -name "l3fwd_common.h" -exec cat -n {} ;echo -e "\n=== Searching l3fwd.c for initialization ==="
find examples/l3fwd -name "l3fwd.c" -exec grep -n "rx_burst_size" {} +
</function_calls><function_calls>
#!/bin/bashGet full file list to understand structure
echo "=== l3fwd directory structure ==="
find examples/l3fwd -type f -name ".c" -o -name ".h" | sortecho -e "\n=== Search for MAX_PKT_BURST in all files ==="
rg -n "MAX_PKT_BURST" examples/l3fwd/
</function_calls><function_calls>
#!/bin/bashCheck for rx_burst_size in command-line parsing
echo "=== Searching for rx-burst option parsing ==="
rg -n "rx.burst|rx_burst" examples/l3fwd/ -B 3 -A 3echo -e "\n=== Check for bounds checking or validation ==="
rg -n "rx_burst_size.*MAX|MAX.*rx_burst_size" examples/l3fwd/ -B 2 -A 2
</function_calls>examples/l3fwd/l3fwd_em.c (1)
640-648: Switch to configurablerx_burst_sizelooks correctUsing
rx_burst_sizehere aligns the RX path with the new runtime-configurable burst size while remaining safe given it is constrained to<= MAX_PKT_BURST. No functional issues spotted.examples/l3fwd/main.c (2)
57-63: Global RX/TX burst defaults and logging are consistent
rx_burst_size/tx_burst_sizeboth default toDEFAULT_PKT_BURST(32), and the startup log line reports the final values, which matches the help text and doc defaults. No issues here.Also applies to: 1690-1691
404-407: Usage text correctly exposes new burst options and defaultsThe additions to
print_usage()for--rx-burst/--tx-burst(with defaults of 32) and the--eth-link-speedmention line up with the implemented options and global defaults. The placeholder list and “Where” section are in sync with the parser.Also applies to: 428-436, 468-469
| if (!strcmp(long_option[idx].name, "iface")) { | ||
| rte_strscpy(iface, optarg, sizeof(iface)); |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review (78-79)
echo "=== Lines 78-79 of examples/vdpa/main.c ==="
sed -n '75,82p' examples/vdpa/main.c
# Search for macro definitions related to buffer sizes
echo ""
echo "=== Search for MAX_PATH_LEN and MAX_VDPA_STR_LEN ==="
rg -n "MAX_PATH_LEN|MAX_VDPA_STR_LEN" examples/vdpa/main.c
# Look for the iface variable declaration
echo ""
echo "=== Search for iface variable declaration ==="
rg -n -B2 -A2 "char.*iface" examples/vdpa/main.c | head -20
# Look for strncmp usage in the file
echo ""
echo "=== Search for strncmp and strcmp usage ==="
rg -n "strncmp|strcmp" examples/vdpa/main.c
# Look at more context around line 78-79
echo ""
echo "=== More context around lines 60-90 ==="
sed -n '60,90p' examples/vdpa/main.cRepository: ovsrobot/dpdk
<shell_result>
Repository: ovsrobot/dpdk
Exit code: 0
stdout:
=== Lines 78-79 of examples/vdpa/main.c ===
break;
/* long options */
case 0:
if (!strcmp(long_option[idx].name, "iface")) {
rte_strscpy(iface, optarg, sizeof(iface));
printf("iface %s\n", iface);
}
if (!strcmp(long_option[idx].name, "interactive")) {
=== Search for MAX_PATH_LEN and MAX_VDPA_STR_LEN ===
23:#define MAX_PATH_LEN 128
25:#define MAX_VDPA_STR_LEN sizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS))
29: char ifname[MAX_PATH_LEN];
40:static char iface[MAX_PATH_LEN - MAX_VDPA_STR_LEN];
105: char ifname[MAX_PATH_LEN];
111: if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN))
135: char ifname[MAX_PATH_LEN];
140: if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN))
360: rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
501: snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
=== Search for iface variable declaration ===
38-static struct vdpa_port vports[MAX_VDPA_SAMPLE_PORTS];
39-
40:static char iface[MAX_PATH_LEN - MAX_VDPA_STR_LEN];
41-static int devcnt;
42-static int interactive;
=== Search for strncmp and strcmp usage ===
78: if (!strcmp(long_option[idx].name, "iface")) {
82: if (!strcmp(long_option[idx].name, "interactive")) {
111: if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN))
140: if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN))
=== More context around lines 60-90 ===
static struct option long_option[] = {
{"iface", required_argument, NULL, 0},
{"interactive", no_argument, &interactive, 1},
{"client", no_argument, &client_mode, 1},
{NULL, 0, 0, 0},
};
int opt, idx;
char *prgname = argv[0];
while ((opt = getopt_long(argc, argv, short_option, long_option, &idx))
!= EOF) {
switch (opt) {
case 'i':
printf("Interactive-mode selected\n");
interactive = 1;
break;
/* long options */
case 0:
if (!strcmp(long_option[idx].name, "iface")) {
rte_strscpy(iface, optarg, sizeof(iface));
printf("iface %s\n", iface);
}
if (!strcmp(long_option[idx].name, "interactive")) {
printf("Interactive-mode selected\n");
interactive = 1;
}
break;
default:
vdpa_usage(prgname);
return -1;
</shell_result>
Script executed:
#!/bin/bash
# Check git diff to see what actually changed in this PR
echo "=== Git diff for examples/vdpa/main.c (if available) ==="
git diff examples/vdpa/main.c 2>/dev/null | head -100
# If git diff doesn't show changes, try to find the PR/branch info
echo ""
echo "=== Current branch and recent commits ==="
git branch -a 2>/dev/null | head -5
git log --oneline -5 examples/vdpa/main.c 2>/dev/null || echo "No git history"
# Find MAX_VDPA_SAMPLE_PORTS definition
echo ""
echo "=== Search for MAX_VDPA_SAMPLE_PORTS ==="
rg -n "MAX_VDPA_SAMPLE_PORTS" examples/vdpa/main.c
# Get all macro definitions to understand buffer sizes
echo ""
echo "=== All #define statements in main.c ==="
sed -n '20,50p' examples/vdpa/main.cRepository: ovsrobot/dpdk
<shell_result>
Repository: ovsrobot/dpdk
Exit code: 0
stdout:
=== Git diff for examples/vdpa/main.c (if available) ===
=== Current branch and recent commits ===
* (HEAD detached at origin/series_36793)
coderabbit_491
main
remotes/origin/HEAD -> origin/main
remotes/origin/main
581fd829 net/nbl: fix Rx/Tx stats concurrency
=== Search for MAX_VDPA_SAMPLE_PORTS ===
24:#define MAX_VDPA_SAMPLE_PORTS 1024
25:#define MAX_VDPA_STR_LEN sizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS))
38:static struct vdpa_port vports[MAX_VDPA_SAMPLE_PORTS];
110: for (i = 0; i < MAX_VDPA_SAMPLE_PORTS; i++) {
125: if (i >= MAX_VDPA_SAMPLE_PORTS)
139: for (i = 0; i < MAX_VDPA_SAMPLE_PORTS; i++) {
285: for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, devcnt); i++) {
388: for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, devcnt); i++) {
=== All #define statements in main.c ===
#include "commands.h" /* auto-generated file from commands.list */
#include "vdpa_blk_compact.h"
#define MAX_PATH_LEN 128
#define MAX_VDPA_SAMPLE_PORTS 1024
#define MAX_VDPA_STR_LEN sizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS))
#define RTE_LOGTYPE_VDPA RTE_LOGTYPE_USER1
struct vdpa_port {
char ifname[MAX_PATH_LEN];
struct rte_vdpa_device *dev;
int vid;
uint64_t flags;
int stats_n;
struct rte_vdpa_stat_name *stats_names;
struct rte_vdpa_stat *stats;
};
static struct vdpa_port vports[MAX_VDPA_SAMPLE_PORTS];
static char iface[MAX_PATH_LEN - MAX_VDPA_STR_LEN];
static int devcnt;
static int interactive;
static int client_mode;
/* display usage */
static void
vdpa_usage(const char *prgname)
{
printf("Usage: %s [EAL options] -- "
" --interactive|-i: run in interactive mode.\n"
</shell_result>
Script executed:
#!/bin/bash
# Look at how the "iface" option is parsed and used
echo "=== Complete context of iface usage in main ==="
rg -n -B5 -A5 "iface.*optarg\|optarg.*iface" examples/vdpa/main.c
# Check if there's any code that relies on partial matching of option names
echo ""
echo "=== getopt_long usage and option parsing ==="
sed -n '60,95p' examples/vdpa/main.cRepository: ovsrobot/dpdk
<shell_result>
Repository: ovsrobot/dpdk
Exit code: 0
stdout:
=== Complete context of iface usage in main ===
=== getopt_long usage and option parsing ===
static struct option long_option[] = {
{"iface", required_argument, NULL, 0},
{"interactive", no_argument, &interactive, 1},
{"client", no_argument, &client_mode, 1},
{NULL, 0, 0, 0},
};
int opt, idx;
char *prgname = argv[0];
while ((opt = getopt_long(argc, argv, short_option, long_option, &idx))
!= EOF) {
switch (opt) {
case 'i':
printf("Interactive-mode selected\n");
interactive = 1;
break;
/* long options */
case 0:
if (!strcmp(long_option[idx].name, "iface")) {
rte_strscpy(iface, optarg, sizeof(iface));
printf("iface %s\n", iface);
}
if (!strcmp(long_option[idx].name, "interactive")) {
printf("Interactive-mode selected\n");
interactive = 1;
}
break;
default:
vdpa_usage(prgname);
return -1;
}
}
if (iface[0] == '\0' && interactive == 0) {
vdpa_usage(prgname);
</shell_result>
Script executed:
#!/bin/bash
# Check the current diff from origin/main
echo "=== Git diff from main ==="
git diff origin/main examples/vdpa/main.c 2>/dev/null | head -150
# Try to get information about the current series
echo ""
echo "=== Git log for this file in current branch ==="
git log --oneline -10 -- examples/vdpa/main.c 2>/dev/null
# Check git show for the latest commit touching this file
echo ""
echo "=== Show the last commit that touched this file ==="
git show --stat HEAD examples/vdpa/main.c 2>/dev/null | head -50Repository: ovsrobot/dpdk
<shell_result>
Repository: ovsrobot/dpdk
Exit code: 0
stdout:
=== Git diff from main ===
diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index 289db264..08aa9332 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -22,6 +22,7 @@
#define MAX_PATH_LEN 128
#define MAX_VDPA_SAMPLE_PORTS 1024
+#define MAX_VDPA_STR_LEN sizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS))
#define RTE_LOGTYPE_VDPA RTE_LOGTYPE_USER1
struct vdpa_port {
@@ -36,7 +37,7 @@ struct vdpa_port {
static struct vdpa_port vports[MAX_VDPA_SAMPLE_PORTS];
-static char iface[MAX_PATH_LEN];
+static char iface[MAX_PATH_LEN - MAX_VDPA_STR_LEN];
static int devcnt;
static int interactive;
static int client_mode;
@@ -74,9 +75,8 @@ parse_args(int argc, char **argv)
break;
/* long options */
case 0:
- if (strncmp(long_option[idx].name, "iface",
- MAX_PATH_LEN) == 0) {
- rte_strscpy(iface, optarg, MAX_PATH_LEN);
+ if (!strcmp(long_option[idx].name, "iface")) {
+ rte_strscpy(iface, optarg, sizeof(iface));
printf("iface %s\n", iface);
}
if (!strcmp(long_option[idx].name, "interactive")) {
=== Git log for this file in current branch ===
581fd829 net/nbl: fix Rx/Tx stats concurrency
=== Show the last commit that touched this file ===
commit 581fd8295b0abaef724351c88e3b6a68a115b4e3
Author: Dimon Zhao <dimon.zhao@nebula-matrix.com>
Date: Tue Nov 25 18:54:36 2025 -0800
net/nbl: fix Rx/Tx stats concurrency
Queue statistics are being continuously updated in Rx/Tx burst
routines while handling traffic. In addition to that, statistics
can be reset (written with zeroes) on statistics reset in other
threads, causing a race condition, which in turn could result in
wrong stats.
The patch provides an approach with reference values, allowing
the actual counters to be writable within Rx/Tx burst threads
only, and updating reference values on stats reset.
Fixes: 661c0ccf2512 ("net/nbl: support statistics")
Signed-off-by: Dimon Zhao <dimon.zhao@nebula-matrix.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
examples/vdpa/main.c | 525 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 525 insertions(+)
</shell_result>
Script executed:
#!/bin/bash
# Calculate the actual buffer size
echo "=== Calculating MAX_VDPA_STR_LEN ==="
python3 << 'PYTHON_EOF'
# MAX_VDPA_STR_LEN = sizeof(RTE_STR(1024))
# RTE_STR stringifies the value, so it becomes "1024"
# sizeof(RTE_STR(1024)) includes null terminator
stringified = "1024"
str_len_with_null = len(stringified) + 1
max_iface_len = 128 - str_len_with_null
print(f"Stringified MAX_VDPA_SAMPLE_PORTS: '{stringified}'")
print(f"Length with null terminator: {str_len_with_null}")
print(f"MAX_VDPA_STR_LEN: {str_len_with_null}")
print(f"iface buffer size: {max_iface_len}")
PYTHON_EOFRepository: ovsrobot/dpdk
<shell_result>
Repository: ovsrobot/dpdk
Script execution failed with error: Request timed out after 120000ms (requestId=e1a4a74e-cf2a-4ae0-aef2-3462775cc052)
</shell_result>
Script executed:
#!/bin/bash
# Since RTE_STR(1024) stringifies to "1024"
# Let's verify the math:
python3 << 'EOF'
# MAX_VDPA_SAMPLE_PORTS = 1024
# When stringified: "1024" = 4 characters
# sizeof() includes the null terminator for a string literal
# So MAX_VDPA_STR_LEN = 4 + 1 = 5
MAX_PATH_LEN = 128
# The string "1024" has 4 chars, plus null = 5 bytes
MAX_VDPA_STR_LEN = 5 # This is what sizeof(RTE_STR(1024)) would be
new_iface_size = MAX_PATH_LEN - MAX_VDPA_STR_LEN
print(f"Original iface buffer size: {MAX_PATH_LEN}")
print(f"MAX_VDPA_STR_LEN: {MAX_VDPA_STR_LEN}")
print(f"New iface buffer size: {new_iface_size}")
print(f"Reduction: {MAX_PATH_LEN - new_iface_size} bytes")
EOFRepository: ovsrobot/dpdk
<shell_result>
Repository: ovsrobot/dpdk
Exit code: 0
stdout:
Original iface buffer size: 128
MAX_VDPA_STR_LEN: 5
New iface buffer size: 123
Reduction: 5 bytes
</shell_result>
Confirm the intentional buffer size reduction for the iface variable.
The PR changes the option name matching from strncmp(long_option[idx].name, "iface", MAX_PATH_LEN) to strcmp(long_option[idx].name, "iface"), which is correct—using strcmp for comparing option names is the standard approach. However, the iface buffer is also reduced from MAX_PATH_LEN (128 bytes) to MAX_PATH_LEN - MAX_VDPA_STR_LEN (123 bytes).
While this 5-byte reduction is unlikely to affect typical interface names (which are generally ≤15 characters on Linux), it does change the maximum storable length. Verify this size constraint is intentional and document if it impacts supported interface name lengths.
🤖 Prompt for AI Agents
In examples/vdpa/main.c around lines 78-79, the code changed the iface buffer
allocation from MAX_PATH_LEN to MAX_PATH_LEN - MAX_VDPA_STR_LEN, unintentionally
reducing the maximum storable interface name by 5 bytes; either restore the
original buffer size (use MAX_PATH_LEN) so iface can hold the full path-length
capacity, or if the reduction is intentional, add a clear comment documenting
the new limit and adjust any copying calls to use sizeof(iface) and validate
optarg length (fail with a clear error) to prevent truncation.
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36793"
Summary by Sourcery
Update DPDK 25.11-rc4 drivers and sample applications for improved burst configuration, link-speed control, and mlx5 flex parser capabilities, while fixing assorted driver/example bugs and aligning build/docs with the new release.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Chores:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.