[PWCI] "net/i40e: fix symmetric toeplitz hashing for SCTP" #183
+32
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After an upgrade to MinGW version 13, some compilation errors appear: drivers/bus/pci/windows/pci.c:362:58: error: 'GUID_DEVCLASS_NETUIO' undeclared drivers/bus/pci/windows/pci_netuio.c:57:39: error: 'GUID_DEVINTERFACE_NETUIO' undeclared The cause is MinGW has set NTDDI_VERSION to the highest version without defining the expected NetUIO constants. The case MinGW64 is added to define NetUIO constants. Some comments are improved to better track includes requirements. Fixes: 6605c7f ("bus/pci: fix build with Windows SDK >= 10.0.20253") Cc: stable@dpdk.org Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
After an upgrade to MinGW version 13, compilation breaks: drivers/net/mlx5/windows/mlx5_ethdev_os.c:285:69: error: 'dev_link.<U1000>.<Uaf00>.link_autoneg' may be used uninitialized This is because link_autoneg is never set in mlx5_link_update(). It can be set to the previous value (no change). Also it does not make sense to check this value to return the update status as it does not change. Fixes: 6fbd737 ("net/mlx5: support link update on Windows") Cc: stable@dpdk.org Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
After an upgrade to MinGW version 13, compilation breaks: In function 'rte_bbdev_queue_ops_dump': lib/bbdev/rte_bbdev.c:1269:63: error: '%s' directive argument is null [-Werror=format-overflow=] fprintf(f, " Enqueue Status Counters %s %" PRIu64 "\n", The enqueue status string may be null if the index is too high, because RTE_BBDEV_ENQ_STATUS_SIZE_MAX is defined to include padding for future enum insertion. This padding case must be checked to avoid printing a dump of a non-existing status. The type of the variable i is also changed to the enum required by the function rte_bbdev_enqueue_status_str(). Fixes: 353e363 ("bbdev: add queue debug dump") Cc: stable@dpdk.org Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: Bruce Richardson <bruce.richardson@intel.com> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
When symmetric toeplitz is configured for SCTP, packets belonging
to the same SCTP session are getting distributed to multiple queues
due to improper hash computation.
Removed SCTP Verification Tag from hash computation of symmetric toeplitz
as it changes for same SCTP session.
Tested the following with the fix & enabling symmetric toeplitz for SCTP:-
1. Packets belonging to the same SCTP session getting into the same queue.
2. Packets belonging to the different SCTP sessions getting distributed
to multiple queues.
Fixes: 0e4ae6c7e864 ("net/i40e: fix symmetric toeplitz hashing for SCTP")
Cc: stable@dpdk.org
Signed-off-by: Anurag Mandal <anurag.mandal@intel.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideThis PR implements proper symmetric Toeplitz RSS hashing for SCTP in the Intel i40e driver, restructures and corrects Windows PCI include and GUID definitions, fortifies BBDEV enqueue-status dumping, and fixes autoneg handling in the mlx5 Windows link-update path. Class diagram for updated i40e hash inset logicclassDiagram
class i40e_hash_get_inset {
+uint64_t i40e_hash_get_inset(uint64_t rss_types, bool symmetric_enable)
}
class rss_conf {
+bool symmetric_enable
+uint64_t inset
}
i40e_hash_get_inset <.. rss_conf : sets inset
note for i40e_hash_get_inset "Now excludes SCTP Verification Tag from hash when symmetric_enable is true."
Class diagram for BBDEV queue ops dump changesclassDiagram
class rte_bbdev_queue_ops_dump {
+void rte_bbdev_queue_ops_dump(uint16_t dev_id, uint16_t queue_id, FILE *f)
}
class rte_bbdev_queue_data {
+enum rte_bbdev_enqueue_status enqueue_status
+uint64_t enqueue_status_count[]
}
rte_bbdev_queue_ops_dump --> rte_bbdev_queue_data : accesses
note for rte_bbdev_queue_ops_dump "Now iterates using enum rte_bbdev_enqueue_status and skips NULL status strings."
Class diagram for mlx5 Windows link update fixclassDiagram
class mlx5_link_update {
+int mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete)
}
class rte_eth_dev {
+data
}
class dev_link {
+link_speed
+link_duplex
+link_autoneg
+link_status
}
rte_eth_dev o-- dev_link
mlx5_link_update --> rte_eth_dev : updates dev_link
note for mlx5_link_update "Now correctly updates link_autoneg after status comparison."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds a mailmap entry. Updates Windows PCI code to query NUMA node via DEVPKEY_Device_Numa_Node and adjusts conditional compilation for MinGW64 in a related header. Introduces symmetric hashing handling in i40e RSS inset computation. Tweaks MLX5 link update logic to ignore autoneg-only changes. Minor bbdev dump loop safety fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PnP as Windows PnP
participant PCI as pci.c
participant SetupAPI as SetupDi API
participant Dev as PCI Device Struct
PnP->>PCI: Enumerate PCI devices
PCI->>SetupAPI: SetupDiGetDevicePropertyW(DEVPKEY_Device_Numa_Node)
alt NUMA node found
SetupAPI-->>PCI: NUMA node (uint16)
PCI->>Dev: Store NUMA node
else Not found / legacy
SetupAPI-->>PCI: Not present
PCI->>Dev: Proceed with default (no NUMA)
end
sequenceDiagram
autonumber
participant Flow as RSS Config Parser
participant I40E as i40e_hash.c
participant HW as Hardware
Flow->>I40E: Parse RSS pattern/func
I40E->>I40E: Set symmetric_enable = (func == SYMMETRIC_TOEPLITZ)
I40E->>I40E: i40e_hash_get_inset(types, symmetric_enable)
alt symmetric_enable
I40E->>I40E: Clear SCTP_VERIFICATION_TAG bits (IPv4/IPv6)
end
I40E->>HW: Program RSS inset/types
sequenceDiagram
autonumber
participant NIC as MLX5 Driver
participant OS as OS Link State
NIC->>OS: Read current link
NIC->>NIC: Compare speed/duplex/up (ignore autoneg)
alt Changed
NIC->>NIC: Update dev_link (preserve prior autoneg assignment step)
else No change
NIC-->>OS: No update
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- This PR touches multiple drivers and subsystems—consider splitting it into focused, per-component patches to streamline review and maintainability.
- Ensure the new symmetric_enable flag is passed to all invocations of i40e_hash_get_inset (not only in pattern parsing) to avoid inconsistent inset calculations.
- In rte_bbdev_queue_ops_dump, the loop index uses an enum type; consider using size_t or an integer type matching the enqueue_status_count array for clarity and safety.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR touches multiple drivers and subsystems—consider splitting it into focused, per-component patches to streamline review and maintainability.
- Ensure the new symmetric_enable flag is passed to all invocations of i40e_hash_get_inset (not only in pattern parsing) to avoid inconsistent inset calculations.
- In rte_bbdev_queue_ops_dump, the loop index uses an enum type; consider using size_t or an integer type matching the enqueue_status_count array for clarity and safety.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/bus/pci/windows/pci.c (1)
276-294: Handle NUMA property type and “unknown” sentinel (-1) robustlySetupDiGetDevicePropertyW may return INT32 or UINT32; and “unknown” can be encoded as -1. Assigning 0xFFFFFFFF (DWORD) to a signed int is implementation-defined. Validate the type and normalize “unknown” to SOCKET_ID_ANY.
- DEVPROPTYPE property_type; - DWORD numa_node; + DEVPROPTYPE property_type; + ULONG numa_node; @@ - res = SetupDiGetDevicePropertyW(dev_info, dev_info_data, - &DEVPKEY_Device_Numa_Node, &property_type, - (BYTE *)&numa_node, sizeof(numa_node), NULL, 0); + res = SetupDiGetDevicePropertyW(dev_info, dev_info_data, + &DEVPKEY_Device_Numa_Node, &property_type, + (BYTE *)&numa_node, sizeof(numa_node), NULL, 0); if (!res) { DWORD error = GetLastError(); if (error == ERROR_NOT_FOUND) { /* On older CPUs, NUMA is not bound to PCIe locality. */ return ERROR_SUCCESS; } RTE_LOG_WIN32_ERR("SetupDiGetDevicePropertyW" "(DEVPKEY_Device_Numa_Node)"); return -1; } - dev->device.numa_node = numa_node; + /* Accept INT32 or UINT32. Normalize unknown (-1 or 0xFFFFFFFF) to SOCKET_ID_ANY. */ + if (property_type == DEVPROP_TYPE_INT32) { + LONG nn = (LONG)numa_node; + dev->device.numa_node = (nn < 0) ? SOCKET_ID_ANY : (int)nn; + } else if (property_type == DEVPROP_TYPE_UINT32) { + dev->device.numa_node = (numa_node == 0xFFFFFFFFu) ? SOCKET_ID_ANY : (int)numa_node; + } else { + /* Unexpected type; leave default. */ + return ERROR_SUCCESS; + }
🧹 Nitpick comments (3)
drivers/net/mlx5/windows/mlx5_ethdev_os.c (1)
297-321: Avoid uninitialized fields; confirm autoneg semantics
- Zero-initialize dev_link to future-proof against struct growth.
- You intentionally ignore autoneg-only changes. Confirm this is desired, since dev_link.autoneg is now frozen to the old value.
Apply:
- struct rte_eth_link dev_link; + struct rte_eth_link dev_link = {0};Would you prefer updating dev_link.link_autoneg from hardware while still excluding it from the “changed” check?
drivers/bus/pci/windows/pci_netuio.h (1)
8-16: Broaden MinGW support and harden guardConsider also MINGW32 and guarding the NTDDI_VERSION comparison to avoid relying on undefined-macro-to-0 behavior.
-#if !defined(NTDDI_WIN10_FE) || NTDDI_VERSION < NTDDI_WIN10_FE || defined(__MINGW64__) +#if !defined(NTDDI_WIN10_FE) || (defined(NTDDI_VERSION) && NTDDI_VERSION < NTDDI_WIN10_FE) || defined(__MINGW64__) || defined(__MINGW32__)drivers/bus/pci/windows/pci.c (1)
15-21: Tighten MinGW guard and avoid accidental redefinition of DEVPKEY_Device_Numa_NodeMake the toolchain check explicit and only define the key if missing. This avoids relying on undefined-macro evaluation and prevents redefinition when newer SDKs provide it.
-/* DEVPKEY_Device_Numa_Node should be defined in devpkey.h */ -#include <devpkey.h> -#if defined RTE_TOOLCHAIN_GCC && (__MINGW64_VERSION_MAJOR < 8) -#include <devpropdef.h> -DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc, - 0xa8, 0xa2, 0x6a, 0x0b, 0x89, 0x4c, 0xbd, 0xa2, 3); -#endif +/* DEVPKEY_Device_Numa_Node should be defined in devpkey.h */ +#include <devpkey.h> +#if defined(RTE_TOOLCHAIN_GCC) && defined(__MINGW64_VERSION_MAJOR) && (__MINGW64_VERSION_MAJOR < 8) +#include <devpropdef.h> +#ifndef DEVPKEY_Device_Numa_Node +DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc, + 0xa8, 0xa2, 0x6a, 0x0b, 0x89, 0x4c, 0xbd, 0xa2, 3); +#endif +#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.mailmap(1 hunks)drivers/bus/pci/windows/pci.c(1 hunks)drivers/bus/pci/windows/pci_netuio.h(1 hunks)drivers/net/intel/i40e/i40e_hash.c(4 hunks)drivers/net/mlx5/windows/mlx5_ethdev_os.c(1 hunks)lib/bbdev/rte_bbdev.c(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
.mailmap (1)
139-139: Mailmap entry LGTMEntry is correctly placed alphabetically and formatted.
lib/bbdev/rte_bbdev.c (1)
1250-1275: Safer enqueue status dumpUsing enum for the loop and skipping NULL strings prevents OOB/invalid prints. Good change.
drivers/net/intel/i40e/i40e_hash.c (4)
564-566: Signature update LGTMAdding symmetric_enable to i40e_hash_get_inset cleanly threads symmetry into inset computation.
1155-1156: Passing symmetric flag into inset computation LGTMEnsures SCTP VT is excluded when symmetric hashing is requested.
1127-1137: Symmetric flag handling LGTM;symmetric_enablefield confirmed
611-620: Fix SCTP VT clearing when multiple RSS types are setEquality checks miss cases where IPv4/IPv6 SCTP bits are combined with other RSS types. Use non-zero tests.
Apply:
- mask = rss_types & RTE_ETH_RSS_NONFRAG_IPV4_SCTP; - if (mask == RTE_ETH_RSS_NONFRAG_IPV4_SCTP) + mask = rss_types & RTE_ETH_RSS_NONFRAG_IPV4_SCTP; + if (mask) inset &= ~I40E_INSET_SCTP_VT; - mask = rss_types & RTE_ETH_RSS_NONFRAG_IPV6_SCTP; - if (mask == RTE_ETH_RSS_NONFRAG_IPV6_SCTP) + mask = rss_types & RTE_ETH_RSS_NONFRAG_IPV6_SCTP; + if (mask) inset &= ~I40E_INSET_SCTP_VT;Likely an incorrect or invalid review comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This is an auto submission for "net/i40e: fix symmetric toeplitz hashing for SCTP".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36355" for details.
Summary by Sourcery
Fix symmetric Toeplitz RSS hashing for SCTP and improve Windows PCI/BBDEV code robustness across drivers.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores