-
Notifications
You must be signed in to change notification settings - Fork 4
[PWCI] "Fixes for Fedora Rawhide" #502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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>
With latest glibc (Fedora Rawhide):
../drivers/bus/ifpga/ifpga_bus.c: In function 'ifpga_parse':
../drivers/bus/ifpga/ifpga_bus.c:479:12: warning: assignment discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
479 | c1 = strchr(name, '|');
| ^
Fixes: 05fa3d4 ("bus/ifpga: add Intel FPGA bus library")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
With latest glibc (Fedora Rawhide):
../drivers/bus/uacce/uacce.c: In function 'uacce_match':
../drivers/bus/uacce/uacce.c:346:21: warning: assignment discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
346 | map = strstr(dev->algs, id_table->dev_alg);
| ^
Fixes: 62b906c ("bus/uacce: introduce UACCE bus")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
With latest glibc (Fedora Rawhide):
../drivers/common/qat/qat_device.c: In function 'qat_dev_cmdline_get_val':
../drivers/common/qat/qat_device.c:183:43: warning: return discards 'const'
qualifier from pointer target type [-Wdiscarded-qualifiers]
183 | return key ? strchr(key, '=') + 1 : NULL;
| ^
../drivers/common/qat/qat_device.c: In function 'cmdline_validate':
../drivers/common/qat/qat_device.c:189:25: warning: initialization discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
189 | char *eq_sign = strchr(arg, '=');
| ^~~~~~
Fixes: 99ab280 ("common/qat: isolate parser arguments configuration")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
With latest glibc (Fedora Rawhide):
../drivers/crypto/caam_jr/caam_jr_uio.c: In function
'file_name_match_extract':
../drivers/crypto/caam_jr/caam_jr_uio.c:111:16: warning: assignment
discards 'const' qualifier from pointer target type
[-Wdiscarded-qualifiers]
111 | substr = strstr(filename, match);
| ^
Fixes: e7a45f3 ("crypto/caam_jr: add UIO specific operations")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
With latest glibc (Fedora Rawhide):
../drivers/net/enetfec/enet_uio.c: In function 'file_name_match_extract':
../drivers/net/enetfec/enet_uio.c:37:16: warning: assignment discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
37 | substr = strstr(filename, match);
| ^
Fixes: b84fdd3 ("net/enetfec: support UIO")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
With latest glibc (Fedora Rawhide):
../drivers/net/memif/rte_eth_memif.c: In function
'memif_check_socket_filename':
../drivers/net/memif/rte_eth_memif.c:1848:13: warning: assignment discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
1848 | tmp = strrchr(filename, '/');
| ^
Fixes: 09c7e63 ("net/memif: introduce memory interface PMD")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
With latest glibc (Fedora Rawhide):
../app/proc-info/main.c: In function 'collectd_resolve_cnt_type':
../app/proc-info/main.c:689:26: warning: initialization discards
'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
689 | char *type_end = strrchr(cnt_name, '_');
| ^~~~~~~
Fixes: 2deb6b5 ("app/procinfo: add collectd format and host id")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WalkthroughThis PR includes version bump to 25.11.0-rc4, maintainer scope expansion, const-correctness improvements across multiple drivers, MLX5/TXGBE hardware feature updates, delta-based statistics implementation in NBL driver, burst size configurability in l3fwd example, and extensive documentation updates for release notes and platform guides. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (3)
drivers/net/nbl/nbl_hw/nbl_txrx.c (1)
21-27: Critical bug: Wrong variable checked after rx_rings allocation.Line 23 checks
tx_ringsinstead ofrx_rings. Ifrx_ringsallocation fails, the function will incorrectly return success with a NULLrx_ringspointer, leading to NULL pointer dereference later.txrx_mgt->rx_rings = rte_calloc("nbl_rxrings", rx_num, sizeof(struct nbl_res_rx_ring *), 0); - if (!txrx_mgt->tx_rings) { + if (!txrx_mgt->rx_rings) { NBL_LOG(ERR, "Allocate the rx rings array failed"); rte_free(txrx_mgt->tx_rings); return -ENOMEM; }drivers/bus/ifpga/ifpga_bus.c (1)
466-507: I'll verify the review comment's concern about potential NULL pointer dereference whenc2is passed tosnprintf(). Let me start by examining the codebase.
<function_calls>
shell
Search for all calls to ifpga_parse to understand how it's invoked
rg -n "ifpga_parse" --type=c -A 2 -B 2
</function_calls>Now let me look at the full context of the
ifpga_parsefunction to understand the control flow:
<function_calls>
shell
Get the full file to see the complete function and any surrounding context
wc -l drivers/bus/ifpga/ifpga_bus.c
</function_calls>Let me read the full function from the file to carefully trace the logic:
<function_calls>
shell
Read lines 466-510 to see the full function with context
sed -n '466,510p' drivers/bus/ifpga/ifpga_bus.c | cat -n
</function_calls>Now let me check for any defensive initialization or checks around
c2:
<function_calls>
shell
Look at the full function more carefully, including any initialization before line 466
sed -n '460,520p' drivers/bus/ifpga/ifpga_bus.c | cat -n
</function_calls>Let me search for how
ifpga_parseis registered or used as a callback:
<function_calls>
shell
Search for where ifpga_parse is referenced (not just called)
rg -n "ifpga_parse" drivers/bus/ifpga/ -A 1 -B 1
</function_calls>Let me check if there are any tests or examples that show how the function is expected to be called:
<function_calls>
shell
Search for documentation or test files related to ifpga
find . -path ./build -prune -o -type f ( -name "ifpga" -o -name "test" ) -print | grep -i ifpga | head -20
</function_calls>Let me look at the entire file context around the function to understand the architecture better:
<function_calls>
shell
See what function this is and what comes before/after
sed -n '450,530p' drivers/bus/ifpga/ifpga_bus.c | cat -n
</function_calls>Let me check if there's a return statement or check that would prevent execution when
c2is NULL:
<function_calls>
shell
Examine the exact initialization and flow
python3 << 'EOF'
Parse the function carefully
func = """ifpga_parse(const char *name, void *addr)
{
int *out = addr;
struct rte_rawdev *rawdev = NULL;
char rawdev_name[RTE_RAWDEV_NAME_MAX_LEN];
const char *c1 = NULL;
const char *c2 = NULL;
int port = IFPGA_BUS_DEV_PORT_MAX;
char str_port[8];
int str_port_len = 0;
int ret;memset(str_port, 0, 8);
c1 = strchr(name, '|');
if (c1 != NULL) {
str_port_len = c1 - name;
c2 = c1 + 1;
}if (str_port_len < 8 &&
str_port_len > 0) {
memcpy(str_port, name, str_port_len);
ret = sscanf(str_port, "%d", &port);
if (ret == -1)
return 0;
}memset(rawdev_name, 0, sizeof(rawdev_name));
snprintf(rawdev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s", c2);
rawdev = rte_rawdev_pmd_get_named_dev(rawdev_name);if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
rawdev &&
(addr != NULL))
*out = port;if ((port < IFPGA_BUS_DEV_PORT_MAX) &&
rawdev)
return 0;
else
return 1;
}"""Trace the flow:
print("=== Flow Analysis ===")
print("1. c2 is initialized to NULL")
print("2. c1 = strchr(name, '|') - searches for '|' in name")
print("3. If c1 != NULL:")
print(" - str_port_len = c1 - name (distance to separator)")
print(" - c2 = c1 + 1 (pointer to char after '|')")
print("4. If c1 == NULL:")
print(" - c2 remains NULL")
print(" - str_port_len remains 0")
print()
print("5. Later check: if (str_port_len < 8 && str_port_len > 0)")
print(" - This requires str_port_len > 0, which only happens if c1 was found")
print(" - If this check fails, port stays IFPGA_BUS_DEV_PORT_MAX")
print()
print("6. snprintf(rawdev_name, ..., "IFPGA:%s", c2)")
print(" - PROBLEM: c2 can be NULL here!")
print(" - This executes regardless of whether '|' was found")
print()
print("=== Verdict ===")
print("c2 CAN be NULL when reaching snprintf()")
print("If name has no '|', c2 stays NULL → UB in snprintf with %s")
EOF
</function_calls>Let me check the git history to see if this is a pre-existing issue or introduced by the const change:
<function_calls>
shell
Check git log for recent changes to this function
git log --oneline -10 -- drivers/bus/ifpga/ifpga_bus.c
</function_calls><function_calls>
shell
Check the diff for the current PR/branch to see what changed
git show HEAD:drivers/bus/ifpga/ifpga_bus.c | sed -n '460,510p' | cat -n
</function_calls>Let me see what the PR changed specifically:
<function_calls>
shell
Show the diff for this specific function
git diff HEAD~1 HEAD -- drivers/bus/ifpga/ifpga_bus.c | head -100
</function_calls><function_calls>
shell
Try to get current branch info
git log --oneline -3
</function_calls><function_calls>
shell
Check if there are uncommitted changes
git status drivers/bus/ifpga/ifpga_bus.c
</function_calls><function_calls>
shell
Let's search for how this function is actually used by examining callers more carefully
rg -n ".parse\s*=" drivers/bus/ifpga/ -B 3 -A 3
</function_calls><function_calls>
shell
Look at the full file to understand the context better
cat -n drivers/bus/ifpga/ifpga_bus.c | sed -n '1,50p'
</function_calls><function_calls>
shell
Look for the structure containing the parse function pointer
rg -n "parse" drivers/bus/ifpga/ifpga_bus.c | head -20
</function_calls><function_calls>
shell
Look at the complete context of the function and see what struct it belongs to
sed -n '400,530p' drivers/bus/ifpga/ifpga_bus.c | cat -n
</function_calls>Let me verify if there's any documentation or contract about the expected input format:
<function_calls>
shell
Look for function documentation or comments
sed -n '450,475p' drivers/bus/ifpga/ifpga_bus.c | cat -n
</function_calls><function_calls>
shell
Search for any documentation or comments above the function
sed -n '420,475p' drivers/bus/ifpga/ifpga_bus.c | cat -n
</function_calls>Based on my verification, I can now provide the rewritten review comment:
c2remains uninitialized (NULL) if the inputnamelacks a'|'separator, causing undefined behavior when passed tosnprintf()with the%sformat specifier.The const change to
c1andc2is appropriate for read-only pointer usage. However, the function has a pre-existing NULL-safety issue:c2is only assigned whenstrchr(name, '|')succeeds; if the separator is absent,c2stays NULL yet is unconditionally passed tosnprintf(rawdev_name, ..., "IFPGA:%s", c2)at line 493. This is undefined behavior.Either validate that the
nameparameter always contains the'|'separator (with a check likeif (!c2) return 1;), or provide a safe default likec2 = "";orc2 = namewhen no separator exists.examples/l3fwd/l3fwd_fib.c (1)
184-185: I'll verify the concerns raised in this review comment about potential stack buffer overflow whenrx_burst_sizeexceedsMAX_PKT_BURST. Let me start by examining the file structure and key definitions.
<function_calls>
shell
#!/bin/bashFirst, find and examine l3fwd_fib.c
find . -name "l3fwd_fib.c" -type f 2>/dev/null | head -5
</function_calls>
shell
#!/bin/bashCheck the size of l3fwd_fib.c and read relevant sections
wc -l examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashRead lines around 184-185 and 241-243 to see the actual code
echo "=== Lines 180-195 (first location) ==="
sed -n '180,195p' examples/l3fwd/l3fwd_fib.c
echo ""
echo "=== Lines 237-250 (second location) ==="
sed -n '237,250p' examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashSearch for MAX_PKT_BURST and rx_burst_size definitions
echo "=== Searching for MAX_PKT_BURST ==="
rg -n "MAX_PKT_BURST" examples/l3fwd/
echo ""
echo "=== Searching for rx_burst_size ==="
rg -n "rx_burst_size" examples/l3fwd/
</function_calls><function_calls>
shell
#!/bin/bashLook at l3fwd_common.h for declarations
find . -name "l3fwd_common.h" -type f 2>/dev/null
</function_calls>
<function_calls>
shell
#!/bin/bashCheck l3fwd_common.h for rx_burst_size and MAX_PKT_BURST
if [ -f examples/l3fwd/l3fwd_common.h ]; then
echo "=== l3fwd_common.h content ==="
cat examples/l3fwd/l3fwd_common.h
elseTry to find it
find examples/l3fwd -name "*.h" -type f | xargs ls -la
fi
</function_calls>
<function_calls>
shell
#!/bin/bashList all header files in l3fwd directory
find examples/l3fwd -name "*.h" -type f
</function_calls>
<function_calls>
shell
#!/bin/bashRead the full context around line 184 in l3fwd_fib.c
echo "=== Full fib_main_loop function context ==="
sed -n '160,250p' examples/l3fwd/l3fwd_fib.c
</function_calls><function_calls>
shell
#!/bin/bashSearch for rte_eth_rx_burst calls in the file
echo "=== RTE_ETH_RX_BURST calls and context ==="
rg -n -B5 -A3 "rte_eth_rx_burst" examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashCheck for MAX_PKT_BURST definition and its value
echo "=== MAX_PKT_BURST definition ==="
rg -n "#define MAX_PKT_BURST" examples/l3fwd/
echo ""Check the beginning of l3fwd_fib.c for includes and defines
echo "=== First 50 lines of l3fwd_fib.c ==="
head -50 examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashSearch for rx_burst_size declaration and initialization
echo "=== rx_burst_size definition and usage ==="
rg -n -B2 -A5 "rx_burst_size\s*=" examples/l3fwd/
</function_calls>
<function_calls>
shell
#!/bin/bashLook for validation of rx_burst_size
echo "=== Search for validation/clamping of rx_burst_size ==="
rg -n -B3 -A3 "(rx_burst_size|burst.size)" examples/l3fwd/.c | grep -i -E "(min|max|clamp|>|<|<=|>=)"
</function_calls>
<function_calls>
shell
#!/bin/bashGet full content around lines 184-185 and also check for function definition
echo "=== Lines 175-200 ==="
sed -n '175,200p' examples/l3fwd/l3fwd_fib.c
echo ""
echo "=== Lines 235-250 ==="
sed -n '235,250p' examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashCheck all files for rx_burst_size declaration and where it comes from
echo "=== All files mentioning rx_burst_size ==="
rg -l "rx_burst_size" examples/l3fwd/
echo ""
echo "=== Full context of rx_burst_size in all files ==="
rg -n -B5 -A5 "rx_burst_size" examples/l3fwd/ 2>/dev/null | head -100
</function_calls>
<function_calls>
shell
#!/bin/bashLook at the entire l3fwd_common.h to understand the declarations
find examples/l3fwd -type f ( -name ".h" -o -name ".c" ) | xargs grep -l "rx_burst_size" | head -5
</function_calls>
<function_calls>
shell
#!/bin/bashExamine main l3fwd file
ls -la examples/l3fwd/*.c | head -10
</function_calls>
<function_calls>
shell
#!/bin/bashGet rx_burst_size from main l3fwd files
echo "=== Searching in all l3fwd C files ==="
for f in examples/l3fwd/*.c; do
if grep -q "rx_burst_size" "$f" 2>/dev/null; then
echo "File: $f"
echo "---"
grep -n -B2 -A5 "rx_burst_size" "$f" 2>/dev/null | head -50
echo ""
fi
done
</function_calls><function_calls>
shell
#!/bin/bashSimpler approach: just read the relevant files directly
echo "=== Content of l3fwd_fib.c lines 1-50 ==="
head -50 examples/l3fwd/l3fwd_fib.cecho ""
echo "=== Lines 170-260 of l3fwd_fib.c ==="
sed -n '170,260p' examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashCheck for #include statements and MAX_PKT_BURST
echo "=== Includes and early definitions in l3fwd_fib.c ==="
head -100 examples/l3fwd/l3fwd_fib.c | grep -E "#include|#define|MAX_PKT"
echo ""
echo "=== Looking for MAX_PKT_BURST anywhere in l3fwd ==="
grep -r "MAX_PKT_BURST" examples/l3fwd/
</function_calls>
<function_calls>
shell
#!/bin/bashLet's check git to understand what changed
echo "=== Git status and recent changes ==="
cd examples/l3fwd 2>/dev/null && git log --oneline -10 l3fwd_fib.c 2>/dev/null || echo "Not a git repo or file not tracked"
cd - > /dev/null 2>&1Let's read the entire l3fwd_fib.c since it's relatively short
wc -l examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashRead entire file since it's < 300 lines
cat -n examples/l3fwd/l3fwd_fib.c
</function_calls><function_calls>
shell
#!/bin/bashSearch for all files that might define MAX_PKT_BURST or rx_burst_size
echo "=== All l3fwd source files ==="
find examples/l3fwd -type f ( -name ".c" -o -name ".h" )echo ""
echo "=== Grep for MAX_PKT_BURST in entire l3fwd directory ==="
grep -r "MAX_PKT_BURST" examples/l3fwd 2>/dev/nullecho ""
echo "=== Grep for rx_burst_size in entire l3fwd directory ==="
grep -r "rx_burst_size" examples/l3fwd 2>/dev/null
</function_calls>
<function_calls>
shell
#!/bin/bashSince l3fwd_fib.c is short, let's just output it all
echo "=== COMPLETE l3fwd_fib.c file ==="
cat examples/l3fwd/l3fwd_fib.c
</function_calls>Let me now search for the related header files and other l3fwd components:
<function_calls>
shell
#!/bin/bashCheck if there's an l3fwd.c or l3fwd_main.c
echo "=== Looking for main l3fwd files ==="
find examples/l3fwd -name ".c" -o -name ".h" | sortecho ""
echo "=== Searching all l3fwd h files for declarations ==="
for f in examples/l3fwd/*.h; do
if [ -f "$f" ]; then
echo "=== File: $f ==="
cat "$f"
echo ""
fi
done
</function_calls>
<function_calls>
shell
#!/bin/bashCheck for any l3fwd.c or similar main file
echo "=== All C files in l3fwd ==="
find examples/l3fwd -maxdepth 1 -name "*.c" -exec ls -lh {} ;echo ""
Read the main l3fwd.c if it exists
if [ -f examples/l3fwd/l3fwd.c ]; then
echo "=== l3fwd.c exists, checking for rx_burst_size and MAX_PKT_BURST ==="
grep -n "rx_burst_size|MAX_PKT_BURST" examples/l3fwd/l3fwd.c
fi
</function_calls>Now let me check if these are defined in any DPDK headers or if they're specific to this example:
<function_calls>
shell
#!/bin/bashSearch the entire repository for these symbols
echo "=== Searching entire repo for MAX_PKT_BURST ==="
grep -r "MAX_PKT_BURST" . 2>/dev/null | grep -v ".git" | head -20echo ""
echo "=== Searching entire repo for rx_burst_size ==="
grep -r "rx_burst_size" . 2>/dev/null | grep -v ".git" | head -20
</function_calls><function_calls>
shell
#!/bin/bashMore targeted search - look in examples directory
echo "=== All l3fwd files ==="
find examples/l3fwd -type f | sortecho ""
echo "=== Check if l3fwd.c exists and search it ==="
if [ -f "examples/l3fwd/l3fwd.c" ]; then
echo "File exists"
wc -l examples/l3fwd/l3fwd.c
echo "Searching for MAX_PKT_BURST:"
grep -n "MAX_PKT_BURST" examples/l3fwd/l3fwd.c | head
else
echo "examples/l3fwd/l3fwd.c does not exist"
fi
</function_calls>
<function_calls>
shell
#!/bin/bashList directory structure
ls -la examples/l3fwd/
</function_calls>
<function_calls>
shell
#!/bin/bashGet actual file listing and content
echo "=== Files in examples/l3fwd/ ==="
find examples/l3fwd -type f -exec basename {} ; | sortecho ""
echo "=== Reading l3fwd_fib.c in full ==="
cat examples/l3fwd/l3fwd_fib.c 2>&1 | head -300
</function_calls>
<function_calls>
shell
#!/bin/bashGet exact line numbers 184-185 and 241-243 from l3fwd_fib.c
echo "=== Exact lines 184-185 ==="
sed -n '184,185p' examples/l3fwd/l3fwd_fib.cecho ""
echo "=== Exact lines 241-243 ==="
sed -n '241,243p' examples/l3fwd/l3fwd_fib.cecho ""
echo "=== Context: lines 175-195 ==="
sed -n '175,195p' examples/l3fwd/l3fwd_fib.cecho ""
echo "=== Context: lines 235-250 ==="
sed -n '235,250p' examples/l3fwd/l3fwd_fib.c
</function_calls>Based on the shell commands I've executed, let me now examine the key definitions:
<function_calls>
shell
#!/bin/bashSearch for where pkts_burst is used with rte_eth_rx_burst
echo "=== Searching for rte_eth_rx_burst usage in l3fwd_fib.c ==="
grep -n "rte_eth_rx_burst" examples/l3fwd/l3fwd_fib.cecho ""
echo "=== Get context around those lines ==="
grep -n -B10 -A5 "rte_eth_rx_burst" examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashCheck the first 100 lines to see includes and global declarations
echo "=== First 100 lines ==="
head -100 examples/l3fwd/l3fwd_fib.cecho ""
echo "=== Search for MAX_PKT_BURST and rx_burst_size in l3fwd_fib.c ==="
grep -n "MAX_PKT_BURST|rx_burst_size" examples/l3fwd/l3fwd_fib.c
</function_calls>
<function_calls>
shell
#!/bin/bashGet all header files and search for these definitions
echo "=== All files in examples/l3fwd/ ==="
ls -1 examples/l3fwd/echo ""
echo "=== Content of l3fwd_common.h ==="
if [ -f examples/l3fwd/l3fwd_common.h ]; then
cat examples/l3fwd/l3fwd_common.h
else
echo "File not found, listing all .h files:"
find examples/l3fwd -name "*.h" -exec echo "File: {}" ; -exec cat {} ;
fi
</function_calls>Let me use a more reliable approach to find and read these files:
<function_calls>
shell
#!/bin/bashUse Python to safely read and display files
python3 << 'PYEOF'
import os
import globFind all files in l3fwd
l3fwd_files = glob.glob('examples/l3fwd/**', recursive=True)
l3fwd_files = [f for f in l3fwd_files if os.path.isfile(f)]
print("Files in l3fwd:")
for f in sorted(l3fwd_files):
print(f" {f}")Now read key files
print("\n" + "="*60)
print("Reading l3fwd_fib.c")
print("="*60)if os.path.exists('examples/l3fwd/l3fwd_fib.c'):
with open('examples/l3fwd/l3fwd_fib.c', 'r') as f:
lines = f.readlines()
print(f"Total lines: {len(lines)}")# Print specific ranges print("\nLines 184-185:") for i in range(183, min(185, len(lines))): print(f"{i+1}: {lines[i]}", end='') print("\nLines 241-243:") for i in range(240, min(243, len(lines))): print(f"{i+1}: {lines[i]}", end='') print("\nLines 175-200 (context):") for i in range(174, min(200, len(lines))): print(f"{i+1}: {lines[i]}", end='') print("\nLines 235-250 (context):") for i in range(234, min(250, len(lines))): print(f"{i+1}: {lines[i]}", end='')PYEOF
</function_calls>
<function_calls>
shell
#!/bin/bashSearch for MAX_PKT_BURST and rx_burst_size across all l3fwd files
python3 << 'PYEOF'
import os
import glob
import reFind all C and H files
files = glob.glob('examples/l3fwd/**', recursive=True)
files = [f for f in files if os.path.isfile(f) and (f.endswith('.c') or f.endswith('.h'))]print("Searching for MAX_PKT_BURST and rx_burst_size:")
print("="*60)for filepath in sorted(files):
try:
with open(filepath, 'r') as f:
content = f.read()
lines = content.split('\n')found_max = False found_burst = False for i, line in enumerate(lines, 1): if 'MAX_PKT_BURST' in line: found_max = True print(f"\n{filepath}:{i}: {line}") if 'rx_burst_size' in line: found_burst = True print(f"\n{filepath}:{i}: {line}") except Exception as e: passPYEOF
</function_calls>Now let me get the full file content to make my assessment:
<function_calls>
shell
#!/bin/bashRead the complete l3fwd_fib.c file using Python
python3 << 'PYEOF'
with open('examples/l3fwd/l3fwd_fib.c', 'r') as f:
lines = f.readlines()
for i, line in enumerate(lines, 1):
print(f"{i:3d}: {line}", end='')
PYEOF
</function_calls>Perfect! Now let me search for MAX_PKT_BURST and rx_burst_size in related files:
<function_calls>
shell
#!/bin/bashCheck other l3fwd files for these symbols
python3 << 'PYEOF'
import os
import globfiles_to_check = glob.glob('examples/l3fwd/*.[ch]')
for filepath in sorted(files_to_check):
with open(filepath, 'r') as f:
content = f.read()if 'MAX_PKT_BURST' in content or 'rx_burst_size' in content: print(f"\n{'='*60}") print(f"File: {filepath}") print('='*60) lines = content.split('\n') for i, line in enumerate(lines, 1): if 'MAX_PKT_BURST' in line or 'rx_burst_size' in line: # Print context start = max(0, i - 3) end = min(len(lines), i + 3) for j in range(start, end): marker = ">>>" if j == i - 1 else " " print(f"{marker} {j+1:3d}: {lines[j]}") print()PYEOF
</function_calls>Now let me check if MAX_PKT_BURST might be coming from DPDK or other common headers:
<function_calls>
shell
#!/bin/bashLook at the includes in l3fwd_fib.c to understand where MAX_PKT_BURST comes from
head -20 examples/l3fwd/l3fwd_fib.c
echo ""
echo "=== Searching entire codebase for MAX_PKT_BURST definition (not just usage) ==="
grep -r "define MAX_PKT_BURST" . 2>/dev/null | grep -v ".git"
</function_calls>Let me also check for the actual rte_eth_rx_burst calls:
<function_calls>
shell
#!/bin/bashSearch for rte_eth_rx_burst in all l3fwd files
python3 << 'PYEOF'
import globfiles = glob.glob('examples/l3fwd/*.[ch]')
for filepath in sorted(files):
with open(filepath, 'r') as f:
lines = f.readlines()for i, line in enumerate(lines, 1): if 'rte_eth_rx_burst' in line: print(f"\n{filepath}:{i}") start = max(0, i - 5) end = min(len(lines), i + 5) for j in range(start, end): marker = ">>>" if j == i - 1 else " " print(f"{marker} {j+1:3d}: {lines[j]}", end='')PYEOF
</function_calls>Excellent! Now let me verify if there's any validation/clamping of rx_burst_size:
<function_calls>
shell
#!/bin/bashSearch for initialization and validation of rx_burst_size
python3 << 'PYEOF'
import globfiles = glob.glob('examples/l3fwd/*.[ch]')
print("Looking for rx_burst_size initialization and validation:")
print("="*70)for filepath in sorted(files):
with open(filepath, 'r') as f:
content = f.read()
lines = content.split('\n')for i, line in enumerate(lines, 1): if 'rx_burst_size' in line: # Show context start = max(0, i - 4) end = min(len(lines), i + 4) print(f"\n{filepath}:{i}") for j in range(start, end): marker = ">>>" if j == i - 1 else " " print(f"{marker} {j+1:3d}: {lines[j]}")PYEOF
</function_calls>Now let me check all the l3fwd source files to get a complete picture:
<function_calls>
shell
#!/bin/bashList ALL files in l3fwd with details
find examples/l3fwd -type f -exec wc -l {} + | sort -n
And cat them all for review
echo ""
echo "=== ALL l3fwd source files content ==="
find examples/l3fwd -type f ( -name ".c" -o -name ".h" ) | sort | while read f; do
echo ""
echo "================================"
echo "FILE: $f"
echo "================================"
cat "$f"
done
</function_calls>Perfect! Now I have comprehensive visibility. Let me analyze the findings:
Based on my comprehensive verification of the codebase, I can now provide the rewritten review comment:
Clamp
rx_burst_sizeto prevent potential stack buffer overflowIn
fib_main_loop(), the RX burst size is unconstrained:struct rte_mbuf *pkts_burst[MAX_PKT_BURST]; ... nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst, rx_burst_size);If
rx_burst_sizeis ever configured larger thanMAX_PKT_BURST, this writes past the end ofpkts_burst, causing a stack buffer overflow.To make this safe independent of global configuration, clamp the burst size:
- nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst, - rx_burst_size); + const uint16_t burst = + RTE_MIN((uint16_t)rx_burst_size, (uint16_t)MAX_PKT_BURST); + nb_rx = rte_eth_rx_burst(portid, queueid, pkts_burst, burst);(or using
RTE_DIM(pkts_burst)).Even if configuration parsing restricts
rx_burst_size, enforcing the bound here prevents subtle regressions.Also applies to: line 241
🧹 Nitpick comments (18)
drivers/net/txgbe/base/txgbe_eeprom.c (1)
140-140: Semaphore mask narrowed to SWFLASH only across all EEPROM helpersUsing
TXGBE_MNGSEM_SWFLASHalone in all EEPROM read/write entry points keeps the acquire/release pattern correct and consistent, and should just reduce semaphore contention assuming SWMBX is only required by mailbox-specific paths elsewhere. I don’t see any missed releases or new race windows introduced by this change.If SWMBX is still used for other management mailbox operations in this driver, you might optionally consider documenting (e.g., in a comment near these helpers or in the MAC ops setup) that EEPROM accesses are intentionally serialized only on SWFLASH now, to avoid future confusion.
Also applies to: 167-167, 198-198, 223-223, 248-248, 275-275, 304-304, 329-329
examples/ip_reassembly/main.c (1)
887-892: Guard condition and log formatting can be tightenedThe new guard is fine as defensive code, but two small tweaks would make it clearer and more consistent:
- Use
>=withRTE_MAX_LCORE/RTE_MAX_QUEUES_PER_PORTsince these macros are “max count” values, so the highest valid ID is typicallymax - 1.- Add a newline to the log message to match surrounding
RTE_LOGusage and avoid run‑on logs.Suggested change:
- /* 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); + /* 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; }doc/guides/nics/mlx5.rst (1)
2812-2813: Clarify "aged flows reported only once" semantics.The documented limitation states aged flows are reported only once with synchronous HW steering. Consider adding one sentence explaining whether repeated aging checks require flow recreation or if there is another mechanism to re-arm aging detection for the same flow.
Consider clarifying the limitation with example:
#. When using synchronous flow API with :ref:`HW steering <mlx5_hws>`, - aged flows are reported only once. + aged flows are reported only once. + Subsequent age checks for the same flow require flow rule recreation + or re-configuration with ``rte_flow_configure()``.This would prevent user confusion about the aging lifecycle.
examples/vdpa/main.c (1)
23-26: Compile‑time suffix length macro is sound and keeps limits self‑consistentUsing
MAX_VDPA_STR_LENassizeof(RTE_STR(MAX_VDPA_SAMPLE_PORTS))correctly tracks the maximum decimal width (plus NUL) of the port index and keeps the array math in one place, so later changes toMAX_VDPA_SAMPLE_PORTSwon’t silently break the sizing logic. If you want extra belt‑and‑suspenders safety, you could add a static assert thatMAX_PATH_LEN > MAX_VDPA_STR_LENto fail early on configuration mistakes, but the current math is already correct.drivers/net/cnxk/cn10k_rx.h (1)
1467-1537: Zero‑initialization of LMT and offset locals is safe; consider alternative to silence warningsInitializing
lmt_id,d_off,lbase, andladdrto zero (Lines 1467–1468) is safe and keeps behavior unchanged because:
- When
NIX_RX_OFFLOAD_SECURITY_Fis set, these variables are always overwritten in the guarded block before use.- When security offload is disabled, subsequent uses remain under
if (flags & NIX_RX_OFFLOAD_SECURITY_F)orif (flags & NIX_RX_OFFLOAD_SECURITY_F && loff)so the initialized values are never consulted.If you want to avoid the extra instructions and still satisfy strict compilers, an optional alternative would be to:
- Wrap the final
if (flags & NIX_RX_OFFLOAD_SECURITY_F && loff)and related LMT flush logic in a narrower scope or helper that’s only compiled when security offload is built in, so the compiler can see definitive initialization paths.drivers/net/nbl/nbl_dev/nbl_dev.h (1)
98-98: VLAN offload setter prototype is fine; attribute use in header is optionalThe declaration is consistent with the definition and its use as a device op; no functional issues. If you want to keep the header a bit cleaner, you could drop
__rte_unusedon the parameters here and keep it only on the definition, since callers conceptually use both arguments.-int nbl_vlan_offload_set(__rte_unused struct rte_eth_dev *dev, __rte_unused int mask); +int nbl_vlan_offload_set(struct rte_eth_dev *dev, int mask);drivers/net/nbl/nbl_dev/nbl_dev.c (1)
761-768: No-opnbl_vlan_offload_setis reasonable; consider mask validation / clearer contractGiven the comment, a no-op implementation that always returns 0 is fine if both RX and TX VLAN handling are entirely driven by
rxmode/txmode.offloadsin your fast paths, with no extra hardware programming needed here.Two follow-ups you might consider:
- Validate
maskagainst the VLAN offload modes you actually support and return-ENOTSUP(or at least log) when unsupported bits are requested, instead of silently accepting everything.- Extend the comment to mention TX behavior as well (if no HW config is needed there either), so it’s clear to future readers why this callback is intentionally a stub.
Please double-check against the DPDK ethdev API/version you are targeting that:
- A do-nothing
vlan_offload_setis acceptable when capabilities are advertised but no HW state needs updating, and- The mask bits passed here are already sanitized against your
*_offload_capaso additional validation isn’t required unless you want stricter behavior.drivers/net/txgbe/base/txgbe_type.h (1)
779-791: Mailbox struct extension for host-interface commands looks goodAdding
host_interface_commandtostruct txgbe_mbx_infowith the shared SP/AML signature cleanly abstracts firmware host-interface calls behind the mailbox interface. Consider adding a short comment above this member indicating it is the unified FW host-interface command hook for clarity.drivers/net/mlx5/mlx5_flow_flex.c (1)
507-577: OFFSET-mode WA logic is sound; only minor readability nitsThe new
header_length_field_mode_wa‑gated block in the OFFSET path (extra bounds check,offsetbump, and the dword‑wrap avoidance) is consistent with the earlierwmax/mask checks and keeps the arithmetic well‑defined. Settingheader_length_field_offset_mode = !attr->header_length_field_mode_wacleanly exposes the firmware variant to DevX.If you want to reduce cognitive load around the unsigned subtraction, you could introduce a small temporary (e.g.
uint32_t diff = attr->header_length_mask_width - msb;) before comparing tooffset, but functionally this looks correct.Please validate on at least one pre‑CX8 (WA‑required) and one CX8+ firmware that the programmed FLEX PARSE GRAPH behaves as expected for OFFSET mode (no off‑by‑one or wrap when the mask width is small).
examples/server_node_efd/shared/common.h (1)
58-67: Based on my verification, I can now provide the rewritten review comment. The original review significantly overstates the severity of the thread-safety concern given that the function is only called during initialization/startup phases, not in worker threads or hot paths.
Buffer size change is correct; thread-safety concern is overstated given initialization-only usage.
The change to use
RTE_RING_NAMESIZEis appropriate for ring naming. However, the original review's "critical thread-safety issue" is misleading:
Thread-safety not critical here: The function is called only during initialization (efd_server/init.c:188 and efd_node/node.c:331), not in worker thread contexts. Each call immediately passes the buffer to
rte_ring_create()orrte_ring_lookup()without storing it, mitigating any practical race condition risk.Buffer size is adequate: Template "MProc_Node_15_RX" (with MAX_NODES=16) expands to 16 chars + null terminator = 17 bytes, well within RTE_RING_NAMESIZE (typically 32 bytes).
Comment inaccuracy: Line 59-60 states "maximum 3 digits" but with MAX_NODES=16, the actual maximum is 2 digits (node IDs 0-15). Update comment to reflect this.
Using
__rte_thread_localis unnecessary given the initialization-only usage pattern and would add unnecessary overhead.drivers/bus/fslmc/fslmc_bus.c (1)
242-305: Const-qualification oft_ptris fine; optional cleanup ofsephandling.Changing
t_ptrtoconst char *is correct since it’s only read. While not introduced here, note that whensep_existsis true,sepstill points at the separator (':'or'=') and is used in the device‑name checks (e.g.strncmp("dpni", sep, 4)), which means forms like"fslmc:dpni.1"/"name=dpni.1"won’t match as the comment suggests. If those forms are expected, consider advancingseppast the separator before validation.drivers/net/memif/rte_eth_memif.c (1)
1837-1869: Const update is good; re-checkfaccessatdirfd usage.Making
tmpconst char *is appropriate since it’s only used to locate the last'/'and for index math; no behavior change.Separately (pre‑existing),
faccessatis called as:if (dir == NULL || (faccessat(-1, dir, F_OK | R_OK | W_OK, AT_EACCESS) < 0)) { ... }On POSIX, the first argument is normally either a valid dir fd or
AT_FDCWD, not-1. Using-1risksEBADFand treating valid directories as invalid. It may be worth switching toAT_FDCWDhere (or plainaccess()if appropriate) for correctness and portability.doc/guides/freebsd_gsg/build_dpdk.rst (1)
19-29: FreeBSD pyelftools / Python 3.11 updates and note formatting are consistent.Switching to
py311-pyelftools, updating the correspondingpkg installexample, and fixing the Sphinx directive to.. note::keeps the documentation accurate for Python 3.11 and valid for Sphinx builds.examples/l3fwd-power/main.c (1)
1504-1505: Clarify and harden--eth-link-speedparsing/validationThe new option is wired correctly into
lgoptsandport_conf.link_speeds, but the parsing behavior is a bit inconsistent with other numeric options:
speed_numis parsed withatoi()and only 10M/100M are rejected explicitly; other invalid or out‑of‑range values are silently ignored (no error, no log), which differs from the stricterparse_uint()handling used elsewhere.- Non‑numeric values (e.g.
--eth-link-speed=foo) will be treated as0and effectively ignored without feedback.- The help text does not indicate units, valid values, or that 10M/100M are unsupported.
Consider:
- Reusing
parse_uint()(with a sensible max, e.g. highest supported speed) orstrtoul()with error checks, and- Treating unsupported speeds similarly to 10M/100M (print error + usage and fail), and
- Expanding the help string to document accepted speeds and units.
This keeps CLI behavior predictable and aligned with other options.
Also applies to: 1746-1747, 1753-1754, 1773-1774, 1958-1970
examples/l3fwd/l3fwd_common.h (1)
28-30: Document/centralize bounds forrx_burst_size/tx_burst_sizeThe switch from a compile-time threshold to
tx_burst_sizeinsend_packetsx4()plus the new externs is reasonable, and the local TX buffer logic still safely clamps toMAX_PKT_BURST.However, now both RX and TX code paths depend on these global burst sizes being kept within sane limits (e.g., not larger than the various
MAX_*capacities used for arrays and SW buffers elsewhere in l3fwd). It would be good to:
- Clearly document the allowed range for
rx_burst_size/tx_burst_size, and- Clamp or validate them once in the CLI parsing (e.g.,
<= MAX_PKT_BURST), rather than assuming all callers know the implicit limits.This keeps the rest of the code (including this function) obviously safe and easier to reason about.
Also applies to: 77-78
examples/l3fwd/main.c (3)
404-405: CLI help for new burst/link-speed options is accurate; consider documenting “0” semanticsThe added usage lines for
--rx-burst,--tx-burst, and--eth-link-speedcorrectly match the option names and default values passed into thefprintfcall.Given
parse_pkt_burst()treats--rx-burst 0as “use PMD-recommended RX burst” and--tx-burst 0as “keep default and log”, you might optionally spell out the special meaning of0in the help text to avoid surprises for users.Also applies to: 428-429, 433-437, 468-469
703-750:parse_pkt_burstlogic is sound; only minor robustness concern around port 0The refactored
parse_pkt_burst()cleanly:
- Parses and clamps user input to
MAX_PKT_BURST.- Updates the pointed-to
burst_szfor positive values.- For RX and value
0, queriesdev_info.default_rxportconf.burst_sizeand uses it when valid and within bounds.- For TX and value
0, leaves*burst_szunchanged but logs that the default will be used.One minor robustness point: using a hard-coded
port_id = 0inrte_eth_dev_info_get(0, &dev_info)assumes port 0 exists and its recommendation is representative. If you want this to be more general, you could consider using the first available or first enabled port instead of always 0.
881-882: New--eth-link-speedand RX/TX burst argument handling integrates cleanly; consider stricter validation for speedsThe added cases for
CMD_LINK_OPT_ETH_LINK_SPEED_NUM,CMD_LINE_OPT_PKT_RX_BURST_NUM, andCMD_LINE_OPT_PKT_TX_BURST_NUMinparse_args()are wired correctly to the new options and helpers:
--eth-link-speedrejects 10/100 Mbps explicitly, then usesrte_eth_speed_bitflag(speed_num, 0)to setport_conf.link_speedswhen it returns a non-zero flag.--rx-burst/--tx-burstdelegate toparse_pkt_burst()with the appropriateis_rx_burstflag and target global.Behaviour-wise, invalid or unsupported
eth-link-speedvalues (i.e., those mapping to0fromrte_eth_speed_bitflag) are silently ignored. If you want stricter UX, you could additionally detect that case and print usage / return an error, instead of quietly falling back to autonegotiation.Also applies to: 930-940, 949-955
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
doc/guides/prog_guide/img/kni_traffic_flow.pngis excluded by!**/*.pngdoc/guides/prog_guide/img/pkt_flow_kni.pngis excluded by!**/*.png
📒 Files selected for processing (70)
MAINTAINERS(1 hunks)VERSION(1 hunks)app/proc-info/main.c(1 hunks)app/test-pmd/csumonly.c(1 hunks)app/test/test_debug.c(2 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/bus/ifpga/ifpga_bus.c(1 hunks)drivers/bus/uacce/uacce.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/common/qat/qat_device.c(3 hunks)drivers/common/qat/qat_device.h(1 hunks)drivers/compress/qat/qat_comp_pmd.c(1 hunks)drivers/crypto/caam_jr/caam_jr_uio.c(1 hunks)drivers/crypto/qat/qat_asym.c(1 hunks)drivers/crypto/qat/qat_sym.c(1 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/enetfec/enet_uio.c(1 hunks)drivers/net/hns3/hns3_rxtx.c(2 hunks)drivers/net/intel/iavf/iavf_ethdev.c(1 hunks)drivers/net/memif/rte_eth_memif.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/softnic/rte_eth_softnic.c
- drivers/net/ntnic/ntnic_ethdev.c
🧰 Additional context used
🧬 Code graph analysis (8)
drivers/common/qat/qat_device.h (1)
drivers/common/qat/qat_device.c (1)
qat_dev_cmdline_get_val(176-184)
drivers/net/txgbe/base/txgbe_aml40.c (1)
drivers/net/txgbe/base/txgbe_mng.c (1)
txgbe_host_interface_command_aml(163-272)
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/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)
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_hw.c (1)
drivers/net/txgbe/base/txgbe_mng.c (1)
txgbe_host_interface_command_sp(99-161)
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/nbl/nbl_dev/nbl_dev.c (2)
lib/ethdev/ethdev_driver.h (1)
rte_eth_dev(72-117)drivers/net/nbl/nbl_hw/nbl_txrx_ops.h (2)
int(19-29)int(31-89)
| static uint8_t | ||
| parse_l4_proto(const struct rte_mbuf *m, uint32_t off, uint32_t ptype, bool parse_inner) | ||
| parse_l4_proto(const struct rte_mbuf *m, uint32_t off, uint32_t ptype, bool in_tunnel) | ||
| { | ||
| uint32_t align_ptype = ptype; | ||
| int frag = 0, ret; | ||
|
|
||
| if (RTE_ETH_IS_IPV4_HDR(ptype)) { | ||
| if (in_tunnel) { | ||
| uint32_t mask_ptype = ptype & RTE_PTYPE_INNER_L3_MASK; | ||
| if (mask_ptype == RTE_PTYPE_INNER_L3_IPV4) | ||
| align_ptype = RTE_PTYPE_L3_IPV4; | ||
| else if (mask_ptype == RTE_PTYPE_INNER_L3_IPV4_EXT) | ||
| align_ptype = RTE_PTYPE_L3_IPV4_EXT; | ||
| else if (mask_ptype == RTE_PTYPE_INNER_L3_IPV6) | ||
| align_ptype = RTE_PTYPE_L3_IPV6; | ||
| else if (mask_ptype == RTE_PTYPE_INNER_L3_IPV6_EXT) | ||
| align_ptype = RTE_PTYPE_L3_IPV6_EXT; | ||
| else | ||
| align_ptype = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and read the file around the specified lines
echo "=== Reading lines 520-580 of app/test-pmd/csumonly.c ==="
sed -n '520,580p' app/test-pmd/csumonly.c | cat -nRepository: ovsrobot/dpdk
Length of output: 2286
🏁 Script executed:
#!/bin/bash
# Search for get_ethertype_by_ptype function
echo "=== Searching for get_ethertype_by_ptype function ==="
rg -n "get_ethertype_by_ptype" app/test-pmd/csumonly.c -A 40
echo ""
echo "=== Checking lines 563-571 (Also applies to section) ==="
sed -n '555,575p' app/test-pmd/csumonly.c | cat -nRepository: ovsrobot/dpdk
Length of output: 4462
🏁 Script executed:
#!/bin/bash
# Search for where RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN is defined or used
echo "=== Searching for EXT_UNKNOWN ptype usage in the file ==="
rg -n "EXT_UNKNOWN" app/test-pmd/csumonly.c
echo ""
echo "=== Checking if these are standard DPDK constants ==="
rg -n "RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN|RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN" . --type=c | head -20Repository: ovsrobot/dpdk
Length of output: 1993
Based on my verification, the review comment is correct and valid. I've confirmed:
-
The
*_EXT_UNKNOWNptypes are real DPDK constants defined inlib/mbuf/rte_mbuf_ptype.h(lines 571, 592) and actively used by multiple hardware drivers (axgbe, zxdh, txgbe). -
The inconsistency is confirmed:
get_ethertype_by_ptype()explicitly handlesRTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWNandRTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN(lines 480, 487), correctly mapping them to IPv4/IPv6 ethertypes. However,parse_l4_proto()has no cases for these ptypes whenin_tunnel=true, causingalign_ptypeto be set to 0, which skips both IPv4 and IPv6 checks and returns 0. -
The impact is real: This affects L4 protocol detection for tunneled packets with these ptypes, breaking checksum offload and TSO handling as the reviewer stated.
The review comment's analysis and suggested fix are sound. The comment should be kept with classification since this is a genuine bug affecting actual hardware-generated packet types.
Handle *_EXT_UNKNOWN inner ptypes in parse_l4_proto
The tunnel-aware refactor generally makes sense, but the in_tunnel mapping currently ignores some inner L3 ptypes that this file otherwise treats as valid IPv4/IPv6:
get_ethertype_by_ptype()has explicit cases forRTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWNandRTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN(lines 480, 487), mapping them to IPv4/IPv6 ethertypes.- In
parse_l4_proto(), whenin_tunnelis true,mask_ptype = ptype & RTE_PTYPE_INNER_L3_MASK;is only checked againstINNER_L3_IPV4,INNER_L3_IPV4_EXT,INNER_L3_IPV6, andINNER_L3_IPV6_EXT. For*_EXT_UNKNOWNvalues,align_ptypebecomes 0, so both IPv4 and IPv6 branches are skipped and the function returns 0.
Effect: Tunneled packets whose inner header is tagged as *_EXT_UNKNOWN (generated by hardware drivers like axgbe and zxdh) will no longer have their L4 protocol detected, so they won't go through the UDP/TCP/SCTP checksum and TSO logic even though they were previously recognized as IPv4/IPv6.
Consider extending the mapping, for example:
uint32_t mask_ptype = ptype & RTE_PTYPE_INNER_L3_MASK;
switch (mask_ptype) {
case RTE_PTYPE_INNER_L3_IPV4:
case RTE_PTYPE_INNER_L3_IPV4_EXT:
case RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN:
align_ptype = RTE_PTYPE_L3_IPV4_EXT; /* or L3_IPV4 if you prefer */
break;
case RTE_PTYPE_INNER_L3_IPV6:
case RTE_PTYPE_INNER_L3_IPV6_EXT:
case RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN:
align_ptype = RTE_PTYPE_L3_IPV6_EXT;
break;
default:
align_ptype = 0;
break;
}(or any equivalent mapping consistent with how you want to treat "EXT_UNKNOWN").
This keeps inner handling aligned with the rest of the file and avoids unexpectedly disabling checksum handling for a subset of tunneled flows.
Also applies to: 563-571
🤖 Prompt for AI Agents
In app/test-pmd/csumonly.c around lines 528-547 (and similarly at 563-571), the
in_tunnel branch masks inner ptype but does not handle
RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN and RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN,
causing align_ptype to become 0 and skipping IPv4/IPv6 L4 protocol detection;
update the in_tunnel mapping to include the *_EXT_UNKNOWN inner ptypes and map
them to the corresponding outer L3 type (e.g., map INNER_L3_IPV4_EXT_UNKNOWN to
RTE_PTYPE_L3_IPV4_EXT and INNER_L3_IPV6_EXT_UNKNOWN to RTE_PTYPE_L3_IPV6_EXT) so
checksum/TSO logic runs for tunneled packets with those hardware-generated
ptypes.
| 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. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix grammar in UNCORE section reference.
Line 104 has a grammar issue: "Please refer the tuning guides" should be "Please refer to the tuning guides".
Apply this diff to fix the grammar:
-Please refer the tuning guides to enable ``HSMP`` and DPDK power management guide.
+Please refer to the tuning guides to enable ``HSMP`` and refer to the DPDK power management guide.🤖 Prompt for AI Agents
In doc/guides/linux_gsg/amd_platform.rst around lines 99 to 105, fix the grammar
in the UNCORE section by changing the phrase "Please refer the tuning guides" to
"Please refer to the tuning guides" so the sentence reads correctly and
references the tuning guides.
| A good option to choose is the MSI installer for both meson and ninja together:: | ||
|
|
||
| http://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer%22 | ||
| http://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the file to see both links (line 29 and 69)
cat -n doc/guides/windows_gsg/build_dpdk.rst | sed -n '20,75p'Repository: ovsrobot/dpdk
Length of output: 2207
🌐 Web query:
Does mesonbuild.com support HTTPS protocol for its documentation pages?
💡 Result:
Yes — the Meson documentation is served over HTTPS (e.g. https://mesonbuild.com/Reference-manual.html). [1][2]
Sources:
[1] mesonbuild.com Reference manual.
[2] mesonbuild/meson commit noting use of https for mesonbuild.com.
Upgrade Meson documentation links from HTTP to HTTPS for consistency and security.
The review comment is valid. Line 29 correctly uses HTTPS for the LLVM link (https://releases.llvm.org), while lines 66 and 69 both use HTTP for the Meson documentation links. Web verification confirms that mesonbuild.com supports HTTPS, making this an appropriate upgrade for consistency with modern security practices and the HTTPS changes already made to LLVM documentation in this PR.
Both occurrences should be updated:
- Line 66:
http://mesonbuild.com/Getting-meson.html→https://mesonbuild.com/Getting-meson.html - Line 69:
http://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer→https://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer
🤖 Prompt for AI Agents
doc/guides/windows_gsg/build_dpdk.rst lines 66 and 69: replace the Meson links
that currently use http with https to match the rest of the docs and ensure
secure links; specifically change http://mesonbuild.com/Getting-meson.html →
https://mesonbuild.com/Getting-meson.html (line 66) and
http://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer
→
https://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer
(line 69).
| static bool | ||
| file_name_match_extract(const char filename[], const char match[], int *number) | ||
| { | ||
| char *substr = NULL; | ||
|
|
||
| substr = strstr(filename, match); | ||
| if (substr == NULL) | ||
| return false; | ||
|
|
||
| /* substring <match> was found in <filename> | ||
| * read number following <match> substring in <filename> | ||
| */ | ||
| if (sscanf(filename + strlen(match), "%d", number) <= 0) | ||
| return false; | ||
|
|
||
| return true; | ||
| return strstr(filename, match) != NULL && | ||
| sscanf(filename + strlen(match), "%d", number) > 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the strstr result when parsing the number to preserve semantics
The new implementation:
return strstr(filename, match) != NULL &&
sscanf(filename + strlen(match), "%d", number) > 0;changes behavior compared to the usual pattern of parsing from the position where match is actually found. If match appears not at the start of filename, this will parse from filename + strlen(match) instead of from just after the matched substring, which conflicts with the documented behavior (“contains a certain substring… extracts the number following the substring”) and with what the old code almost certainly did.
Current sysfs names ("uioX", "fsl-jrX") happen to have match at the beginning, so this likely works today, but it silently narrows the accepted patterns and may break if naming ever changes.
To keep the compact style while preserving semantics, consider:
static bool
file_name_match_extract(const char filename[], const char match[], int *number)
{
- /* substring <match> was found in <filename>
- * read number following <match> substring in <filename>
- */
- return strstr(filename, match) != NULL &&
- sscanf(filename + strlen(match), "%d", number) > 0;
+ const char *substr = strstr(filename, match);
+
+ /* substring <match> was found in <filename>
+ * read number following <match> substring in <filename>
+ */
+ return substr != NULL &&
+ sscanf(substr + strlen(match), "%d", number) > 0;
}This keeps the simplification but maintains the intended “number after the matched substring” behavior.
🤖 Prompt for AI Agents
In drivers/crypto/caam_jr/caam_jr_uio.c around lines 106 to 114, the current
code calls strstr(filename, match) only to test for existence but then uses
filename + strlen(match) when parsing the number, which parses from the wrong
position if match is not at the start; fix by storing the result of strstr in a
pointer, check that pointer is not NULL, and pass pointer + strlen(match) to
sscanf to parse the trailing number so the parsed number starts immediately
after the actual matched substring.
| extern uint32_t max_pkt_len; | ||
|
|
||
| extern uint32_t nb_pkt_per_burst; | ||
| extern uint32_t rx_burst_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send_single_packet is keyed off rx_burst_size, ignoring tx_burst_size
send_single_packet() is part of the TX path, but the flush threshold and burst size passed to send_burst() still use rx_burst_size. With the new independent --rx-burst / --tx-burst options, the TX-side knob has no effect on this path, which is inconsistent with the rest of the example and user expectations.
I recommend switching this to use tx_burst_size and making it visible here:
extern uint32_t max_pkt_len;
-extern uint32_t rx_burst_size;
+extern uint32_t rx_burst_size;
+extern uint32_t tx_burst_size;
extern uint32_t mb_mempool_cache_size;
@@
- /* enough pkts to be sent */
- if (unlikely(len == rx_burst_size)) {
- send_burst(qconf, rx_burst_size, port);
+ /* enough pkts to be sent */
+ if (unlikely(len == tx_burst_size)) {
+ send_burst(qconf, tx_burst_size, port);
len = 0;
}Also applies to: 151-152
🤖 Prompt for AI Agents
In examples/l3fwd/l3fwd.h around lines 115 (and also update usages at 151-152),
the TX path's send_single_packet is incorrectly keyed off rx_burst_size; declare
and use tx_burst_size instead so the TX flush threshold and burst size use the
independent TX knob. Replace the extern uint32_t rx_burst_size reference here
with extern uint32_t tx_burst_size (and update the send_single_packet calls at
lines ~151-152 to pass tx_burst_size/tx flush threshold) so TX-side behavior
honors the --tx-burst option.
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36807"
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.