[PWCI] "Fixes for iavf VLAN insertion offload" #291
Conversation
When reserving a specific memory amount, it was possible to pass the first allocations and fail on a later allocation where there was no check, resulting in a crash. It is fixed by stopping the test if allocation failed. Fixes: fd368e1 ("test/hash: test more corner cases") Fixes: 9c7d8ee ("test/hash: add RCU tests") Cc: stable@dpdk.org Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Reviewed-by: David Marchand <david.marchand@redhat.com>
When running on limited platforms like GitHub Actions, the functional unit test "hash_readwrite_func_autotest" will hit a timeout, especially when running with UBSan: 46/102 DPDK:fast-tests / hash_readwrite_func_autotest TIMEOUT 30.01s killed by signal 15 SIGTERM Similarly to what was done in the commit fd368e1 ("test/hash: test more corner cases"), some constants are decreased. In order to keep the performance test as it was, a multiplier is kept for performance test case only. Signed-off-by: Thomas Monjalon <thomas@monjalon.net> Acked-by: David Marchand <david.marchand@redhat.com>
QinQ insertion was enabled in the bulk transmit function but not the single packet transmit function. Implement it in the single packet function. Also, fix an issue that would arise in the event an mbuf had both the VLAN and QINQ offload flags set. In this case the L2TAG2 field would be written to twice and could cause the tag to be corrupted. Reorder the logic of populating the L2TAG2 field and ensure that the field is only written to once. Fixes: 3aa4efa ("net/iavf: support VLAN insertion in AVX512 Tx") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Commit fdc3796 ("net/iavf: support QinQ insertion offload in scalar Tx") broke single VLAN insertion offload in cases where the v2 offload capability and both inner and outer insertion were supported because it caused inner VLAN tags to be inserted instead of outer. When an iavf tx queue is being set up, if v2 offload capability is supported, the driver queries the insertion capabilities and takes note of where VLAN tags should be placed in the transmit and/or context descriptors for insertion offload. In the offending commit, when both inner and outer insertion was reported as supported, the flag "vlan_flag" was changed to hold the location for inner VLAN tags. However this caused inner VLAN tags to be inserted in the case of single VLAN offload which is incorrect behaviour for this use case. To fix this, revert the "vlan_flag" back to holding the location for outer VLAN tags and update the datapath code accordingly. Fixes: fdc3796 ("net/iavf: support QinQ insertion offload in scalar Tx") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThis PR introduces a performance-mode parameter to test infrastructure for scaling allocations and iterations, adds allocation error handling in performance tests, and refactors VLAN/QinQ tag insertion logic in the IAVF driver to use simplified, granular control paths for determining tag placement and descriptor construction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–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 |
![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/net/intel/iavf/iavf_rxtx_vec_avx512.c (1)
2197-2204: QinQ tags wiped in ctx offload path
low_ctx_qw{0,1}get their L2TAG2 payload set just above, butiavf_fill_ctx_desc_tunnelling_field()assigns*qw0 = …, so in the offload path we zero out those bits. With VLAN/QinQ plus checksum/TX offloads, the context descriptor is emitted without the required L2TAG2 value and outer/inner tags stop being inserted. Please invoke the tunnelling helper before we populate the VLAN/QinQ fields (or reapply the tag afterwards) so the tag survives.@@ - if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { + if (offload) { + iavf_fill_ctx_desc_tunnelling_field(&low_ctx_qw1, pkt[1]); + iavf_fill_ctx_desc_tunnelling_field(&low_ctx_qw0, pkt[0]); + } + + if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) { @@ - if (offload) { - iavf_txd_enable_offload(pkt[1], &hi_data_qw1, vlan_flag); - iavf_txd_enable_offload(pkt[0], &hi_data_qw0, vlan_flag); - iavf_fill_ctx_desc_tunnelling_field(&low_ctx_qw1, pkt[1]); - iavf_fill_ctx_desc_tunnelling_field(&low_ctx_qw0, pkt[0]); - } + if (offload) { + iavf_txd_enable_offload(pkt[1], &hi_data_qw1, vlan_flag); + iavf_txd_enable_offload(pkt[0], &hi_data_qw0, vlan_flag); + }
🧹 Nitpick comments (1)
app/test/test_hash_readwrite.c (1)
551-551: Consider storing the entry count to avoid hardcoded assumptions.The hardcoded
TOTAL_ENTRY * PERF_MULTIPLIERworks currently sincetest_hash_readwrite_perfalways callsinit_paramswithperf=true. However, this creates tight coupling between theinit_paramscall and this memset.Consider storing
hash_params.entriesin thetbl_rw_test_paramstructure during initialization, then using that stored value here. This would make the code more maintainable and less prone to bugs if the initialization logic changes.Example refactor:
Add to the structure definition (around line 44):
struct { uint32_t *keys; uint8_t *found; uint32_t num_insert; uint32_t rounded_tot_insert; struct rte_hash *h; uint32_t total_entries; // Add this field } tbl_rw_test_param;Store the value in
init_params(after line 149):tbl_rw_test_param.total_entries = hash_params.entries;Use it at line 551:
memset(tbl_rw_test_param.found, 0, tbl_rw_test_param.total_entries);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/test/test_hash_readwrite.c(8 hunks)app/test/test_hash_readwrite_lf_perf.c(1 hunks)drivers/net/intel/iavf/iavf_rxtx.c(3 hunks)drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c(3 hunks)drivers/net/intel/iavf/iavf_rxtx_vec_common.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/test/test_hash_readwrite.c (1)
app/test/test_hash_readwrite_lf_perf.c (1)
init_params(148-192)
🔇 Additional comments (8)
app/test/test_hash_readwrite_lf_perf.c (1)
1313-1316: LGTM! Proper allocation failure handling added.The NULL check gracefully handles allocation failure and properly jumps to the error cleanup path, preventing potential null pointer dereference in subsequent operations.
app/test/test_hash_readwrite.c (7)
21-25: LGTM! Clear separation between functional and performance test parameters.The macro definitions clearly distinguish functional test sizes from performance test sizes, and the comment helps clarify the scaling intent.
75-78: LGTM! Proper allocation failure handling.The NULL check prevents potential crashes and returns an appropriate error code.
141-141: LGTM! Function signature extended to support performance mode.The new
perfparameter enables conditional scaling of hash table size for performance tests.
149-149: LGTM! Clean conditional scaling for performance mode.The ternary expression appropriately scales the hash table entries based on the performance mode flag.
186-186: LGTM! Consistent use of computed entry count.Using
hash_params.entriesinstead of hardcodedTOTAL_ENTRYensures allocations and loops scale correctly with the performance mode setting.Also applies to: 192-192, 201-201
230-230: LGTM! Functional tests correctly use non-performance parameters.The functional test correctly passes
falsefor the perf parameter and uses the appropriate functional test insert counts.Also applies to: 234-234, 236-236
393-393: LGTM! Performance test correctly enables performance mode.The performance test appropriately passes
trueto enable scaled allocations.
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36536"
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Refactor