[PWCI] "app/testpmd: warn on shared rxq switch mismatch" #487
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>
Shared Rx queues do not work with every combination of ports. Besides requiring the corresponding device capability, shared Rx queues also require all ports of one share group and queue ID have the same switch domain and Rx domain. When these fields do not match, shared Rx queues are not properly set up and queue sharing may fail silently. This can happen even in some less intuitive cases like multiple VFs of one physical NIC. To make debugging issues with shared Rx queue configuration easier, this commit introduces simple checks and warning messages for all members of a share_group and share_qid to warn whenever there is a mismatch in the switch and Rx domain. Signed-off-by: Adrian Schollmeyer <a.schollmeyer@syseleven.de> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideRefactors l3fwd burst handling (separate RX/TX burst options), adds eth-link-speed handling to sample apps, tightens mlx5 flex parser and HCA capability handling, introduces shared Rx queue switch-domain mismatch warnings in testpmd, updates several PMDs to consistently use tx_pkt_prepare dummy callbacks, and makes a series of small robustness and documentation/meson build tweaks across examples and drivers. Sequence diagram for l3fwd RX/TX burst size parsing and configurationsequenceDiagram
actor User
participant L3fwdApp as L3fwdApp_main
participant Args as parse_args
participant Burst as parse_pkt_burst
participant PMD as rte_eth_dev_info_get
User->>L3fwdApp: start l3fwd with CLI options
L3fwdApp->>Args: parse_args(argc, argv)
alt RX burst CLI option
Args->>Burst: parse_pkt_burst(optarg, true, &rx_burst_size)
Burst->>Burst: parse optarg to pkt_burst
alt pkt_burst > MAX_PKT_BURST
Burst-->>Args: log and keep rx_burst_size
else pkt_burst > 0
Burst->>Burst: rx_burst_size = pkt_burst
Burst-->>Args: return
else pkt_burst == 0
Burst->>PMD: rte_eth_dev_info_get(0, &dev_info)
PMD-->>Burst: dev_info.default_rxportconf.burst_size
alt burst_size == 0 or burst_size > MAX_PKT_BURST
Burst-->>Args: log and keep rx_burst_size
else valid burst_size
Burst->>Burst: rx_burst_size = burst_size
Burst-->>Args: log PMD RX burst value
end
end
end
alt TX burst CLI option
Args->>Burst: parse_pkt_burst(optarg, false, &tx_burst_size)
Burst->>Burst: parse optarg to pkt_burst
alt pkt_burst > MAX_PKT_BURST
Burst-->>Args: log and keep tx_burst_size
else pkt_burst > 0
Burst->>Burst: tx_burst_size = pkt_burst
Burst-->>Args: return
else pkt_burst == 0
Burst-->>Args: log using default tx_burst_size
end
end
Args-->>L3fwdApp: parsed rx_burst_size, tx_burst_size
L3fwdApp->>L3fwdApp: log final RX and TX burst sizes
L3fwdApp-->>User: l3fwd starts packet processing with configured bursts
Class diagram for updated mlx5 capability and flex parser structuresclassDiagram
class mlx5_hca_flex_attr {
+uint8_t sample_outer_0
+uint8_t sample_outer_1
+uint8_t sample_outer_2
+uint8_t sample_outer_3
+uint8_t sample_inner
+uint8_t sample_tunnel_outer
+uint8_t sample_tunnel_inner
+uint8_t sample_tunnel_inner2
+uint8_t zero_size_supported
+uint8_t sample_id_in_out
+uint8_t header_length_field_mode_wa
+uint16_t max_base_header_length
+uint8_t max_sample_base_offset
+uint16_t max_next_header_offset
}
class mlx5_devx_graph_node_attr {
+uint32_t header_length_mode
+uint32_t header_length_base_value
+uint32_t header_length_field_shift
+uint32_t header_length_field_offset
+uint32_t header_length_field_offset_mode
+uint32_t header_length_field_mask
+mlx5_devx_match_sample_attr sample[MLX5_GRAPH_NODE_SAMPLE_NUM]
+uint32_t next_header_field_offset
}
class mlx5_hca_attr {
+uint32_t log_max_ft_size
+uint32_t log_max_tir
+uint64_t system_image_guid
+uint32_t log_max_conn_track_offload
}
class mlx5_ifc_parse_graph_node_cap_bits {
+u8 max_num_arc_in[8]
+u8 max_num_arc_out[8]
+u8 max_num_sample[8]
+u8 reserved_at_78[1]
+u8 header_length_field_offset_mode[1]
+u8 reserved_at_79[1]
+u8 parse_graph_anchor[1]
+u8 reserved_at_7c[1]
+u8 sample_tunnel_inner2[1]
}
class mlx5_ifc_parse_graph_flex_bits {
+u8 sample_tunnel_inner2[1]
+u8 next_header_field_offset[16]
+u8 reserved_at_160[18]
+u8 head_anchor_id[6]
+u8 reserved_at_178[1]
+u8 header_length_field_offset_mode[1]
+u8 reserved_at_17a[1]
+u8 next_header_field_size[5]
+u8 header_length_field_mask[32]
}
mlx5_hca_flex_attr <.. mlx5_ifc_parse_graph_node_cap_bits : populated_from
mlx5_devx_graph_node_attr <.. mlx5_ifc_parse_graph_flex_bits : encoded_to
mlx5_devx_graph_node_attr <.. mlx5_hca_flex_attr : constrained_by
mlx5_hca_attr <.. mlx5_hca_flex_attr : part_of_overall_caps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughRelease candidate patch (25.11.0-rc4) with diverse changes across multiple components. Includes MLX5 firmware header offset field configuration, RX/TX burst size refactoring in l3fwd examples, TX packet preparation function assignments across multiple drivers, comprehensive documentation updates for platforms and NICs, and Meson build configuration enhancements. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 - here's some feedback:
- In testpmd's rx_queue_setup(), the shared RXQ validation loop iterates q from 0 to nb_rxq for all ports, which looks like the RX queue count for the current port and may not match other ports' queue counts, risking access to uninitialized rxq entries; consider using each port's own RX queue count (e.g., o_port->nb_rxq or o_port->dev_info.nb_rx_queues) as the loop bound.
- In l3fwd, send_single_packet() now uses rx_burst_size as the threshold to flush TX packets, while send_packetsx4() uses tx_burst_size; for consistency and clearer semantics, consider using tx_burst_size for the TX flush threshold in send_single_packet() as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In testpmd's rx_queue_setup(), the shared RXQ validation loop iterates q from 0 to nb_rxq for all ports, which looks like the RX queue count for the current port and may not match other ports' queue counts, risking access to uninitialized rxq entries; consider using each port's own RX queue count (e.g., o_port->nb_rxq or o_port->dev_info.nb_rx_queues) as the loop bound.
- In l3fwd, send_single_packet() now uses rx_burst_size as the threshold to flush TX packets, while send_packetsx4() uses tx_burst_size; for consistency and clearer semantics, consider using tx_burst_size for the TX flush threshold in send_single_packet() as well.
## Individual Comments
### Comment 1
<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):** Bounds check on lcore/queue allows one off-by-one invalid index to slip through.
Because valid lcore and queue IDs are in [0, RTE_MAX_LCORE) and [0, RTE_MAX_QUEUES_PER_PORT), this check should use `>=` instead of `>`. As is, `lcore == RTE_MAX_LCORE` or `queue == RTE_MAX_QUEUES_PER_PORT` will pass the check even though they are out of range, which defeats the validation and can still result in invalid or inconsistent pool names.
</issue_to_address>
### Comment 2
<location> `doc/guides/rel_notes/release_25_11.rst: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):** Correct vendor name spelling for VMware ESXi.
Please update this entry to `VMware ESXi 8.0.3` to match the official product name.
```suggestion
* VMware ESXi 8.0.3
```
</issue_to_address>
### Comment 3
<location> `doc/guides/nics/mlx5.rst:1438-1443` </location>
<code_context>
+ - When creating a rule with a partial `mask` provided in the flow item,
+ the `spec` value must be calculated after the "AND" operation with the `mask`.
+ 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):** Improve grammar in "Such limitation will be removed in future."
Suggest rephrasing to "Such a limitation will be removed in the future." or "This limitation will be removed in the future." for more natural wording.
```suggestion
- When creating a rule with a partial `mask` provided in the flow item,
the `spec` value must be calculated after the "AND" operation with the `mask`.
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.
```
</issue_to_address>
### Comment 4
<location> `doc/guides/nics/mlx5.rst:1413` </location>
<code_context>
+#. When using synchronous flow API,
+ the following limitations and considerations apply:
+
+ - There are limitations on size of match fields.
+ Exceeding these limitations will result in an error,
+ unlike other flow engines (``dv_flow_en`` < 2)
</code_context>
<issue_to_address>
**nitpick (typo):** Slight grammar improvement for "limitations on size of match fields".
Consider rephrasing this bullet to: "There are limitations on the size of the match fields."
```suggestion
- There are limitations on the size of the match fields.
```
</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):** Fix subject-verb agreement in "AMD EPYC SoC support UNCORE power functions".
Change to: "AMD EPYC SoC supports UNCORE power functions ..." so the verb matches the singular subject.
```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):** Improve grammar in "Please refer the tuning guides" sentence.
Suggestion: “Please refer to the tuning guides to enable `HSMP` and the DPDK power management guide.” (add “to” after “refer” and an article before “DPDK power management guide”).
```suggestion
Please refer to the tuning guides to enable ``HSMP`` and the DPDK Power Management Guide.
```
</issue_to_address>
### Comment 7
<location> `doc/guides/linux_gsg/amd_platform.rst:110-119` </location>
<code_context>
+NIC
+---
+
+To use Solarflare x4 on AMD EPYC TURIN, please ensure the following
+
+#. Upgrade Linux or distro version to 6.13 or higher.
+
+#. Disable PCIe power option via ``vfio-pci.disable_idle_d3=1`` in GRUB.
+
+#. Enable the hardware with out-of-tree sfc (contact Solarflare) Linux kernel driver first;
+ then bind with ``vfio-pci`` next.
+
+#. Please use PMD args ``fw_variant=ultra-low-latency,perf_profile=throughput``.
+
+To use Solarflare x2 on AMD EPYC TURIN, please ensure the following
+
+#. Upgrade Linux or distro version to 6.13 or higher.
</code_context>
<issue_to_address>
**suggestion (typo):** Add colons after "please ensure the following" to clearly introduce the subsequent lists.
Since these sentences introduce ordered lists, ending them with a colon (“…please ensure the following:”) would improve grammatical correctness and clarity.
Suggested implementation:
```
To use Solarflare x4 on AMD EPYC TURIN, please ensure the following:
```
```
To use Solarflare x2 on AMD EPYC TURIN, please ensure the following:
```
</issue_to_address>
### Comment 8
<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):** Correct "type of writing" to "time of writing".
'Version at time of writing' is the standard phrasing here.
```suggestion
The specific package for pyelftools is dependent on the version of python in use,
Python 3.11 being the version at 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.
| * Red Hat Enterprise Linux Server release 9.6 | ||
| * Ubuntu 24.04 LTS | ||
| * Ubuntu 24.04.3 LTS | ||
| * Vmware ESXi 8.0.3 |
There was a problem hiding this comment.
suggestion (typo): Correct vendor name spelling for VMware ESXi.
Please update this entry to VMware ESXi 8.0.3 to match the official product name.
| * Vmware ESXi 8.0.3 | |
| * VMware ESXi 8.0.3 |
| 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. |
There was a problem hiding this comment.
issue (typo): Correct "type of writing" to "time of writing".
'Version at time of writing' is the standard phrasing here.
| 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. | |
| The specific package for pyelftools is dependent on the version of python in use, | |
| Python 3.11 being the version at time of writing, hence the ``py311`` prefix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
examples/server_node_efd/shared/common.h (1)
59-64: Clean up now-stale comment about buffer size calculationUsing
RTE_RING_NAMESIZEforbufferis a good, future‑proof change, but the preceding lines still say the size is “calculated by%ubeing replaced by maximum 3 digits (plus an extra byte for safety)”, which is no longer true.I’d update or drop that part of the comment and keep a single, accurate explanation (e.g., “Buffer size matches
RTE_RING_NAMESIZEsince it is used as a ring name”).examples/l3fwd/main.c (1)
930-940: Improve error handling for eth-link-speed parsing.Line 931 uses
atoi, which doesn't provide error handling for invalid input. If the user provides non-numeric input,atoireturns 0, which might silently pass validation checks.Additionally, line 938's validation logic silently ignores invalid speed values by not updating
port_conf.link_speeds. Consider providing explicit feedback to the user.case CMD_LINK_OPT_ETH_LINK_SPEED_NUM: - speed_num = atoi(optarg); + { + char *end = NULL; + errno = 0; + long speed_val = strtol(optarg, &end, 10); + if (errno != 0 || end == optarg || *end != '\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 (speed_num >= 0 && rte_eth_speed_bitflag(speed_num, 0) > 0) + uint32_t bitflag = rte_eth_speed_bitflag(speed_num, 0); + if (bitflag > 0) { - port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); + port_conf.link_speeds = bitflag; + } else { + fprintf(stderr, "Invalid or unsupported speed number: %d\n", speed_num); + print_usage(prgname); + return -1; + } + } break;examples/vdpa/main.c (1)
25-25: Prefix length bounding forifaceis correct; consider surfacing truncationThe new
MAX_VDPA_STR_LENand reducedifacebuffer safely bound"%s%d"sosnprintfintovports[...].ifname[MAX_PATH_LEN]cannot overflow, which should keep-Wformat-truncationhappy.Optional improvement: check the return value of
rte_strscpy(iface, optarg, sizeof(iface))and warn the user if--ifacewas truncated, rather than silently shortening the path.Also applies to: 40-40, 78-80
doc/guides/nics/ice.rst (1)
839-839: Minor typo in example C snippet (flow = flow = ...)Unrelated to this patch but visible here: the example uses
struct rte_flow *flow = flow = rte_flow_create(...);, which is invalid C. It should just initialize fromrte_flow_create, e.g.:- struct rte_flow *flow = flow = rte_flow_create(port_id, &attr, pattern, actions, &error); + struct rte_flow *flow = rte_flow_create(port_id, &attr, pattern, actions, &error);doc/guides/freebsd_gsg/build_dpdk.rst (1)
19-19: FreeBSD pyelftools update looks right; small wording fixSwitching to
py311-pyelftoolsand updating the install command keeps the docs aligned with Python 3.11 on FreeBSD.The note text has a small typo and could read more smoothly. For example:
- 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. + The specific pyelftools package depends on the Python version in use, + with Python 3.11 being the version at the time of writing, hence the ``py311`` prefix.Please confirm
py311-pyelftoolsis available on all FreeBSD versions you intend to support (and consider mentioning the need to adjust the prefix if/when the default Python version changes).Also applies to: 21-24, 28-28
app/test-pmd/testpmd.c (1)
2678-2719: Shared RxQ switch-domain mismatch checks are logically sound; consider minor robustness tweaks.The pre-check that scans all ports/queues when
rx_conf->share_group > 0correctly:
- Filters on matching
share_group/share_qidbefore comparing domains.- Uses existing
ports[pid].dev_info.switch_info.{domain_id,rx_domain}fields, which are already populated in the port init path.- Is confined to configuration time, so the O(#ports · nb_rxq) loop is acceptable.
Two optional improvements:
- Skip
pid == port_idexplicitly to make intent clearer (even though current comparisons never warn on self).- Consider using
TESTPMD_LOG(WARNING, ...)instead offprintf(stderr, ...)for consistency with other logging in this file.Otherwise, the behavior (warnings only, no functional change to queue setup) looks safe and helpful.
doc/guides/linux_gsg/amd_platform.rst (1)
90-105: Clarify uncore support wording to avoid contradicting the new section.You now document
rte_power_uncoresupport (from 25.03) and tested platforms, but the note still says “DPDK uncore support on Linux is work in progress.” That can be read as contradictory.Consider tightening this to e.g. “DPDK uncore support on Linux is under active development; initial support is available from 25.03 on GENOA, SIENA and TURIN” or similar, to align the note with the new Uncore subsection.
drivers/net/mlx5/mlx5.c (1)
1083-1091: SRH flex parser length WA logic looks consistent with new HCA attributesThe conditional handling of
header_length_field_mode_waandheader_length_mask_widthcorrectly distinguishes legacy vs new firmware and keeps all offset adjustments localized to this block. Consider adding a short comment or assertion about the expectedheader_length_mask_widthrange (e.g.1–8) to make the WA assumptions explicit for future maintainers, but functionally this looks sound.doc/guides/nics/mlx5.rst (4)
650-651: Clarify how dv_xmeta_en=3 behaves for sync+HWS usersStating that tunnel offload is “not supported” for the synchronous API with HWS is useful, but it would help to briefly say whether the driver rejects such configurations at
rte_flow_validate()/createtime or silently falls back (e.g. to SWS). That would make the operational impact clearer to users.
1366-1372: Good explicit requirement to call rte_flow_configure() for async APICalling out that
rte_flow_configure()must be invoked before any flow rules when using the async API (and recommending it for sync) is important for HWS. Consider adding a brief cross-reference to the later bullet describing defaultrte_flow_port_attrbehavior so readers connect the recommendation with memory-usage tuning.
1410-1444: Sync+HWS limitations are well captured; consider tightening wording around defaultsThe new bullets concisely describe key differences of the sync API on HWS vs SWS (field-size limits, update semantics, default
rte_flow_port_attr, and partial-mask pitfalls). One minor wording nit: the sentence about “zero flow queues (one is created by default)” can confuse readers; it may be clearer to say that ifrte_flow_configure()is not called, the driver implicitly creates a single internal flow queue of size 32 and allocates maximum counts for configurable actions, and thatrte_flow_configure()should be used to constrain these. Behavior-wise, this section looks consistent.
2947-2949: Clarifying that only METER_MARK is supported on HWS avoids misuseThe note that HWS supports
RTE_FLOW_ACTION_TYPE_METER_MARKbut notRTE_FLOW_ACTION_TYPE_METERis important for users porting configurations from SWS. It would be worth cross-linking to the earlier metering section that explains how to structure policies around METER_MARK, but the limitation itself is clearly stated.
📜 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 (37)
VERSION(1 hunks)app/test-pmd/testpmd.c(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/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
🧰 Additional context used
🧬 Code graph analysis (1)
app/test-pmd/testpmd.c (1)
app/test-pmd/config.c (2)
pid(1218-1220)pid(4611-4704)
⏰ 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)
examples/server_node_efd/efd_server/main.c (1)
71-71: Good fix for MAC address buffer sizing.The change from
sizeof(err_address)(18 bytes) toRTE_ETHER_ADDR_FMT_SIZE + 1correctly sizes the buffer to accommodate the formatted MAC address string with the newline character. Verification confirms thatRTE_ETHER_ADDR_FMT_SIZEis defined as 18 in the DPDK headers, resulting in a buffer size of 19 bytes, which properly accommodates the "XX:XX:XX:XX:XX:XX\n\0" format.drivers/net/txgbe/txgbe_rxtx.c (1)
2340-2357: Simple TX path now advertises a validtx_pkt_preparecallbackSetting
dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;in the simple (no-offload) path makes the TX-prepare hook consistently non-NULL and avoids any caller having to special-case this PMD. This is aligned with the full-featured path (which usestxgbe_prep_pkts) and with similar changes in other drivers; no functional regressions are apparent given the existing constraints on mixed queue configurations.drivers/net/ngbe/ngbe_rxtx.c (1)
1920-1945: Consistent dummytx_pkt_preparefor ngbe simple TX pathAssigning
dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;in the simple TX configuration removes the previous NULL case and aligns this PMD’s behavior with others in the tree. Full-featured queues still usengbe_prep_pkts, so the overall TX-prepare behavior remains coherent.drivers/net/hns3/hns3_rxtx.c (2)
4546-4572: LGTM: Consistent dummy function assignment when device is inactive.The addition of
tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummyon line 4564 properly handles the case when the device is not started or is resetting. This is consistent with the existing pattern of settingtx_pkt_burstandrx_pkt_burstto dummy functions in the same branch, and aligns with the broader changes across multiple drivers to ensure non-NULL TX prepare handlers.
4916-4931: LGTM: Proper TX prepare function handling when stopping datapath.The addition of
tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummyon line 4920 correctly ensures that the prepare function is set to a dummy implementation when the TX datapath is stopped. This is consistent with the existing assignment oftx_pkt_burstto dummy on line 4919, and properly pairs withhns3_start_tx_datapath(line 4937) which restores the real function.doc/guides/sample_app_ug/l3_forward.rst (1)
80-81: LGTM! Documentation accurately reflects new command-line options.The additions of
--rx-burstand--tx-burstoptions are well-documented with clear descriptions and default values.Also applies to: 118-121
examples/l3fwd/l3fwd_common.h (2)
28-30: LGTM! Burst size variables properly declared.The external declarations for
rx_burst_sizeandtx_burst_sizeprovide clean separation of RX and TX burst configuration.
77-77: LGTM! Correct use of tx_burst_size for TX path.The condition properly uses
tx_burst_sizeto determine when to send packets immediately on the transmit path.examples/l3fwd/l3fwd_lpm.c (1)
207-208: LGTM! RX burst size correctly updated.The
rte_eth_rx_burstcall now uses the configurablerx_burst_sizeparameter, enabling per-direction burst control.examples/l3fwd/l3fwd_em.c (1)
646-647: LGTM! RX burst size correctly updated.The
rte_eth_rx_burstcall now uses the configurablerx_burst_sizeparameter.examples/l3fwd/l3fwd_acl.c (1)
1138-1139: LGTM! RX burst size correctly updated.The
rte_eth_rx_burstcall now uses the configurablerx_burst_sizeparameter.examples/l3fwd/l3fwd_fib.c (1)
241-242: LGTM! RX burst size correctly updated.The
rte_eth_rx_burstcall now uses the configurablerx_burst_sizeparameter.examples/l3fwd/l3fwd.h (1)
115-115: LGTM! Public variable declaration updated.The external declaration correctly changes from
nb_pkt_per_bursttorx_burst_size.examples/l3fwd-power/main.c (1)
1958-1970: No issues found. The rte_eth_speed_bitflag API is available and correctly used.The API is properly defined in
lib/ethdev/rte_ethdev.c(exported viaRTE_EXPORT_SYMBOL) and declared inlib/ethdev/rte_ethdev.h. The usage pattern at lines 1968-1969 matches the identical implementation inexamples/l3fwd/main.c(lines 938-939) and is consistent with usage across the ethdev core and multiple drivers throughout the codebase.examples/l3fwd/main.c (5)
60-62: LGTM: Per-direction burst size variables introduced.The introduction of separate
rx_burst_sizeandtx_burst_sizevariables properly supports independent configuration of RX and TX burst sizes.
404-406: LGTM: Help text properly documents new options.The usage text clearly describes the new
--rx-burst,--tx-burst, and--eth-link-speedcommand-line options with appropriate defaults and descriptions.Also applies to: 428-428, 433-436, 468-469
762-762: LGTM: Command-line option infrastructure properly extended.The new command-line options are properly defined with consistent naming conventions and correctly integrated into the parsing infrastructure.
Also applies to: 784-785, 795-795, 817-819, 824-824, 847-848
949-955: LGTM: Burst parsing cases correctly invoke updated function.The command-line parsing cases properly call
parse_pkt_burstwith the correct direction flag and burst size pointer for both RX and TX configurations.
1690-1691: LGTM: Startup logging provides useful configuration visibility.The log statement helpfully displays the configured RX and TX burst sizes at startup, aiding in troubleshooting and configuration verification.
VERSION (1)
1-1: Version bump is consistent with rc4 releaseSingle-line bump to
25.11.0-rc4is straightforward and self‑contained.doc/guides/nics/ionic.rst (2)
2-2: Updated copyright range looks correctExtending to 2018–2025 matches continued maintenance of this guide.
18-19: Pollara 400 model addition is consistent with existing listNew bullet and PDF link follow the same style and indentation as the other models; no issues seen.
doc/guides/cryptodevs/ionic.rst (2)
2-2: Crypto guide copyright year update is fineRange 2021–2025 is consistent with ongoing changes in this file.
19-20: Pollara 400 crypto support entry looks goodThe new model entry and PDF link match the style and wording of the NIC guide, keeping the docs aligned.
examples/meson.build (1)
82-83: Dropping-Wno-format-truncationis reasonable; watch for new warningsSetting
default_cflags = machine_argssimplifies the configuration and lets compilers report real-Wformat-truncationissues in examples.Please ensure CI builds all examples with typical GCC/Clang versions so any newly unmasked format‑truncation warnings are caught early.
doc/guides/meson.build (1)
25-25:install_tag: 'doc'onhtml_guidesis appropriateTagging the Sphinx HTML target as
'doc'aligns it with other documentation install targets and doesn’t affect build behavior.doc/guides/nics/ice.rst (1)
90-91: 25.11 DDP table entries look consistentThe new 25.11 rows for both E810 and E830 follow the existing table format and value conventions; no structural or formatting issues spotted.
Please double‑check the kernel driver/DDP/firmware version tuple against the authoritative Intel DDP release notes, since those versioned dependencies may evolve independently of this doc.
Also applies to: 100-101
doc/guides/nics/i40e.rst (1)
102-141: Additions to kernel/firmware matrix look consistent.The new 25.11 rows for X710/XL710/XXV710 and X722 follow the existing table format and ordering; no issues from a docs or structure standpoint.
doc/api/meson.build (3)
24-31: Tagging API docs targets withinstall_tag: 'doc'is appropriate.Adding
install_tag: 'doc'toexamples.doxkeeps behavior unchanged while enabling finer-grained packaging of documentation artifacts.
71-82:doxygen-htmlinstall tagging is consistent with guides.The
install_tag: 'doc'addition here matches the guides’ Meson changes and won’t affect build or install semantics beyond tagging.
86-96:doxygen-mantagging is consistent and low-risk.Tagging the manpage target with
install_tag: 'doc'is consistent with the other doc targets; no functional impact besides improved install classification.doc/guides/rel_notes/release_25_11.rst (1)
349-600: Tested Platforms matrix is detailed and structurally consistent.The new Intel/NVIDIA/IBM platform lists are well-structured, use the existing release-notes style (vendor → CPU/OS/NIC subsections), and should render correctly in Sphinx; no blocking issues spotted.
doc/guides/linux_gsg/amd_platform.rst (2)
21-27: New BERGAMO|SIENA and TURIN tuning-guide links look fine.The added references to BERGAMO|SIENA and TURIN follow the existing list style and point at the expected AMD tuning PDFs; no structural concerns.
107-130: Turin + Solarflare NIC guidance is clear and actionable.The new Solarflare x4/x2 instructions for AMD EPYC TURIN (kernel ≥ 6.13,
vfio-pci.disable_idle_d3=1, driver binding sequence, and PMD args) are specific and follow the rest of the guide’s style. No blocking issues from a docs perspective.doc/guides/windows_gsg/build_dpdk.rst (4)
28-33: LLVM download text and example link update are fine.Pointing to the generic LLVM releases page and using a current example installer URL keeps this section accurate without changing the overall workflow.
48-52: MinGW-w64 wording tweak is harmless.The “latest installer version” phrasing is clearer while keeping the same recommendation (install in a space-free path like
C:\MinGW); no issues.
65-70: Meson/Ninja MSI installer reference remains correct.The MSI link and surrounding text still describe the recommended Meson+Ninja install path on Windows; no functional change beyond mild formatting.
125-147: Reworked Visual Studio Developer Command Prompt guidance improves clarity.The explicit
VsDevCmd.bat -arch=amd64example plus the note pointing to the VS 2022 docs for PowerShell usage make it easier to get a correctly configured 64‑bit prompt. The build steps below remain unchanged, so this is a safe doc improvement.drivers/common/mlx5/mlx5_devx_cmds.h (2)
108-127: Newheader_length_field_mode_wafield is consistent with flex parser usage.Adding
header_length_field_mode_watomlx5_hca_flex_attrprovides a clean way for higher-level code (e.g.mlx5_flex_translate_length) to decide whether to apply the legacy offset workaround vs. the new hardware mode. Struct layout changes are internal to the driver and align with how the attribute is consumed in the flex code.
645-653:header_length_field_offset_modeaddition matches the new offset-mode semantics.Extending
mlx5_devx_graph_node_attrwithheader_length_field_offset_modeallows the flex parser creation path to select between old and new offset-handling behaviors. Combined with zero-initialization of the struct, leaving this field untouched for non-OFFSET modes is safe.drivers/net/mlx5/mlx5_flow_flex.c (1)
473-605: Offset-mode handling now cleanly distinguishes legacy vs. new hardware behavior.In
FIELD_MODE_OFFSET:
- The existing mask/shift validation and offset adjustment logic is now gated by
attr->header_length_field_mode_wa, so the workaround is only applied when required.node->header_length_field_offset_modeis set to!attr->header_length_field_mode_wa, which cleanly encodes “new offset mode” vs. “legacy workaround” into the DevX graph node attributes.The rest of the validation and assignments are unchanged, so behavior for existing hardware should be preserved while enabling the new mode where supported.
drivers/net/mlx5/mlx5.c (1)
1491-1499: dv_flow_en and allow_duplicate_pattern defaults now match intended behaviorDefaulting
config->dv_flow_en = 1andconfig->allow_duplicate_pattern = 1here, then downgradingdv_flow_enwhensh->dev_cap.dv_flow_enis false, matches the documented semantics (“DV by default if supported, otherwise legacy Verbs”) and the allow‑duplicate default documented later. This should also keepmlx5_probe_again_args_validate()consistent because it reuses the same helper. No functional issues spotted.doc/guides/nics/mlx5.rst (7)
699-701: dv_flow_en default description matches driver behaviorThe documented default (“configured to 1 if supported, else 0”) aligns with
mlx5_shared_dev_ctx_args_config()now initializingdv_flow_ento 1 and then clearing it whensh->dev_cap.dv_flow_enis false. This keeps user expectations and implementation in sync.
824-837: allow_duplicate_pattern default matches implementationDocumenting the default of
allow_duplicate_patternas 1 matches the new initialization inmlx5_shared_dev_ctx_args_config(). The behavioral explanation of modes 0 vs 1 is clear; no issues here.
2396-2403: Sync+HWS representor limitations are clearly statedThe added notes that sync+HWS does not support
PORT_REPRESENTORitems, disallows transfer rules on representor ports, and causes proxy-port rules without explicit represented-port matching to apply to all ports are important caveats. The suggested alternative (TX_QUEUEwith per-queue rules) gives users a workable path.
2593-2596: Explicitly steering MARK users to META for sync+HWS is helpfulDocumenting that
RTE_FLOW_ITEM_TYPE_MARK(16‑bit match) is unsupported with sync+HWS and recommendingMETA(32‑bit) as an alternative makes the limitation concrete and offers a clear migration path. This matches the intent of the HWS metadata model.
2812-2814: Single-report behavior for aged flows under sync+HWS is good to documentStating that aged flows are reported only once in the sync+HWS path sets the right expectation for applications polling
rte_flow_get_aged_flows(). No textual issues; this aligns with typical hardware-aging semantics.
3819-3821: GRE root-table limitation under sync+HWS is clearly scopedDocumenting that
c_rsvd0_vermatching is not supported on group 0 for sync+HWS keeps expectations in line with hardware capabilities. The scope (field + group + steering mode) is precise; no further change needed.
2771-2772: I'll verify these version requirements by searching for current documentation on mlx5 driver compatibility and DOCA versions.Document the source of specific version requirements and consider "X or later" phrasing for future-proofing
The version constraints you've documented (kernel 6.4, rdma-core 60.0, DOCA 3.2.0) don't appear in standard mlx5 driver documentation. The minimal supported rdma-core version is 16, which differs significantly from 60.0. These higher versions are likely specific to the count actions on root tables feature.
To address the staleness concern:
Reference the source: Add a note or link to the driver dependency matrix or NVIDIA documentation that confirms these specific versions are required for this feature.
Clarify the scope: Explicitly state that these versions are for count actions on root tables, distinguishing them from general mlx5 support requirements.
Consider future-proofing: Phrasing as "rdma-core 60.0 or later" would reduce maintenance burden if newer versions become recommended, though this depends on whether newer versions actually support the feature or may introduce breaking changes.
Without access to the specific driver dependency matrix these constraints were based on, I recommend verifying these versions directly with NVIDIA's official documentation and updating the rst file with either inline comments explaining the constraints or a reference to the authoritative source.
drivers/common/mlx5/mlx5_devx_cmds.c (2)
781-799: Flex parser: new header_length_field_offset_mode wiring looks correctUsing
MLX5_SET(parse_graph_flex, ...)forheader_length_field_offset_modeis consistent with the new PRM field and the surrounding flex graph fields; assumingdata->header_length_field_offset_modeis initialized as 0/1, this should program FW correctly with no functional regressions.
884-926: WA flag derivation from parse-graph caps is internally consistent
header_length_field_mode_wabeing set to the logical negation of the FWheader_length_field_offset_modebit aligns with a “workaround-needed-when-bit-is-0” convention and reuses the existing capability query pattern. The newMLX5_GET(parse_graph_node_cap, ..., header_length_field_offset_mode)call matches the updated PRM struct; no issues seen here.drivers/common/mlx5/mlx5_prm.h (3)
2111-2132: New header_length_field_offset_mode bits align with devx usageThe additions of
header_length_field_offset_modeto bothmlx5_ifc_parse_graph_node_cap_bitsandmlx5_ifc_parse_graph_flex_bits, plus the adjusted reserved bits, keep the bitfield packing correct and match the newMLX5_SET/MLX5_GETcalls inmlx5_devx_cmds.c. This gives FW a dedicated 1‑bit flag for the offset mode while preserving surrounding fields’ layout.Also applies to: 4979-4993
1600-1602: No stale steering format references found; review concern not substantiatedThe verification shows all references to
MLX5_STEERING_LOGIC_FORMAT_*constants in the codebase only useCONNECTX_5, which remains defined. No references to removed or undefined steering format constants (CONNECTX_7, CONNECTX_8, etc.) exist in the repository. The code is healthy as-is.Likely an incorrect or invalid review comment.
2351-2357: ESW flow-table caps: offset repositioning is safeVerification confirms no code hardcodes offsets for
ft_header_modify_esw_fdb. The only usage inmlx5_devx_cmds.c:1455accesses the field through theMLX5_GETmacro, which resolves positions dynamically based on struct layout. Widening the leading reserved region is therefore safe.
| /* 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); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Review the boundary condition and log formatting.
Several observations about this validation:
-
Potential off-by-one error: The check uses
lcore > RTE_MAX_LCORE, but sincelcore_queue_confis declared as an array of sizeRTE_MAX_LCORE(line 159), valid indices are 0 toRTE_MAX_LCORE - 1. The condition should likely belcore >= RTE_MAX_LCOREto properly reject the boundary value. -
Missing newline in log message: The error log message at line 889 should end with
\nfor consistent formatting with other log messages in the codebase. -
Comment clarity: The comment mentions "pool name won't be too long," but the primary concern is input validation to prevent out-of-bounds array access, not string length (since
snprintfis bounds-safe anyway).
Apply this diff to address the issues:
- /* 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",
+ /* Defensive check: validate input parameters to prevent out-of-bounds access. */
+ 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;
}To verify the correct boundary values for DPDK constants, run:
🏁 Script executed:
#!/bin/bash
# Description: Verify DPDK constant definitions and usage patterns for lcore and queue bounds.
# Search for RTE_MAX_LCORE definition and usage patterns
echo "=== RTE_MAX_LCORE definition and array declarations ==="
rg -nP 'define\s+RTE_MAX_LCORE' --type=c
echo -e "\n=== Array declarations using RTE_MAX_LCORE ==="
rg -nP '\[RTE_MAX_LCORE\]' --type=c -C2
echo -e "\n=== RTE_MAX_QUEUES_PER_PORT definition ==="
rg -nP 'define\s+RTE_MAX_QUEUES_PER_PORT' --type=c
echo -e "\n=== Boundary check patterns for lcore in similar validation code ==="
rg -nP '(lcore|core_id)\s*(>=|>)\s*RTE_MAX_LCORE' --type=c -C2Length of output: 97541
Fix off-by-one boundary check error in input validation.
The boundary checks at lines 888–890 use > instead of >=, which is inconsistent with DPDK conventions throughout the codebase and creates an off-by-one error. When checking array bounds for lcore_queue_conf[RTE_MAX_LCORE] and similar arrays, the correct condition is lcore >= RTE_MAX_LCORE, not lcore > RTE_MAX_LCORE, to prevent the out-of-bounds index value RTE_MAX_LCORE from passing validation.
Update lines 888–890:
- if (lcore > RTE_MAX_LCORE || queue > RTE_MAX_QUEUES_PER_PORT) {
+ if (lcore >= RTE_MAX_LCORE || queue >= RTE_MAX_QUEUES_PER_PORT) {
RTE_LOG(ERR, IP_RSMBL, "invalid lcore %u or queue %u",Also update the comment to reflect the actual purpose:
- /* Should never happen but check so that pool name won't be too long. */
+ /* Validate input bounds to prevent array out-of-bounds access. */📝 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.
| /* 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); | |
| return -1; | |
| } | |
| /* Validate input bounds to prevent array out-of-bounds access. */ | |
| if (lcore >= RTE_MAX_LCORE || queue >= RTE_MAX_QUEUES_PER_PORT) { | |
| RTE_LOG(ERR, IP_RSMBL, "invalid lcore %u or queue %u", | |
| lcore, queue); | |
| return -1; | |
| } |
🤖 Prompt for AI Agents
In examples/ip_reassembly/main.c around lines 887 to 892, the input validation
uses '>' which allows the sentinel value RTE_MAX_LCORE or
RTE_MAX_QUEUES_PER_PORT to pass and can lead to out-of-bounds access; change the
condition to use '>=' for both lcore and queue (i.e. if (lcore >= RTE_MAX_LCORE
|| queue >= RTE_MAX_QUEUES_PER_PORT)) and update the preceding comment to
accurately describe that this check prevents indexes from being equal to or
exceeding the array size rather than "never happen".
| if (unlikely(len == rx_burst_size)) { | ||
| send_burst(qconf, rx_burst_size, port); |
There was a problem hiding this comment.
Critical: Incorrect burst size variable for TX path.
The send_single_packet function uses rx_burst_size for the TX queue threshold, but should use tx_burst_size instead. This function enqueues packets for transmission, and the burst threshold should match the TX configuration.
This is inconsistent with send_packetsx4 in l3fwd_common.h (line 77), which correctly uses tx_burst_size.
Apply this diff to use the correct TX burst size:
/* enough pkts to be sent */
- if (unlikely(len == rx_burst_size)) {
- send_burst(qconf, rx_burst_size, port);
+ if (unlikely(len == tx_burst_size)) {
+ send_burst(qconf, tx_burst_size, port);
len = 0;
}📝 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.
| if (unlikely(len == rx_burst_size)) { | |
| send_burst(qconf, rx_burst_size, port); | |
| if (unlikely(len == tx_burst_size)) { | |
| send_burst(qconf, tx_burst_size, port); |
🤖 Prompt for AI Agents
In examples/l3fwd/l3fwd.h around lines 151-152, the TX path mistakenly compares
len against rx_burst_size; change this to use tx_burst_size instead so the
send_single_packet function flushes the transmit queue using the configured TX
burst threshold (replace rx_burst_size with tx_burst_size in that conditional).
| if (is_rx_burst) { | ||
| /* If user gives a value of zero, query the PMD for its recommended | ||
| * Rx burst size. | ||
| */ | ||
| ret = rte_eth_dev_info_get(0, &dev_info); | ||
| if (ret != 0) | ||
| return; | ||
| burst_size = dev_info.default_rxportconf.burst_size; | ||
| if (burst_size == 0) { | ||
| RTE_LOG(INFO, L3FWD, "PMD does not recommend a burst size. Using default value %d. " | ||
| "User provided value must be in [1, %d]\n", | ||
| rx_burst_size, MAX_PKT_BURST); | ||
| return; | ||
| } else if (burst_size > MAX_PKT_BURST) { | ||
| RTE_LOG(INFO, L3FWD, "PMD recommended burst size %d exceeds maximum value %d. " | ||
| "Using default value %d\n", | ||
| burst_size, MAX_PKT_BURST, rx_burst_size); | ||
| return; | ||
| } | ||
| *burst_sz = burst_size; | ||
| RTE_LOG(INFO, L3FWD, "Using PMD-provided RX burst value %d\n", burst_size); |
There was a problem hiding this comment.
Major: Hardcoded port 0 in PMD burst query is problematic.
Line 729 queries device info from port 0 unconditionally, which has several issues:
- Port 0 may not exist or may not be enabled in the port mask
- Different ports/PMDs may have different recommended burst sizes
- Could cause a crash or incorrect configuration
Consider one of these approaches:
- Query the first enabled port from
enabled_port_maskinstead of hardcoding port 0 - Defer this query until port initialization when the actual port is known
- Document that port 0 must be enabled when using
--rx-burst 0
Additionally, lines 730-731 silently return on error without logging. Add a warning message to help users diagnose configuration issues.
if (is_rx_burst) {
/* If user gives a value of zero, query the PMD for its recommended
* Rx burst size.
*/
- ret = rte_eth_dev_info_get(0, &dev_info);
- if (ret != 0)
+ /* Find first enabled port to query */
+ uint16_t portid;
+ bool found = false;
+ RTE_ETH_FOREACH_DEV(portid) {
+ if ((enabled_port_mask & (1 << portid)) != 0) {
+ ret = rte_eth_dev_info_get(portid, &dev_info);
+ found = true;
+ break;
+ }
+ }
+ if (!found || ret != 0) {
+ RTE_LOG(WARNING, L3FWD, "Failed to query PMD for RX burst size. Using default value %d\n",
+ rx_burst_size);
return;
+ }📝 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.
| if (is_rx_burst) { | |
| /* If user gives a value of zero, query the PMD for its recommended | |
| * Rx burst size. | |
| */ | |
| ret = rte_eth_dev_info_get(0, &dev_info); | |
| if (ret != 0) | |
| return; | |
| burst_size = dev_info.default_rxportconf.burst_size; | |
| if (burst_size == 0) { | |
| RTE_LOG(INFO, L3FWD, "PMD does not recommend a burst size. Using default value %d. " | |
| "User provided value must be in [1, %d]\n", | |
| rx_burst_size, MAX_PKT_BURST); | |
| return; | |
| } else if (burst_size > MAX_PKT_BURST) { | |
| RTE_LOG(INFO, L3FWD, "PMD recommended burst size %d exceeds maximum value %d. " | |
| "Using default value %d\n", | |
| burst_size, MAX_PKT_BURST, rx_burst_size); | |
| return; | |
| } | |
| *burst_sz = burst_size; | |
| RTE_LOG(INFO, L3FWD, "Using PMD-provided RX burst value %d\n", burst_size); | |
| if (is_rx_burst) { | |
| /* If user gives a value of zero, query the PMD for its recommended | |
| * Rx burst size. | |
| */ | |
| /* Find first enabled port to query */ | |
| uint16_t portid; | |
| bool found = false; | |
| RTE_ETH_FOREACH_DEV(portid) { | |
| if ((enabled_port_mask & (1 << portid)) != 0) { | |
| ret = rte_eth_dev_info_get(portid, &dev_info); | |
| found = true; | |
| break; | |
| } | |
| } | |
| if (!found || ret != 0) { | |
| RTE_LOG(WARNING, L3FWD, "Failed to query PMD for RX burst size. Using default value %d\n", | |
| rx_burst_size); | |
| return; | |
| } | |
| burst_size = dev_info.default_rxportconf.burst_size; | |
| if (burst_size == 0) { | |
| RTE_LOG(INFO, L3FWD, "PMD does not recommend a burst size. Using default value %d. " | |
| "User provided value must be in [1, %d]\n", | |
| rx_burst_size, MAX_PKT_BURST); | |
| return; | |
| } else if (burst_size > MAX_PKT_BURST) { | |
| RTE_LOG(INFO, L3FWD, "PMD recommended burst size %d exceeds maximum value %d. " | |
| "Using default value %d\n", | |
| burst_size, MAX_PKT_BURST, rx_burst_size); | |
| return; | |
| } | |
| *burst_sz = burst_size; | |
| RTE_LOG(INFO, L3FWD, "Using PMD-provided RX burst value %d\n", burst_size); |
🤖 Prompt for AI Agents
In examples/l3fwd/main.c around lines 725-745, the code unconditionally calls
rte_eth_dev_info_get(0, ...) which can target a non-existent/disabled port and
hide errors; change this to find the first enabled port from enabled_port_mask
(or defer the call to when a specific port is being initialized), use that port
id when calling rte_eth_dev_info_get, and if no enabled port is found log a
warning and return; also check the return from rte_eth_dev_info_get and log an
error message (including the port id and ret) instead of silently returning so
users can diagnose configuration issues.
NOTE: This is an auto submission for "app/testpmd: warn on shared rxq switch mismatch".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36785" for details.
Summary by Sourcery
Adjust networking examples, mlx5 driver behavior, and documentation while adding new configuration options and safety checks.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Summary by CodeRabbit
New Features
--rx-burst,--tx-burst, and--eth-link-speedfor flexible configuration.Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.