[PWCI] "net/intel: fixes and improvements to rx path selection" #179
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
The common function for detecting whether an rxq could use a vector rx path would automatically disqualify rx queues that had the timestamp offload enabled. This was incorrect behaviour because the iavf driver which uses this common function supports timestamp offload on its vector paths. Fix this by removing the conditional check for timestamp offload. Fixes: 9eb6058 ("net/intel: extract common Rx vector criteria") Cc: stable@dpdk.org Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Two rx paths had incorrect feature and offload definitions which led to incorrect path selections. Fix these. Remove timestamp offload from the list of offloads supported by paths that use the flexible rx descriptor. It is only available in the "offload" versions of those paths. Fixes: 91e3205 ("net/iavf: use common Rx path selection infrastructure") Cc: stable@dpdk.org Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
The scalar bulk alloc rx burst function supports both legacy and flexible rx descriptors. The rx path selection infrastructure introduced in commit 91e3205 ("net/iavf: use common Rx path selection infrastructure") cannot define a path that supports both descriptor formats. To solve this problem, have two rx path definitions which both point to the same rx burst function but report different descriptor formats. This allows the rx path selection function to choose the correct path. Fixes: 91e3205 ("net/iavf: use common Rx path selection infrastructure") Cc: stable@dpdk.org Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
In order to improve readability, reformat the rx path infos array. Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
In order to improve readability, reformat the rx path infos array. Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
In order to improve readability, reformat the rx path infos array. Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideThis PR refactors the RX path selection in Intel PMDs by converting positional initializers into explicit, designated initializers with nested feature definitions, adds a new bulk-alloc flex RXD path, and adjusts offload capability checks to allow timestamping on vector paths. Class diagram for updated ci_rx_path_info structure and RX path enumsclassDiagram
class ci_rx_path_info {
+pkt_burst
+info
+features
}
class ci_rx_path_features {
+rx_offloads
+simd_width
+extra
}
class ci_rx_path_extra {
+disabled
+scattered
+flex_desc
+bulk_alloc
}
ci_rx_path_info --> ci_rx_path_features : features
ci_rx_path_features --> ci_rx_path_extra : extra
class IAVF_RX_FUNC_TYPE {
IAVF_RX_DISABLED
IAVF_RX_DEFAULT
IAVF_RX_SCATTERED
IAVF_RX_FLEX_RXD
IAVF_RX_SCATTERED_FLEX_RXD
IAVF_RX_BULK_ALLOC
IAVF_RX_BULK_ALLOC_FLEX_RXD
IAVF_RX_SSE
IAVF_RX_SSE_SCATTERED
IAVF_RX_SSE_FLEX_RXD
...
}
IAVF_RX_FUNC_TYPE --> ci_rx_path_info : maps to
Class diagram for new RX path: IAVF_RX_BULK_ALLOC_FLEX_RXDclassDiagram
class ci_rx_path_info {
+pkt_burst = iavf_recv_pkts_bulk_alloc
+info = "Scalar Bulk Alloc Flex"
+features
}
class ci_rx_path_features {
+rx_offloads = IAVF_RX_SCALAR_FLEX_OFFLOADS
+simd_width = RTE_VECT_SIMD_DISABLED
+extra
}
class ci_rx_path_extra {
+flex_desc = true
+bulk_alloc = true
}
ci_rx_path_info --> ci_rx_path_features : features
ci_rx_path_features --> ci_rx_path_extra : extra
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughRefactors RX path configuration initializers across i40e, iavf, and ice to use named fields with a nested features structure. Adjusts vector capability gating to ignore TIMESTAMP offload and only block on BUFFER_SPLIT. Adds a new iavf RX function enum and removes TIMESTAMP from a flex vector offload set. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App
participant PMD as RX Queue Setup
participant Caps as ci_rxq_vec_capable
App->>PMD: Configure RX queue (offloads)
PMD->>Caps: Check vector capability
rect rgba(220,240,255,0.5)
note right of Caps: New check (changed)
Caps->>Caps: if BUFFER_SPLIT enabled -> reject
end
alt Capable
Caps-->>PMD: OK (vector path allowed)
PMD-->>App: Vector RX path selectable
else Not capable
Caps-->>PMD: Reject
PMD-->>App: Fallback to scalar path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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:
- Double‐check that adding IAVF_RX_BULK_ALLOC_FLEX_RXD doesn’t break the enum indexing or any loops over rx_path_infos—make sure all bounds or path counts are updated accordingly.
- The designated‐initializer blocks in iavf, i40e, and ice are nearly identical—consider abstracting them into a shared macro or helper to eliminate boilerplate and reduce future maintenance.
- After lifting the timestamp‐offload restriction in ci_rxq_vec_capable, verify that vector paths with timestamping behave correctly and don’t regress existing timestamp functionality.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double‐check that adding IAVF_RX_BULK_ALLOC_FLEX_RXD doesn’t break the enum indexing or any loops over rx_path_infos—make sure all bounds or path counts are updated accordingly.
- The designated‐initializer blocks in iavf, i40e, and ice are nearly identical—consider abstracting them into a shared macro or helper to eliminate boilerplate and reduce future maintenance.
- After lifting the timestamp‐offload restriction in ci_rxq_vec_capable, verify that vector paths with timestamping behave correctly and don’t regress existing timestamp functionality.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: 1
🧹 Nitpick comments (2)
drivers/net/intel/ice/ice_rxtx.c (1)
3670-3769: Designated initializer styleSame note as in i40e: consider using
.features = { .extra = { .scattered = true } }for broader C compiler portability; otherwise looks consistent.drivers/net/intel/iavf/iavf_rxtx.c (1)
3940-3947: ARM Neon entry’s offload mask: confirm intentNeon path uses Vector implementation (iavf_recv_pkts_vec) but sets rx_offloads = IAVF_RX_SCALAR_OFFLOADS. Verify if this is deliberate; otherwise align with vector mask (e.g., IAVF_RX_VECTOR_OFFLOADS).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
drivers/net/intel/common/rx.h(1 hunks)drivers/net/intel/i40e/i40e_rxtx.c(1 hunks)drivers/net/intel/iavf/iavf.h(1 hunks)drivers/net/intel/iavf/iavf_rxtx.c(1 hunks)drivers/net/intel/iavf/iavf_rxtx.h(0 hunks)drivers/net/intel/ice/ice_rxtx.c(1 hunks)
💤 Files with no reviewable changes (1)
- drivers/net/intel/iavf/iavf_rxtx.h
🧰 Additional context used
🧬 Code graph analysis (3)
drivers/net/intel/ice/ice_rxtx.c (3)
drivers/net/intel/ice/ice_rxtx_vec_sse.c (2)
ice_recv_pkts_vec(523-528)ice_recv_scattered_pkts_vec(575-596)drivers/net/intel/ice/ice_rxtx_vec_avx2.c (4)
ice_recv_pkts_vec_avx2(671-677)ice_recv_scattered_pkts_vec_avx2(754-763)ice_recv_pkts_vec_avx2_offload(679-685)ice_recv_scattered_pkts_vec_avx2_offload(765-774)drivers/net/intel/ice/ice_rxtx_vec_avx512.c (4)
ice_recv_pkts_vec_avx512(697-702)ice_recv_scattered_pkts_vec_avx512(805-821)ice_recv_pkts_vec_avx512_offload(708-714)ice_recv_scattered_pkts_vec_avx512_offload(829-847)
drivers/net/intel/i40e/i40e_rxtx.c (5)
drivers/net/intel/i40e/i40e_rxtx_vec_altivec.c (2)
i40e_recv_pkts_vec(374-379)i40e_recv_scattered_pkts_vec(425-446)drivers/net/intel/i40e/i40e_rxtx_vec_neon.c (2)
i40e_recv_pkts_vec(523-528)i40e_recv_scattered_pkts_vec(576-597)drivers/net/intel/i40e/i40e_rxtx_vec_sse.c (2)
i40e_recv_pkts_vec(530-535)i40e_recv_scattered_pkts_vec(583-604)drivers/net/intel/i40e/i40e_rxtx_vec_avx2.c (2)
i40e_recv_pkts_vec_avx2(611-616)i40e_recv_scattered_pkts_vec_avx2(665-680)drivers/net/intel/i40e/i40e_rxtx_vec_avx512.c (2)
i40e_recv_pkts_vec_avx512(678-683)i40e_recv_scattered_pkts_vec_avx512(733-750)
drivers/net/intel/iavf/iavf_rxtx.c (4)
drivers/net/intel/iavf/iavf_rxtx_vec_neon.c (1)
iavf_recv_pkts_vec(337-342)drivers/net/intel/iavf/iavf_rxtx_vec_sse.c (4)
iavf_recv_pkts_vec(1088-1093)iavf_recv_scattered_pkts_vec(1151-1172)iavf_recv_pkts_vec_flex_rxd(1100-1105)iavf_recv_scattered_pkts_vec_flex_rxd(1220-1242)drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c (8)
iavf_recv_pkts_vec_avx2(1416-1422)iavf_recv_scattered_pkts_vec_avx2(1517-1526)iavf_recv_pkts_vec_avx2_offload(1424-1430)iavf_recv_scattered_pkts_vec_avx2_offload(1528-1537)iavf_recv_pkts_vec_avx2_flex_rxd(1436-1442)iavf_recv_scattered_pkts_vec_avx2_flex_rxd(1609-1618)iavf_recv_pkts_vec_avx2_flex_rxd_offload(1444-1450)iavf_recv_scattered_pkts_vec_avx2_flex_rxd_offload(1620-1629)drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c (8)
iavf_recv_pkts_vec_avx512(1616-1622)iavf_recv_scattered_pkts_vec_avx512(1701-1707)iavf_recv_pkts_vec_avx512_offload(1792-1798)iavf_recv_scattered_pkts_vec_avx512_offload(1800-1807)iavf_recv_pkts_vec_avx512_flex_rxd(1628-1634)iavf_recv_scattered_pkts_vec_avx512_flex_rxd(1781-1790)iavf_recv_pkts_vec_avx512_flex_rxd_offload(1809-1819)iavf_recv_scattered_pkts_vec_avx512_flex_rxd_offload(1821-1830)
⏰ 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 (5)
drivers/net/intel/i40e/i40e_rxtx.c (1)
3293-3393: RX path table looks good; verify offload masks and consider portable designators
- Please confirm NEON/AltiVec vector entries intentionally use I40E_RX_SCALAR_OFFLOADS (not vector-specific masks).
- Minor: for maximum portability across toolchains, consider
.features = { .extra = { .scattered = true } }instead of.extra.scattered = true.drivers/net/intel/iavf/iavf.h (1)
330-331: RX path enum handling is complete:iavf_rx_path_infosand its RTE_DIM-bound lookups already includeIAVF_RX_BULK_ALLOC_FLEX_RXD; no further updates required.drivers/net/intel/common/rx.h (1)
238-240: Vector timestamp gating is safely enforced by per-driver masks
RX vector masks for ICE, I40E, IAVF, and IDPF all exclude RTE_ETH_RX_OFFLOAD_TIMESTAMP, so ci_rx_path_select prevents vector paths when timestamp offload is requested.drivers/net/intel/iavf/iavf_rxtx.c (2)
3723-3772: Structurized RX path initializers look goodClear, named-field init with features/flags improves readability and path selection. Consistent with TX path style.
3778-3778: Ignore incorrect macro rename suggestions
The macrosIAVF_RX_VECTOR_OFFLOAD_OFFLOADSandIAVF_RX_VECTOR_OFFLOAD_FLEX_OFFLOADSare defined iniavf_rxtx.hto include extended offload flags; no renaming is required.Likely an incorrect or invalid review comment.
Comment on lines
+3691
to
+3705
| [ICE_RX_SSE] = { | ||
| .pkt_burst = ice_recv_pkts_vec, | ||
| .info = "Vector SSE", | ||
| .features = { | ||
| .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS, | ||
| .simd_width = RTE_VECT_SIMD_128, | ||
| .extra.bulk_alloc = true}}, | ||
| [ICE_RX_SSE_SCATTERED] = { | ||
| .pkt_burst = ice_recv_scattered_pkts_vec, | ||
| .info = "Vector SSE Scattered", | ||
| .features = { | ||
| .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS, | ||
| .simd_width = RTE_VECT_SIMD_128, | ||
| .extra.scattered = true, | ||
| .extra.bulk_alloc = true}}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Potential macro typo: ICE_RX_VECTOR_OFFLOAD_OFFLOADS
The SSE entries use ICE_RX_VECTOR_OFFLOAD_OFFLOADS. If this macro is not defined, compilation will fail. For the non-“offload” SSE variants, this likely should be ICE_RX_VECTOR_OFFLOADS.
Run to confirm the macro exists and where it’s defined:
If it’s a typo, apply:
@@
- .features = {
- .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS,
+ .features = {
+ .rx_offloads = ICE_RX_VECTOR_OFFLOADS,
.simd_width = RTE_VECT_SIMD_128,
.extra.bulk_alloc = true}},
@@
- .features = {
- .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS,
+ .features = {
+ .rx_offloads = ICE_RX_VECTOR_OFFLOADS,
.simd_width = RTE_VECT_SIMD_128,
.extra.scattered = true,
.extra.bulk_alloc = true}},Also applies to: 3706-3721
🏁 Script executed:
#!/bin/bash
rg -n -C2 'ICE_RX_VECTOR_OFFLOAD_OFFLOADS|ICE_RX_VECTOR_OFFLOADS' -- drivers/net/intelLength of output: 4334
Use ICE_RX_VECTOR_OFFLOADS for SSE paths
- drivers/net/intel/ice/ice_rxtx.c:3695 (“Vector SSE”)
- drivers/net/intel/ice/ice_rxtx.c:3702 (“Vector SSE Scattered”)
- .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS,
+ .rx_offloads = ICE_RX_VECTOR_OFFLOADS,📝 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.
Suggested change
| [ICE_RX_SSE] = { | |
| .pkt_burst = ice_recv_pkts_vec, | |
| .info = "Vector SSE", | |
| .features = { | |
| .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS, | |
| .simd_width = RTE_VECT_SIMD_128, | |
| .extra.bulk_alloc = true}}, | |
| [ICE_RX_SSE_SCATTERED] = { | |
| .pkt_burst = ice_recv_scattered_pkts_vec, | |
| .info = "Vector SSE Scattered", | |
| .features = { | |
| .rx_offloads = ICE_RX_VECTOR_OFFLOAD_OFFLOADS, | |
| .simd_width = RTE_VECT_SIMD_128, | |
| .extra.scattered = true, | |
| .extra.bulk_alloc = true}}, | |
| [ICE_RX_SSE] = { | |
| .pkt_burst = ice_recv_pkts_vec, | |
| .info = "Vector SSE", | |
| .features = { | |
| .rx_offloads = ICE_RX_VECTOR_OFFLOADS, | |
| .simd_width = RTE_VECT_SIMD_128, | |
| .extra.bulk_alloc = true}}, | |
| [ICE_RX_SSE_SCATTERED] = { | |
| .pkt_burst = ice_recv_scattered_pkts_vec, | |
| .info = "Vector SSE Scattered", | |
| .features = { | |
| .rx_offloads = ICE_RX_VECTOR_OFFLOADS, | |
| .simd_width = RTE_VECT_SIMD_128, | |
| .extra.scattered = true, | |
| .extra.bulk_alloc = true}}, |
🤖 Prompt for AI Agents
In drivers/net/intel/ice/ice_rxtx.c around lines 3691 to 3705, the SSE feature
entries use the incorrect constant ICE_RX_VECTOR_OFFLOAD_OFFLOADS for
.features.rx_offloads; change both occurrences (the "Vector SSE" entry at ~3695
and the "Vector SSE Scattered" entry at ~3702) to ICE_RX_VECTOR_OFFLOADS so the
SSE paths reference the correct offload mask constant, leaving all other fields
unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36348"
Summary by Sourcery
Refactor Intel PMD RX path selection tables to use designated initializers, introduce a new IAVF bulk-alloc flex descriptor path, and simplify offload capability checks
New Features:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Refactor