[PWCI] "[RFC] test/latencystats: loosen requirements on measured latency" #504
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>
Implement the vlan_offload_set device operation to allow enabling or disabling VLAN offloads through the API. rte_eth_dev_set_vlan_offload() calls the driver to update the hardware configuration. Because this driver performs VLAN stripping entirely in software, only a dummy implementation is needed to avoid returning an error. Validation of unsupported or invalid flags is already handled in ethdev. Fixes: 9d7757d ("net/nbl: simulate VLAN offload") Signed-off-by: Dimon Zhao <dimon.zhao@nebula-matrix.com>
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>
Fix grammar, spelling and formatting of DPDK 25.11 release notes. Signed-off-by: John McNamara <john.mcnamara@intel.com>
If the packet is ether+ipv6+vxlan+ether+ipv4+udp, the parse_l4_proto() function will treat the inner IPv4 header as an IPv6 header. As a result, it cannot identify the inner L4 protocol type, leading to the failure of the inner UDP checksum offload. This commit changes the handling of the inner layer to match that of the outer layer, allowing for the reuse of subsequent processes. It also fixes the issue where the L4 could not be identified in the case of an ether+ipv4+vxlan+ether+ipv6+udp encapsulation. Bugzilla ID: 1813 Fixes: 4961596 ("app/testpmd: fix L4 protocol retrieval from L3 header") Fixes: 76730c7 ("app/testpmd: use packet type parsing API") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Tested-by: Shani Peretz <shperetz@nvidia.com> Tested-by: Mingjin Ye <mingjinx.ye@intel.com>
1001s Running binary with argv[]:'/tmp/autopkgtest.LvdXhc/build.6vT/src/build/app/dpdk-test' '--file-prefix=debug' '-l' '0' '--no-huge' '-m' '2048' <...> 1001s EAL: Cannot use IOVA as 'PA' since physical addresses are not available This is similar to the commit 6b07762 ("test: force IOVA mode on PPC64 without huge pages") on ppc64 iova-mode=pa is the default, but it doesn't work with --no-huge, so add --iova-mode=va to the test arguments. Fixes: 2b403dd ("test/debug: fix crash with mlx5 devices") Cc: stable@dpdk.org Signed-off-by: Luca Boccassi <bluca@debian.org> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
With latest glibc (Fedora Rawhide):
drivers/bus/fslmc/fslmc_bus.c: In function ‘rte_fslmc_parse’:
drivers/bus/fslmc/fslmc_bus.c:298:15: error:
assignment discards ‘const’ qualifier from pointer target type
298 | t_ptr = strchr(sep, '.');
| ^
Fixes: e67a616 ("bus/fslmc: support device iteration")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Acked-by: David Marchand <david.marchand@redhat.com>
Use device configured Rx offloads instead of hardcoded values while draining work. Fixes: e8594de ("event/cnxk: implement event port quiesce function") Cc: stable@dpdk.org Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com> Acked-by: Jerin Jacob <jerinj@marvell.com>
For different devices, the mailbox flow between software and firmware is different. There are several mailbox commands in the txgbe driver, but only txgbe_hic_sr_read() was changed to use the new flow. It leads to other mailbox commands timeout for Amber-Lite devices, which is required to use the new flow. So this patch fills in the missing part. For the sake of code tidy, txgbe_hic_sr_read() should change to use the locked function txgbe_host_interface_command(), and this function could be change to pointer in struct txgbe_mbx_info for different devices. Fixes: 6a139ad ("net/txgbe: add new SW-FW mailbox interface") Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
This reverts commit d21c2fe. While fixing an issue with enabling Rx timestamping on PFs which don't report Rx timestamping, the offending commit broke PFs which do support Rx timestamping. Fixes: d21c2fe ("net/iavf: fix check for PF Rx timestamp support") Suggested-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com> Acked-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
On QEMU Arm there are failures because the minimum measured latency was zero. This is likely because the TSC counter emulation is coarse enough that successive measurements return same value. Modify the test assertions to allow for TSC being slow (or even constant). Also, fix the test to cleanup before validating results and print the values if a failure occurs. The field name values printing loop was an earlier debug step not needed. Fixes: b34508b ("test/latency: update with more checks") Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThis pull request includes a version bump to 25.11.0-rc4, adds BPF maintainers tracking, enhances packet parsing for tunnel scenarios, refactors hardware capability detection in MLX5 drivers, updates event and network drivers with initialization fixes and new callbacks, and extends documentation with platform/feature updates and release notes. Additionally, l3fwd example application is enhanced with configurable RX/TX burst sizing and link speed options. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/meson.build (1)
1-1: PR title doesn't match the file changes.The PR title references "test/latencystats: loosen requirements on measured latency", but this file is
examples/meson.build. Please clarify whether:
- This is part of a larger changeset where other files modify latencystats, or
- The PR metadata is incorrect
drivers/net/nbl/nbl_hw/nbl_txrx.c (1)
21-27: Fix incorrect NULL check after allocatingrx_ringsIn
nbl_res_txrx_alloc_rings, Line 23 currently checkstxrx_mgt->tx_ringsinstead oftxrx_mgt->rx_ringsafter allocating the RX ring array. This means RX allocation failures are not detected and can later cause NULL dereferences.Recommend:
- txrx_mgt->rx_rings = rte_calloc("nbl_rxrings", rx_num, - sizeof(struct nbl_res_rx_ring *), 0); - if (!txrx_mgt->tx_rings) { + txrx_mgt->rx_rings = rte_calloc("nbl_rxrings", rx_num, + sizeof(struct nbl_res_rx_ring *), 0); + if (!txrx_mgt->rx_rings) { NBL_LOG(ERR, "Allocate the rx rings array failed"); rte_free(txrx_mgt->tx_rings); return -ENOMEM; }
🧹 Nitpick comments (9)
doc/guides/linux_gsg/amd_platform.rst (2)
99-105: Clarify Uncore feature availability and reference documentation.The new Uncore subsection references
rte_power_uncorefrom "25.03" onwards. Verify that this version number is accurate for the DPDK release cycle. Additionally, the note to "refer the tuning guides to enable HSMP and DPDK power management guide" lacks specific links or section references. Consider adding explicit links to these guides or inline documentation for improved user experience.
107-130: Add context for NIC section and validate technical prerequisites.The new NIC section introduces Solarflare x4 and x2 prerequisites for TURIN but lacks context explaining why these specific settings are needed or how they impact performance. Additionally, verify:
- Linux/distro version requirement "6.13 or higher" is stable and documented.
- The
vfio-pci.disable_idle_d3=1GRUB parameter is the correct and supported mechanism.- The PMD args
fw_variant=ultra-low-latency,perf_profile=throughputare current and match Solarflare driver documentation.Consider adding brief explanations for each prerequisite to help users understand the rationale.
examples/l3fwd/main.c (3)
930-940: Usestrtoulinstead ofatoifor robust error handling.
atoi()returns 0 on parse error, making it impossible to distinguish between invalid input and a literal "0". Other parsers in this file (e.g.,parse_queue_size,parse_mbcache_size) usestrtoulwith proper error checking.Additionally, invalid speed values are silently ignored without warning the user.
case CMD_LINK_OPT_ETH_LINK_SPEED_NUM: - speed_num = atoi(optarg); + { + char *end = NULL; + long speed_val = strtol(optarg, &end, 10); + if ((optarg[0] == '\0') || (end == NULL) || (*end != '\0')) { + 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) + if (speed_num > 0 && rte_eth_speed_bitflag(speed_num, 0) > 0) { port_conf.link_speeds = rte_eth_speed_bitflag(speed_num, 0); + } else if (speed_num != 0) { + fprintf(stderr, "Invalid eth-link-speed: %d\n", speed_num); + print_usage(prgname); + return -1; + } + } break;
762-762: Minor naming inconsistency.
CMD_LINK_OPT_ETH_LINK_SPEEDuses a different prefix (CMD_LINK_OPT_) compared to other options which useCMD_LINE_OPT_. This inconsistency may cause confusion.-#define CMD_LINK_OPT_ETH_LINK_SPEED "eth-link-speed" +#define CMD_LINE_OPT_ETH_LINK_SPEED "eth-link-speed"Similarly update
CMD_LINK_OPT_ETH_LINK_SPEED_NUMtoCMD_LINE_OPT_ETH_LINK_SPEED_NUMfor consistency.
428-428: Consider documenting valid values for eth-link-speed.The help text "force link speed" doesn't explain what values are accepted or which speeds are unsupported (10M, 100M). Users may not know what to pass.
- " --eth-link-speed: force link speed\n" + " --eth-link-speed SPEED: Force link speed in Mbps (e.g., 1000, 10000, 25000, 40000, 100000).\n" + " Note: 10 and 100 Mbps are not supported.\n"app/test/test_latencystats.c (1)
186-205: Resource cleanup and relaxed latency ordering checks look correctStopping the port and returning mbufs before assertions is safer, and replacing strict non‑zero checks with
Min <= Avg <= Maxguarded bysamples > 0is reasonable for coarse TSC environments. If you touch this again, consider using named constants or an enum for metric indices instead of barevalues[0/1/2/4]for clarity.app/test/test_debug.c (1)
49-49: Extendedtest_argsand PPC64 IOVA override look saneResizing
test_argsto 8 and using the new slot for--iova-mode=vaonRTE_ARCH_PPC_64with--no-hugesolves the known IOVA=PA issue while preserving existing behavior elsewhere (empty-string placeholders). If others edit this area later, a brief comment on howprocess_dupinterprets empty entries could help avoid misunderstandings, but it’s not required.Also applies to: 157-171
drivers/common/mlx5/mlx5_devx_cmds.c (2)
785-803: Wiringheader_length_field_offset_modeinto flex‑parser object is correct; verify caller initializationPopulating
parse_graph_flex.header_length_field_offset_modefromdata->header_length_field_offset_modeis consistent with the new PRM bit and the struct definition inmlx5_prm.h. Theinbuffer is zero‑initialized so older firmware will still see0if callers leave the new field at its default.Please just double‑check that all call sites of
mlx5_devx_cmd_create_flex_parser()initializeheader_length_field_offset_modeinstruct mlx5_devx_graph_node_attr(or consciously rely on the default), to avoid programming an unintended mode on hardware that supports this field.
884-917: Clarify semantics ofheader_length_field_mode_wainversion
header_length_field_mode_wais currently set as the logical negation of the 1‑bit capability:attr->header_length_field_mode_wa = !MLX5_GET(parse_graph_node_cap, hcattr, header_length_field_offset_mode);This is likely intentional (WA enabled when the HCA does not advertise the new offset mode), but it’s not obvious from the name alone, and future readers could easily “simplify” it incorrectly.
I recommend:
Using an explicit comparison for readability, e.g.:
attr->header_length_field_mode_wa = (MLX5_GET(parse_graph_node_cap, hcattr, header_length_field_offset_mode) == 0);Optionally adding a brief comment tying this to the PRM field semantics (when the WA is required), so it’s clear that
1means “workaround active because device uses legacy header‐length behavior”.Also please verify against the latest PRM that
header_length_field_offset_modeis indeed a single‑bit boolean and that “0 => WA needed, 1 => no WA” is the intended mapping used by the downstream code that consultsheader_length_field_mode_wa.
📜 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 (60)
MAINTAINERS(1 hunks)VERSION(1 hunks)app/test-pmd/csumonly.c(1 hunks)app/test/test_debug.c(2 hunks)app/test/test_latencystats.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(6 hunks)doc/guides/sample_app_ug/l3_forward.rst(2 hunks)doc/guides/windows_gsg/build_dpdk.rst(4 hunks)drivers/bus/fslmc/fslmc_bus.c(1 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/event/cnxk/cn10k_eventdev.c(3 hunks)drivers/event/cnxk/cn10k_worker.h(1 hunks)drivers/event/cnxk/cn20k_eventdev.c(3 hunks)drivers/event/cnxk/cn20k_worker.h(1 hunks)drivers/net/cnxk/cn10k_rx.h(2 hunks)drivers/net/cnxk/cn20k_rx.h(2 hunks)drivers/net/hns3/hns3_rxtx.c(2 hunks)drivers/net/intel/iavf/iavf_ethdev.c(1 hunks)drivers/net/mlx5/mlx5.c(2 hunks)drivers/net/mlx5/mlx5_flow_flex.c(2 hunks)drivers/net/nbl/nbl_dev/nbl_dev.c(1 hunks)drivers/net/nbl/nbl_dev/nbl_dev.h(1 hunks)drivers/net/nbl/nbl_ethdev.c(1 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/base/txgbe_aml.c(2 hunks)drivers/net/txgbe/base/txgbe_aml40.c(2 hunks)drivers/net/txgbe/base/txgbe_eeprom.c(8 hunks)drivers/net/txgbe/base/txgbe_hw.c(2 hunks)drivers/net/txgbe/base/txgbe_mng.c(11 hunks)drivers/net/txgbe/base/txgbe_mng.h(1 hunks)drivers/net/txgbe/base/txgbe_type.h(1 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 (5)
drivers/net/nbl/nbl_dev/nbl_dev.h (1)
drivers/net/nbl/nbl_dev/nbl_dev.c (1)
nbl_vlan_offload_set(761-768)
drivers/net/txgbe/base/txgbe_aml.c (1)
drivers/net/txgbe/base/txgbe_mng.c (1)
txgbe_host_interface_command_aml(163-272)
drivers/net/nbl/nbl_ethdev.c (1)
drivers/net/nbl/nbl_dev/nbl_dev.c (1)
nbl_vlan_offload_set(761-768)
drivers/net/txgbe/base/txgbe_mng.h (1)
drivers/net/txgbe/base/txgbe_mng.c (2)
txgbe_host_interface_command_sp(99-161)txgbe_host_interface_command_aml(163-272)
app/test-pmd/csumonly.c (1)
lib/mbuf/rte_mbuf.h (1)
rte_pktmbuf_read(1837-1844)
🔇 Additional comments (80)
doc/guides/cryptodevs/ionic.rst (2)
2-2: Copyright year updated appropriately.The annual copyright year update is correct and follows standard practice.
19-20: I'll verify the review comment by checking the URL accessibility and the file context. Let me start by examining the file and validating the URL.
<function_calls>
cat -n doc/guides/cryptodevs/ionic.rst | head -30
</function_calls>Let me search for the file first:
<function_calls>
find . -name "ionic.rst" -type f
</function_calls>Now let me check the URL accessibility:
<function_calls>
AMD Pollara 400 product brief PDF site:amd.com
</function_calls>doc/guides/meson.build (1)
25-25: LGTM! Documentation install tagging added.The addition of
install_tag: 'doc'enables better categorization of documentation artifacts during installation, consistent with similar changes indoc/api/meson.build.doc/guides/nics/ice.rst (2)
90-91: LGTM! E810 compatibility information updated for DPDK 25.11.The new row documents the kernel driver (2.3.14), DDP package versions, and firmware (4.9) compatible with DPDK 25.11 for E810 controllers.
100-101: LGTM! E830 compatibility information updated for DPDK 25.11.The new row documents the kernel driver (2.3.14), DDP package versions, and firmware (1.2) compatible with DPDK 25.11 for E830 controllers.
doc/api/meson.build (1)
30-30: LGTM! Documentation install tagging added to all API doc targets.The addition of
install_tag: 'doc'to the examples.dox, doxygen-html, and doxygen-man custom targets enables consistent categorization of API documentation artifacts during installation, aligning with the tagging indoc/guides/meson.build.Also applies to: 80-80, 95-95
doc/guides/nics/i40e.rst (2)
107-108: LGTM! X710/XL710/XXV710 compatibility information updated for DPDK 25.11.The new row documents the kernel driver (2.28.13) and firmware (9.55) versions compatible with DPDK 25.11 for X710/XL710/XXV710 controllers.
140-141: LGTM! X722 compatibility information updated for DPDK 25.11.The new row documents the kernel driver (2.28.13) and firmware (6.51) versions compatible with DPDK 25.11 for X722 controllers.
doc/guides/nics/ionic.rst (2)
2-2: Copyright year update looks good.The year has been appropriately updated to reflect 2025.
18-19: New product entry is properly formatted and consistent; URL accessibility requires manual verification outside sandbox environment.The Pollara 400 AI NIC entry on lines 18-19 follows the established documentation pattern with correct reStructuredText link syntax and maintains consistency with other model entries (DSC-25, DSC-100, DSC2-200, DSC3-400). The URL path structure matches the pattern used for other recent product briefs (DSC3-400, DSC2-200).
URL accessibility cannot be verified in the sandbox environment due to external domain access restrictions, but the entry is otherwise complete and correct. Consider verifying the PDF link manually or through production deployment.
doc/guides/rel_notes/release_25_11.rst (4)
12-170: Release note refinements are clear and well-structured.The feature descriptions have been improved with clearer wording, explicit feature states (e.g., line 40 "This feature is disabled by default"), and better context. The new feature entries are comprehensive and appropriately formatted for release notes.
173-200: Removed items documentation includes helpful migration guidance.The removals are clearly documented with context and where applicable, migration paths or alternatives are provided (e.g., using xstats API instead of removed queue stats fields; using RTE_FLOW_ACTION_TYPE_RSS instead of repr_matching_en). This supports user transition effectively.
202-234: API and ABI changes are well-documented with clear context.Breaking changes are described with sufficient detail to help developers understand the impact. Examples include explaining why SM2 struct member changed to a union (to support partial calculation) and that pcapng link type was previously hardcoded. This supports downstream adoption.
237-491: Tested Platforms section provides comprehensive test matrix.The expansion from "Known Issues" to detailed "Tested Platforms" is a valuable improvement. The matrix documents tested CPU types, OS versions, driver versions, firmware versions, and device specifications across multiple platform families and NIC vendors. This gives users clear visibility into the tested scope and helps them validate their own deployments.
The hierarchical structure (platforms → CPU/OS → NICs with firmware details) is well-organized and easy to scan. Inclusion of both in-tree and out-of-tree driver versions, firmware versions, and device IDs provides sufficient technical depth for compatibility assessment.
drivers/net/cnxk/cn20k_rx.h (2)
310-310: LGTM: Explicit initialization improves safety.Initializing
sgto 0 at declaration prevents potential undefined behavior and is a good defensive programming practice in receive path code.
848-850: LGTM: Consolidated initialization ensures defined starting values.Explicitly initializing these variables and declaring them early ensures they have safe default values before use in conditional security offload code paths.
drivers/event/cnxk/cn20k_worker.h (1)
55-59: LGTM: Explicit initialization prevents undefined behavior.Initializing these variables at declaration (especially the
mppointer to NULL) ensures safe defaults for use in conditional security offload code paths.drivers/net/cnxk/cn10k_rx.h (2)
575-575: LGTM: NULL initialization improves pointer safety.Initializing
inb_privto NULL at declaration prevents potential use of uninitialized pointer, which is critical for security processing code.
1467-1468: LGTM: Explicit initialization ensures safe defaults.Initializing these variables at declaration ensures they have defined values before use in conditional security offload and LMT operation code paths.
drivers/event/cnxk/cn10k_worker.h (1)
55-58: LGTM: Consistent initialization pattern improves code safety.Explicit initialization of these variables (especially the
mppointer to NULL) ensures safe defaults for conditional security offload operations. Changes are consistent with the corresponding cn20k implementation.doc/guides/linux_gsg/amd_platform.rst (2)
91-92: Verify platform list consistency and power driver documentation.Line 91 updates the tested platforms from "MILAN, GENOA, BERGAMO" to "MILAN, GENOA, BERGAMO, SIENA and TURIN". Confirm this addition aligns with actual testing and support status. Additionally, the mention of "amd_pstate" in "passive" mode (line 92) should be validated against the referenced tuning guides and DPDK power management documentation.
25-27: Typo correction and TURIN link are accurate. No issues found.The correction from "BERGAMO|SIENNA" to "BERGAMO|SIENA" is correct—AMD's official documentation confirms "Siena" as the codename for the EPYC 8004 series. The TURIN tuning guide link (line 27) follows the standard AMD tuning guide URL pattern and is consistent with existing entries. All platform references throughout the file (lines 25, 27, 91, 103, 110, 121) use "SIENA" consistently with no spelling variations.
examples/server_node_efd/shared/common.h (1)
58-67: RTE_RING_NAMESIZE is properly available; verification request is unnecessary.The code change is correct and idiomatic. All source files that call
get_rx_queue_name()explicitly include<rte_ring.h>before includingcommon.h:
efd_node/node.c: includes<rte_ring.h>at line 25, then"common.h"at line 35efd_server/init.c: includes<rte_ring.h>at line 24, then"common.h"at line 38efd_server/main.c: includes<rte_ring.h>at line 25, then"common.h"at line 40The
get_rx_queue_name()function is only invoked ininit.candnode.c, both of which properly include the required header. No additional includes are needed.Likely an incorrect or invalid review comment.
examples/server_node_efd/efd_server/main.c (1)
71-71: Buffer sizing corrected for MAC address with newline.The change to
RTE_ETHER_ADDR_FMT_SIZE + 1is correct. TheRTE_ETHER_ADDR_FMT_SIZEconstant is defined as 18 bytes (confirmed inlib/net/rte_ether.h:259), which accounts for the MAC address format "XX:XX:XX:XX:XX:XX" (17 chars) plus null terminator (1 byte).Since the format string at line 86 adds a newline (
RTE_ETHER_ADDR_PRT_FMT "\n"), the buffer requires 19 bytes total: 17 for the MAC format + 1 for the newline + 1 for the null terminator. The old size ofsizeof(err_address)(18 bytes) was insufficient and risked buffer overflow.drivers/bus/fslmc/fslmc_bus.c (1)
241-311: Const-qualifyingt_ptris correct and behavior-preservingChanging
t_ptrtoconst char *matches the type ofsep, avoids discarding const fromstrchr(sep, '.'), and is consistent with howt_ptris only read (NULL-check andsscanf) and never mutated. No behavioral impact; this is a clean const-correctness fix.examples/l3fwd/l3fwd_fib.c (1)
241-242: LGTM! Burst size parameter updated correctly.The RX burst call now uses the configurable
rx_burst_sizeparameter, aligning with the per-direction burst sizing enhancement introduced in this PR.examples/l3fwd/l3fwd_acl.c (1)
1138-1139: LGTM! RX burst size properly configured.Correctly updated to use
rx_burst_sizefor configurable per-direction burst control.examples/l3fwd/l3fwd_lpm.c (1)
207-208: LGTM! Burst parameter correctly updated.The change to
rx_burst_sizeis consistent with the configurable burst sizing introduced across the L3FWD example.examples/l3fwd/l3fwd_em.c (1)
646-647: LGTM! Burst size configuration applied.Properly uses
rx_burst_sizefor RX burst operations, maintaining consistency with the broader refactoring.doc/guides/sample_app_ug/l3_forward.rst (1)
80-81: LGTM! Documentation properly updated.The new
--rx-burstand--tx-burstCLI options are well-documented with appropriate defaults (32 packets).Also applies to: 118-121
examples/l3fwd/l3fwd_common.h (2)
28-30: LGTM! External declarations added.Properly exposes
rx_burst_sizeandtx_burst_sizefor configurable burst control.
77-77: LGTM! Threshold now configurable.The immediate-send threshold now uses
tx_burst_size, making it configurable rather than hardcoded. This allows for better tuning of TX batching behavior.examples/l3fwd/l3fwd.h (2)
115-115: LGTM! Public API updated.The external declaration correctly reflects the new
rx_burst_sizeparameter.
151-153: LGTM! Buffer flush logic properly updated.The TX buffer flush threshold now uses
rx_burst_size, ensuring packets are sent when the buffer reaches the configured burst size.examples/l3fwd-power/main.c (3)
1504-1504: LGTM! Usage text updated.Documentation properly includes the new
--eth-link-speedoption.
1746-1746: LGTM! CLI option properly registered.The new link speed option is correctly defined and added to the long options array.
Also applies to: 1773-1773
1958-1970: LGTM! Link speed validation and configuration implemented correctly.The implementation properly:
- Parses the numeric speed value
- Rejects unsupported fixed speeds (10M, 100M)
- Validates the speed using
rte_eth_speed_bitflag- Applies the configuration to
port_conf.link_speedsexamples/l3fwd/main.c (3)
60-62: LGTM!The new
rx_burst_sizeandtx_burst_sizeglobal variables are properly initialized toDEFAULT_PKT_BURST, providing separate control for RX and TX burst sizes as intended.
949-955: LGTM!The RX and TX burst options are correctly wired to
parse_pkt_burstwith appropriate flags and target variables.
1690-1691: LGTM!Useful runtime logging of the active RX/TX burst sizes after argument parsing.
drivers/net/txgbe/txgbe_rxtx.c (1)
2338-2368: Initializetx_pkt_preparefor the simple TX pathAssigning
dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;in the no‑offload/simple path makes the TX prepare callback consistently defined in both simple and full-featured modes, avoiding reliance on a NULL pointer and aligning with other drivers. No further issues spotted here.drivers/net/ngbe/ngbe_rxtx.c (1)
1918-1945: Settx_pkt_preparein ngbe simple TX path as wellUsing
rte_eth_tx_pkt_prepare_dummyfor the simple (no‑offload) TX path ensuresdev->tx_pkt_prepareis always initialized and keeps behaviour consistent with the full-featured branch and with other PMDs. Looks correct.drivers/net/hns3/hns3_rxtx.c (2)
4552-4565: Using dummy tx_pkt_prepare when device is not started looks correctSetting
eth_dev->tx_pkt_prepare = rte_eth_tx_pkt_prepare_dummy;in the non-start / resetting path keeps the function pointer non-NULL and consistent with the dummy burst functions, reducing risk of accidental calls into the real prepare path while TX/RX are disabled. Looks good to me.
4918-4922: Stopping TX now consistently disables both burst and prepare pathsAssigning both
dev->tx_pkt_burstanddev->tx_pkt_prepareto their dummy implementations before updating fast-path ops makes the TX datapath fully inert while stopped, avoiding any mismatch between prep and xmit behavior. This change is sound.doc/guides/windows_gsg/build_dpdk.rst (4)
48-48: Improved phrasing clarity.Rewording "latest version installer" to "latest installer version" is slightly clearer and more grammatically natural.
133-146: Microsoft documentation link is valid and reStructuredText formatting is correct.Verification completed:
Microsoft documentation link: ✓ Valid and current. The URL points to the official VS 2022 Developer Command Prompt documentation on learn.microsoft.com and is still accessible.
reStructuredText syntax: ✓ Correct. The note block uses proper RST directives with correct indentation, and the inline reference link format
text <URL>_ follows RST standards.Removed VS 2022 caveat: Unable to confirm. No evidence found in git history or current documentation of a caveat about "VS 2022 not working" being present before removal. The claim about removal cannot be verified from available context.
The refactoring appropriately consolidates build documentation and the new note block directing users to official Microsoft resources is a helpful addition. The link and syntax are both accurate.
28-32: LLVM download links and version are valid but outdated.The LLVM 19.1.0 Windows installer link is correctly formatted and accessible via the canonical
releases.llvm.orgsource through the GitHub releases page. However, verification shows that LLVM 21.1.5 is the current stable release (as of November 2025), while 19.1.0 dates to 2024.Since the documentation specifies only "LLVM 14.0.0 (or later)" as the minimum requirement (line 13), LLVM 19.1.0 is functionally appropriate. However, given the "For example" preamble, consider either:
- Updating the example to reference a current stable version (21.1.5), or
- Adding a note that users should download the latest stable release from
releases.llvm.orgrather than pinning to 19.1.0The links themselves are valid and properly target the canonical LLVM download location.
69-69: URL is valid and currently accessible.The Meson MSI installer documentation page exists at the documented URL. The official Meson documentation confirms the
Getting-meson.htmlpage is current and includes the MSI installer section. The URL structuremesonbuild.com/Getting-meson.htmlis correct and points to valid, accessible documentation.examples/vdpa/main.c (2)
25-40: Compile-time-safe sizing ofifaceprefix looks goodDefining
MAX_VDPA_STR_LENand shrinkingifacetoMAX_PATH_LEN - MAX_VDPA_STR_LENgives a clear compile-time guarantee that buildingifnamewith"%s%d"cannot overflow theMAX_PATH_LENbuffer for any index withinMAX_VDPA_SAMPLE_PORTS. No issues spotted with this change.
78-80: Long-option handling and bounded copy are correctSwitching to
strcmp(long_option[idx].name, "iface")is simpler and more robust than relying on string prefixes, and usingrte_strscpy(iface, optarg, sizeof(iface))correctly honors the (now smaller)ifacebuffer size. Behavior of the"iface"option is preserved aside from the safer truncation bound.examples/meson.build (1)
81-83: This change is intentional and well-justified—the original review concern is unfounded.The removal of
-Wno-format-truncationis a deliberate improvement, not an oversight. Commit 03a8f97 ("examples: enable format truncation warning") explicitly states: "There were several hidden bugs in examples because format truncation warning was disabled." The change has been reviewed and approved by maintainers (Acked-by: Bruce Richardson).Enabling the format truncation warning exposes these previously hidden bugs rather than covering them up. This is the correct approach—examples should compile cleanly and demonstrate best practices, not hide warnings.
MAINTAINERS (1)
1905-1912: BPF maintainer path extension looks consistentAdding
F: app/test/bpf/under BPF maintains the existing structure and mirrors other app/test entries; no issues spotted.VERSION (1)
1-1: Version bump is straightforwardSingle-line update to
25.11.0-rc4; nothing else in this file to validate.drivers/event/cnxk/cn10k_eventdev.c (2)
122-151: Usingdev->rx_offloadsin flush path is consistent with fast‑path selectionPassing
dev->rx_offloadsintocn10k_sso_hws_get_work_emptymakes the flush logic respect the same RX offload configuration as the dequeue fast path. No functional issues spotted assumingrx_offloadsis kept current during adapter reconfiguration.
456-516: Quiesce path now also honorsdev->rx_offloadsBoth quiesce-time calls to
cn10k_sso_hws_get_work_emptynow usedev->rx_offloads, which keeps behavior aligned with the configured RX offloads. This looks correct and symmetric with the flush changes.drivers/event/cnxk/cn20k_eventdev.c (1)
188-220: Dynamicrx_offloadsmask in cn20k flush/quiesce is appropriateUsing
dev->rx_offloadsforcn20k_sso_hws_get_work_emptyin both flush and quiesce paths matches the fast‑path selection logic and keeps cn20k aligned with cn10k; no issues seen.Also applies to: 506-552
app/test-pmd/csumonly.c (1)
529-574: Inner vs outer L3 handling inparse_l4_protois now consistentThe new
in_tunnelflag andalign_ptypemapping correctly decouple where we read (inner vs. outer headers) from which L3 bits we test, soRTE_ETH_IS_{IPV4,IPV6}_HDRnow sees an inner‑L3‑aligned ptype when parsing the inner header. Outer parsing remains unchanged. This should fix incorrect L4 protocol detection for tunnelled packets without regressing non‑tunnel cases.Also applies to: 705-735
drivers/common/mlx5/mlx5_devx_cmds.h (2)
108-127: Flex HCA attribute flag addition looks consistentAdding
header_length_field_mode_wa:1tostruct mlx5_hca_flex_attrcleanly exposes the new firmware capability bit and fits the existing bitfield style. No issues from a header/API perspective.
646-653: Graph node offset‑mode bitfield is wired correctly
header_length_field_offset_mode:1instruct mlx5_devx_graph_node_attrprovides a dedicated flag for offset semantics; placement next toheader_length_field_offsetis appropriate and matches its intended use in the C implementation.drivers/net/mlx5/mlx5.c (2)
1078-1091: SRH flex parser offset switch onheader_length_field_mode_wais soundConditionally applying the legacy offset workaround when
attr->header_length_field_mode_wais set and using the direct offset (12) otherwise keeps the old arithmetic intact while enabling the newer firmware behavior. No correctness issues spotted.
1490-1497: Confirm intent behind new defaults fordv_flow_enandallow_duplicate_patternSetting
config->dv_flow_en = 1andconfig->allow_duplicate_pattern = 1as unconditional defaults changes behavior when devargs omit these keys (before capability adjustment). This seems aligned with always‑DV and duplicate‑pattern‑allowed operation but may surprise setups relying on previous defaults, especially when probing again on a shared context.Please confirm this default change is intentional and documented where needed.
drivers/net/mlx5/mlx5_flow_flex.c (1)
515-576: Flex header length OFFSET mode: WA gating and offset mode flag verified as correctly initializedThe
header_length_field_mode_waflag is reliably initialized from HCA capabilities during device probe inmlx5_devx_cmd_query_hca_attr()→mlx5_devx_cmd_query_hca_parse_graph_node_cap()(mlx5_devx_cmds.c:916–917). The flag is derived as the negation of the hardware capabilityheader_length_field_offset_mode, ensuring legacy FW (cap=0) sets WA=1 and new FW (cap=1) sets WA=0. Device initialization fails entirely if the HCA query fails, preventing any uninitialized access. The gating logic at line 557 and the inverse assignment at line 575 correctly preserve old behavior on legacy FW while enabling new offset semantics when supported. All validation checks remain unchanged and sound.drivers/common/mlx5/mlx5_prm.h (4)
1600-1602: Steering format constants as macros look fineReplacing the exposed enum with
#defineconstants for the supported steering formats is ABI‑friendly and keeps the values unchanged. No functional issues here.
2118-2120: Newheader_length_field_offset_modecapability bit is structurally correctThe added 1‑bit
header_length_field_offset_modebetweenmax_num_sampleandparse_graph_anchorkeeps the bitfield packing consistent and matches how it is consumed viaMLX5_GET(parse_graph_node_cap, ..., header_length_field_offset_mode)inmlx5_devx_cmd_query_hca_parse_graph_node_cap(). No structural problems from this addition.
2351-2357: Updated ESW flow‑table caps reserved regionExpanding
reserved_at_0to0x800bits beforeft_header_modify_esw_fdbkeeps subsequent fields aligned with the PRM layout and does not affect any in‑use fields. Looks good.
4990-4992: Flex‑parserheader_length_field_offset_modebit aligns with capability structAdding
header_length_field_offset_modehere mirrors the capability bit inmlx5_ifc_parse_graph_node_cap_bitsand is correctly placed betweenhead_anchor_idandnext_header_field_size, preserving 32‑bit alignment across the group. This matches its programming inmlx5_devx_cmd_create_flex_parser().doc/guides/nics/mlx5.rst (1)
650-651: Documentation updates appropriately clarify synchronous flow API limitations with HW steering.The changes systematically add clarity about constraints when using the synchronous flow API with hardware steering (dv_flow_en=2). Key improvements include:
- Default behavior clarification (lines 699-700): Simplified explanation of
dv_flow_endefault values- Prerequisite documentation (lines 1366-1372): Notes that
rte_flow_configure()must/should be called- Comprehensive limitations (lines 1410-1443): Covers match field sizes, rule update behavior, port configuration, and masked spec handling
- API-specific constraints (lines 2396-2402, 2593-2595, 2812-2813, 3816-3820, 2947-2948): Documents limitations across port representor, MARK item, aged flows, GRE fields, and meter actions
All changes use consistent terminology and are appropriately placed near relevant sections. Limitations provide actionable guidance with alternatives where applicable.
Also applies to: 699-700, 1366-1443, 2396-2402, 2593-2595, 2812-2813, 2947-2948, 3816-3820
drivers/net/nbl/nbl_hw/nbl_resource.h (1)
136-176: *Additional _stats_reset fields are consistent with new delta-based stats logicAdding
txq_stats_resetandrxq_stats_resetalongside the existing queue stats cleanly supports snapshot-based resets without affecting hardware-facing layout. Looks good.Also applies to: 178-213
drivers/net/nbl/nbl_hw/nbl_txrx.c (1)
724-804: *Delta-based stats using _stats_reset look correct but rely on clear invariantsUsing
current - resetfor each queue innbl_res_txrx_get_stats()and snapshotting withrxq->rxq_stats_reset = rxq->rxq_stats;/txq->txq_stats_reset = txq->txq_stats;innbl_res_txrx_reset_stats()gives “since last reset” semantics without disturbing live counters.A couple of points to keep in mind:
- This assumes
rxq_stats/txq_statsare only ever reset via queue reallocation (which zeroes both structs) and not manuallymemset()elsewhere.- For new queues created after a reset,
*_stats_resetstart at zero (fromrte_zmalloc[_socket]), which is consistent with “since allocation” until the next reset.Given those invariants, the implementation looks good.
If there are any out-of-tree helpers that still
memset()rxq_stats/txq_statsdirectly, they should be updated to use*_resetinstead or be audited to avoidcurrent < resetscenarios.drivers/net/nbl/nbl_dev/nbl_dev.h (1)
73-99: VLAN offload setter prototype matches implementation and usageThe added
nbl_vlan_offload_set()declaration matches the definition innbl_dev.cand the ethdev ops wiring, so the public interface is consistent.drivers/net/nbl/nbl_ethdev.c (1)
29-49: VLAN offload callback wiring looks correctAssigning
.vlan_offload_set = nbl_vlan_offload_setcleanly exposes the new API througheth_dev_ops; behavior is governed by the stub innbl_dev.c, which matches the comment about handling VLAN in software.drivers/net/nbl/nbl_dev/nbl_dev.c (1)
761-768: Perfect. I have now verified the complete design:
- nbl advertises VLAN_STRIP capability in
nbl_dev_infos_get()(line 422 of nbl_dev.c).- ethdev core validates and updates config before calling the driver callback (confirmed from web search).
- RX path performs SW VLAN stripping at lines 684–685 of nbl_hw/nbl_txrx.c, gated on
dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP.The design is correct. The no-op
nbl_vlan_offload_set()callback is the right pattern because ethdev itself manages the software configuration state, and since nbl uses software VLAN stripping (not hardware offload), the callback needs only return success without doing anything.
No-op
nbl_vlan_offload_set()is the correct design for software-based VLAN handlingThe stub implementation is appropriate: ethdev core validates capabilities, updates
dev->data->dev_conf.rxmode.offloads, and invokes the callback only for device-specific (hardware) programming. Since nbl performs VLAN stripping in software in the RX path (nbl_res_txrx_recv_pkts, lines 684–685 of nbl_hw/nbl_txrx.c) by checking the offloads flag and callingrte_vlan_strip(), no hardware changes are needed in the callback. Returning 0 is correct.drivers/net/txgbe/base/txgbe_aml.c (1)
20-34: AML ops wiring for mailbox host-interface looks correctHooking
mbx->host_interface_commandtotxgbe_host_interface_command_amlintxgbe_init_ops_aml()is consistent with the SP/AML40 paths and correctly centralizes FW interaction through the mailbox interface.drivers/net/txgbe/base/txgbe_aml40.c (1)
19-35: AML40 mailbox host-interface routing matches AML/SP patternsThe addition of
mbxand assignment totxgbe_host_interface_command_amlintxgbe_init_ops_aml40()correctly wires AML40 FW interaction through the shared mailbox callback, consistent with the other PF MAC types.drivers/net/txgbe/base/txgbe_type.h (1)
779-791: Mailbox interface extension viahost_interface_commandis reasonableAdding
host_interface_commandtostruct txgbe_mbx_infocleanly exposes the HIC entry point through the mailbox layer and matches the SP/AML/AML40 initialization wiring; no structural or ABI concerns within the driver.drivers/net/txgbe/base/txgbe_eeprom.c (1)
140-140: Using SWFLASH-only semaphore in EEPROM helpers matches new HIC layeringRestricting
masktoTXGBE_MNGSEM_SWFLASHin all hostif-backed EEPROM read/write helpers is coherent with the new design wherembx->host_interface_commandinternally manages the SWMBX semaphore; FLASH access remains serialized while HIC access is centralized through the mailbox callback.Also applies to: 167-167, 198-198, 223-223, 248-248, 275-275, 304-304, 329-329
drivers/net/txgbe/base/txgbe_mng.h (1)
211-214: SP/AML host-interface command prototypes are correctly declaredThe new declarations for
txgbe_host_interface_command_sp()andtxgbe_host_interface_command_aml()match their implementations and thembx->host_interface_commandcallback type, completing the public API surface for these HIC variants.drivers/net/txgbe/base/txgbe_mng.c (2)
82-101: SP/AML HIC entry points are cleanly separated and documentedRenaming the generic HIC helper to
txgbe_host_interface_command_sp()and keeping a dedicatedtxgbe_host_interface_command_aml()with AML-specific mailbox handling makes the SP vs AML split explicit while preserving existing behavior; the updated comment accurately reflects the SP-specific path.Also applies to: 163-172
306-307: <!-- [scratchpad_start]
[task review_verification FINAL]
[observations]
- txgbe_init_ops_dummy is called but no output showing its implementation
- The context shows it's called before the mac.type switch
- Then VF-specific init_ops_vf is called which overrides any dummy functions
[conclusion]
Even if txgbe_init_ops_dummy sets some function pointers, txgbe_init_ops_vf is called immediately after for VF, which would override them. The VF init_ops_vf function does NOT set:
- set_fw_drv_ver
- set_link_hostif
- host_interface_command in mbx
VERIFICATION COMPLETE - The review comment's concern is VALID but ADDRESSED:
The architecture properly isolates VF from HIC helpers because:
- VF initialization calls txgbe_init_ops_vf, not the PF-specific init paths
- txgbe_init_ops_vf does NOT initialize any HIC-related function pointers
- VF initialization does NOT call txgbe_init_hw (which calls txgbe_disable_lldp)
- VF has no EEPROM access through HIC helpers
- No VF code paths invoke txgbe_hic_* helpers directly or indirectly
The centralization of HIC commands through mbx->host_interface_command is SAFE.
</scratchpad_end] -->Now I have completed the verification. Based on my comprehensive analysis of the codebase, I can now provide the rewritten review comment:
Verification confirmed: VF code paths are properly isolated from HIC helpers and do not invoke them.
The architectural isolation is achieved through separate initialization paths:
VF initialization (eth_txgbevf_dev_init → txgbe_init_shared_code → txgbe_init_ops_vf):
- Calls
txgbe_init_ops_vf()which sets only VF-specific operations- Does NOT initialize
mac->set_fw_drv_ver,phy->set_link_hostif, ormbx->host_interface_command- Does NOT call
txgbe_init_hw()(which invokestxgbe_disable_lldpand HIC helpers)- Uses
hw->mac.reset_hw()andhw->mac.start_hw()directly insteadPF initialization (eth_txgbe_dev_init → calls hw->mac.init_hw):
- Calls
txgbe_init_hw()which uses HIC helpers via the centralizedmbx->host_interface_commandVF EEPROM access: Not wired through HIC helpers; VF lacks access to
txgbe_hic_sr_read/writepathsCentralizing all FW commands through
mbx->host_interface_commandis a solid abstraction that cleanly decouples call sites from underlying SP vs AML transport, with no risk to VF code paths due to their separate initialization.drivers/net/txgbe/base/txgbe_hw.c (1)
2999-3015: SP path mailbox host-interface hookup is consistent and safeThe code review is accurate. Verification confirms:
- The
mbx->host_interface_command = txgbe_host_interface_command_spassignment at line 3014 follows the identical pattern used in AML (txgbe_aml.c:33) and AML40 (txgbe_aml40.c:34) initialization functions, ensuring consistency across chip variants.- The
txgbe_init_ops_spfunction is called at line 2536 withintxgbe_init_shared_code(lines 2524–2556), guaranteeing that the mailbox callback is set before any firmware interaction code can execute. All HIC operations in txgbe_mng.c invoke via this callback, confirming proper routing to the SP-specific handler.- The initialization order is enforced by design: driver entry points call
txgbe_init_shared_codebefore any device-level operations.
| * py311-pyelftools | ||
|
|
||
| .. note: | ||
| .. note:: | ||
|
|
||
| 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):: | ||
|
|
||
| pkg install meson pkgconf py38-pyelftools | ||
| pkg install meson pkgconf py311-pyelftools | ||
|
|
There was a problem hiding this comment.
Fix doc typo and consider adding ninja to install example
- Line 24: “version at type of writing” → should be “time of writing”.
- The package list (lines 16–19) includes
ninja, but the samplepkg installcommand (line 28) omits it; consider changing to:
pkg install meson ninja pkgconf py311-pyelftools.
🤖 Prompt for AI Agents
In doc/guides/freebsd_gsg/build_dpdk.rst around lines 19 to 29, fix the typo
“type of writing” to “time of writing” and update the example install command to
include ninja; change the note text accordingly and modify the pkg install
sample to: pkg install meson ninja pkgconf py311-pyelftools so the package list
matches the earlier mention.
| #. 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) | ||
| that handle this by creating a tree of rules. | ||
|
|
||
| - When updating a rule by inserting a new one and deleting the old one, | ||
| for non-zero group, after adding the new rule, | ||
| and before the deletion of the old rule, the new rule will be matched, | ||
| contrary to the behavior in other flow engines (``dv_flow_en`` < 2) | ||
| in which the old rule will be matched. | ||
|
|
||
| - By default, the port is configured with zeroed ``rte_flow_port_attr``: | ||
| there are zero flow queues (one is created by default), | ||
| no actions, and no flags set. | ||
| The default flow queue size for ``rte_flow_queue_attr`` is 32 | ||
| (used for the internal flow queue). | ||
| If the application uses any configurable actions | ||
| (such as meter, age, conntrack or counter), | ||
| the system will allocate the maximum number of available actions per port. | ||
| To optimize memory usage, | ||
| the application should call ``rte_flow_configure`` | ||
| and specify only the required number of actions. | ||
| If the application needs to modify flow queue settings, | ||
| it should also use ``rte_flow_configure``. | ||
|
|
||
| - 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. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other mentions of synchronous flow API limitations that might duplicate these points
# Look for keywords related to the limitations documented
rg -n "synchronous flow API|sync.*flow|match field.*size|rule update|masked spec|port_attr|flow queue" doc/guides/nics/mlx5.rst | head -100Repository: ovsrobot/dpdk
Length of output: 1551
🏁 Script executed:
#!/bin/bash
# Extract the sections mentioned in synchronous flow API with HW steering
# Look at lines around 2396, 2593, 2812, 3819 to understand their content
echo "=== Section at line 2396 ==="
sed -n '2390,2410p' doc/guides/nics/mlx5.rst
echo -e "\n=== Section at line 2593 ==="
sed -n '2587,2607p' doc/guides/nics/mlx5.rst
echo -e "\n=== Section at line 2812 ==="
sed -n '2806,2826p' doc/guides/nics/mlx5.rst
echo -e "\n=== Section at line 3819 ==="
sed -n '2813,2833p' doc/guides/nics/mlx5.rst
echo -e "\n=== Section at line 3819 (corrected) ==="
sed -n '3813,3833p' doc/guides/nics/mlx5.rstRepository: ovsrobot/dpdk
Length of output: 3444
🏁 Script executed:
#!/bin/bash
# Check the context around line 1410 to see section headers and context
sed -n '1360,1415p' doc/guides/nics/mlx5.rst
echo -e "\n=== Check for section header (look further back) ==="
sed -n '1300,1365p' doc/guides/nics/mlx5.rst | grep -E "^[A-Za-z]|^~+|^-+|^#" | tail -20Repository: ovsrobot/dpdk
Length of output: 2911
Based on my verification, I can now provide the rewritten review comment:
Remove or clarify the inconsistency in header wording for consistency with related limitation items.
The comprehensive limitations block at lines 1410–1443 is well-structured and contains no duplicate information. However, there's a minor inconsistency: the header "When using synchronous flow API," (line 1410) should explicitly mention HW steering to match the pattern used in other sync API limitation items in the same section (lines 2396, 2593, 2812, 3819), which all say "When using synchronous flow API with :ref:HW steering <mlx5_hws>".
Verified:
- No duplication of limitations across sections
- General limitations (match fields, rule updates, port configuration, masked spec) are distinct from specific item/action limitations documented elsewhere
- All terminology is consistent ("synchronous flow API", "HW steering")
- No conflicting statements
Recommendation: Change line 1410 from "When using synchronous flow API," to "When using synchronous flow API with :ref:HW steering <mlx5_hws>," for consistency.
🤖 Prompt for AI Agents
In doc/guides/nics/mlx5.rst around lines 1410 to 1443, the header at line 1410
reads "When using synchronous flow API," but other synchronous API limitation
sections explicitly reference hardware steering; update the header to "When
using synchronous flow API with :ref:`HW steering <mlx5_hws>`," to match the
wording used in lines 2396, 2593, 2812, and 3819 for consistency across the
document.
| if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) | ||
| dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_TIMESTAMP; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the diff for this specific file to see the before/after
echo "=== Git diff for the file ==="
git diff HEAD -- drivers/net/intel/iavf/iavf_ethdev.c | head -100
echo -e "\n=== Checking file existence and content near lines 1180-1181 ==="
if [ -f "drivers/net/intel/iavf/iavf_ethdev.c" ]; then
sed -n '1175,1185p' drivers/net/intel/iavf/iavf_ethdev.c
else
echo "File not found at that path"
fiRepository: ovsrobot/dpdk
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Check git history to see previous version
echo "=== Recent commits touching this file ==="
git log --oneline -20 -- drivers/net/intel/iavf/iavf_ethdev.c
echo -e "\n=== Git show HEAD~1 for this specific file section (if available) ==="
git show HEAD~1:drivers/net/intel/iavf/iavf_ethdev.c 2>/dev/null | sed -n '1175,1185p' || echo "Previous version not available in current branch"
echo -e "\n=== Search for ptp_caps structure definition ==="
rg -n "struct.*ptp_caps|ptp_caps.*{" --type c -A5
echo -e "\n=== Search for ptp_caps RX capability checks ==="
rg -n "ptp_caps.*RX\|ptp_caps.*rx" --type c -C3Repository: ovsrobot/dpdk
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Search for ptp_caps structure
echo "=== Search for ptp_caps struct definition ==="
rg -n "ptp_caps" --type c -C5 drivers/net/intel/iavf/ | head -80
echo -e "\n=== Search for iavf_get_ptp_cap function ==="
rg -n "iavf_get_ptp_cap" --type c -A10
echo -e "\n=== Search for VF_CAP_PTP usage in context ==="
rg -n "VF_CAP_PTP" --type c -B2 -A2 drivers/net/intel/iavf/
echo -e "\n=== Check iavf.h for ptp_caps member ==="
rg -n "ptp_caps" --type c drivers/net/intel/iavf/iavf.h -C3Repository: ovsrobot/dpdk
Length of output: 9640
🏁 Script executed:
#!/bin/bash
# Search for any previous checks or git history details
echo "=== Check for VIRTCHNL_1588_PTP_CAP_RX_TSTAMP definition ==="
rg -n "VIRTCHNL_1588_PTP_CAP_RX_TSTAMP" --type c -B2 -A2
echo -e "\n=== Check if ptp_caps was previously used in capability advertisement ==="
git log -p --all -- drivers/net/intel/iavf/iavf_ethdev.c 2>/dev/null | grep -A5 -B5 "RX_OFFLOAD_TIMESTAMP\|ptp_caps" | head -50
echo -e "\n=== Summary of ptp_caps type ==="
echo "vf->ptp_caps is: uint32_t (from iavf.h line 288)"
echo "It's populated by iavf_get_ptp_cap() which stores the caps field from virtchnl_ptp_caps"Repository: ovsrobot/dpdk
Length of output: 1508
🏁 Script executed:
#!/bin/bash
# Check the actual behavior: is ptp_caps queried and available at the time of capability advertisement?
echo "=== Timeline check: when is ptp_caps populated? ==="
sed -n '1015,1030p' drivers/net/intel/iavf/iavf_ethdev.c
echo -e "\n=== When is dev_info_get called where capability is advertised? ==="
rg -n "dev_info_get\|get_dev_info" --type c drivers/net/intel/iavf/iavf_ethdev.c | head -5
echo -e "\n=== Check function containing lines 1180-1181 ==="
sed -n '1140,1200p' drivers/net/intel/iavf/iavf_ethdev.c | head -70Repository: ovsrobot/dpdk
Length of output: 2799
Based on my thorough verification, I can now provide the rewritten review comment:
Capability advertisement for RX timestamp offload is incomplete - must verify ptp_caps.
The condition at lines 1180-1181 only checks VIRTCHNL_VF_CAP_PTP but does not verify the specific RX timestamp capability in ptp_caps. This inconsistency with queue configuration at iavf_vchnl.c:1276-1279 can cause incorrect capability advertisement.
At queue configuration time, enabling RX timestamp requires:
vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP(capability flag)vf->ptp_caps & VIRTCHNL_1588_PTP_CAP_RX_TSTAMP(specific capability bit)
However, capability advertisement only checks the flag, not the capability bit. If hardware supports VF_CAP_PTP but not RX timestamps specifically, RTE_ETH_RX_OFFLOAD_TIMESTAMP will be incorrectly advertised, leading to runtime failures when applications attempt to use the feature.
Since ptp_caps is already populated during initialization (lines 1022-1027), the capability check should verify both:
if ((vf->vf_res->vf_cap_flags & VIRTCHNL_VF_CAP_PTP) &&
(vf->ptp_caps & VIRTCHNL_1588_PTP_CAP_RX_TSTAMP))
dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;This matches the pattern used elsewhere in the code (e.g., vlan_v2_caps checks) and ensures capability advertisement is consistent with actual hardware support.
🤖 Prompt for AI Agents
In drivers/net/intel/iavf/iavf_ethdev.c around lines 1180-1181, the code
advertises RX timestamp offload based only on VIRTCHNL_VF_CAP_PTP but does not
check the specific RX timestamp bit in vf->ptp_caps; update the condition to
require both the VF_CAP_PTP flag and the VIRTCHNL_1588_PTP_CAP_RX_TSTAMP bit in
vf->ptp_caps before setting dev_info->rx_offload_capa |=
RTE_ETH_RX_OFFLOAD_TIMESTAMP so advertisement matches actual hardware capability
and the queue-time check in iavf_vchnl.c.
| /* 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
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the file
find . -name "main.c" -path "*ip_reassembly*" | head -5Repository: ovsrobot/dpdk
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check file size
wc -l ./examples/ip_reassembly/main.c
# Read the section around lines 887-892
echo "=== Lines 880-900 (validation code) ==="
sed -n '880,900p' ./examples/ip_reassembly/main.c | cat -n
# Read the section around line 1069
echo "=== Lines 1060-1080 (upstream validation) ==="
sed -n '1060,1080p' ./examples/ip_reassembly/main.c | cat -n
# Search for the constant definitions
echo "=== Search for RTE_MAX_QUEUES_PER_PORT ==="
rg -n "RTE_MAX_QUEUES_PER_PORT" ./examples/ip_reassembly/main.c
echo "=== Search for MAX_RX_QUEUE_PER_LCORE ==="
rg -n "MAX_RX_QUEUE_PER_LCORE" ./examples/ip_reassembly/main.cRepository: ovsrobot/dpdk
Length of output: 2047
🏁 Script executed:
#!/bin/bash
# Search for RTE_MAX_QUEUES_PER_PORT in include files
echo "=== Searching for RTE_MAX_QUEUES_PER_PORT definition ==="
find . -name "*.h" -type f | xargs rg -l "RTE_MAX_QUEUES_PER_PORT" | head -10
# Also check the includes in main.c to understand the context
echo "=== Includes at top of main.c ==="
head -30 ./examples/ip_reassembly/main.c | grep -E "^#include"
# Check what constants are defined locally
echo "=== Local constants defined in main.c ==="
rg -n "^#define" ./examples/ip_reassembly/main.c | head -20Repository: ovsrobot/dpdk
Length of output: 1632
🏁 Script executed:
#!/bin/bash
# Search for RTE_MAX_QUEUES_PER_PORT definition in DPDK headers
echo "=== RTE_MAX_QUEUES_PER_PORT definition ==="
rg -n "RTE_MAX_QUEUES_PER_PORT" ./lib/ethdev/rte_ethdev.h -A 2 -B 2
# Also check what value it might be
echo "=== Search in config ==="
rg -n "RTE_MAX_QUEUES_PER_PORT" ./config/rte_config.h -A 2 -B 2
# Let's understand the queue parameter better - trace back where queue comes from
echo "=== Function signature for setup_queue_tbl ==="
sed -n '880,895p' ./examples/ip_reassembly/main.c | cat -n
sed -n '870,880p' ./examples/ip_reassembly/main.c | cat -nRepository: ovsrobot/dpdk
Length of output: 3257
Fix the comparison operators and add missing newline; the constant is appropriate but loose.
The validation has the following issues:
-
Incorrect comparison operators: The condition uses
>but should use>=for proper bounds checking. Valid lcore IDs range from 0 toRTE_MAX_LCORE-1, so checkinglcore > RTE_MAX_LCOREincorrectly allowslcore == RTE_MAX_LCOREto pass. Line 1070 correctly uses>=for the same validation, and DPDK's ethdev library consistently uses>=withRTE_MAX_QUEUES_PER_PORT(e.g., lines 6357, 6433, 6504). -
Missing newline: The error message at line 889 is missing a trailing newline character, causing poor log formatting.
-
Constant verification:
RTE_MAX_QUEUES_PER_PORT(1024) is a valid DPDK constant, but the actualqueueparameter is bounded byMAX_RX_QUEUE_PER_LCORE(16) via upstream validation at line 602. Using the larger constant is overly permissive; consider usingMAX_RX_QUEUE_PER_LCOREfor tighter bounds checking.
Apply this diff to fix the comparison operators and missing newline:
/* 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",
+ 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;
}📝 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; | |
| } | |
| /* 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; | |
| } |
🤖 Prompt for AI Agents
In examples/ip_reassembly/main.c around lines 887 to 892, the bounds check uses
the wrong comparison operators and the log message lacks a trailing newline;
change the comparisons to use >= for both lcore and queue (e.g., lcore >=
RTE_MAX_LCORE and queue >= RTE_MAX_QUEUES_PER_PORT) and append a '\n' to the
RTE_LOG message, and optionally tighten the queue bound by replacing
RTE_MAX_QUEUES_PER_PORT with the more appropriate MAX_RX_QUEUE_PER_LCORE to
enforce the actual upstream limit.
| 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.
Hardcoded port 0 may not exist or be representative.
When user provides --rx-burst 0, the code queries port 0 for PMD-recommended burst size. However:
- Port 0 may not exist or may not be enabled by the user's port mask
- Different ports may have different recommended burst sizes
- This runs during argument parsing before port validation
Consider deferring the PMD query until after port configuration, or document that this uses port 0's recommendation for all ports.
if (is_rx_burst) {
- /* If user gives a value of zero, query the PMD for its recommended
- * Rx burst size.
+ /* If user gives a value of zero, query the PMD for its recommended Rx burst size.
+ * Note: Uses port 0 as a reference; different ports may have different recommendations.
*/
ret = rte_eth_dev_info_get(0, &dev_info);
- if (ret != 0)
+ if (ret != 0) {
+ RTE_LOG(WARNING, L3FWD, "Cannot query port 0 for burst size, using default %d\n",
+ *burst_sz);
return;
+ }🤖 Prompt for AI Agents
In examples/l3fwd/main.c around lines 725-745, the code queries PMD info using
hardcoded port 0 when rx_burst_size == 0 which may not exist or represent
enabled ports; change this so the PMD query targets a valid enabled port (or run
after port configuration): either (A) defer querying dev_info until after port
setup and use each port's recommended value or a per-port lookup, or (B) if
keeping argument-time behavior, determine the first enabled port from the
configured port mask (validate it exists and is up) and call
rte_eth_dev_info_get on that port, check ret and handle errors, ensure
burst_size is clamped to [1, MAX_PKT_BURST], and update log messages to include
which port's recommendation was used so the behavior is explicit.
NOTE: This is an auto submission for "[RFC] test/latencystats: loosen requirements on measured latency".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36809" for details.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.