Open
Conversation
The info_get callback doesn't need to check its args since already done by ethdev. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
The packet length in packet burst generator was uint8_t which limited usefulness for testing larger packet sizes. The number of packets segments per packet is currently limited by mbuf nb_segs which is 16 bits. The comment is incorrect. Change nb_pkt_per_burst to uint16_t since that is the limit for tx_burst. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
Add a test for null PMD including different packet sizes. This test was generated with Claude AI based off of existing test_pmd_ring.c with some cleanup afterwards. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideRefactors packet burst generator helpers for wider parameter types and clearer segment length handling, simplifies null PMD device info reporting, and adds a comprehensive unit test suite and meson wiring for the null PMD covering RX/TX behavior, stats, configuration, RSS/RETA, and multithreaded TX semantics. Sequence diagram for null PMD test using packet burst generatorsequenceDiagram
actor Tester
participant TestApp as test_pmd_null
participant EthDev as rte_eth_dev_null
participant BurstGen as packet_burst_generator
Tester->>TestApp: run test_null_pmd_tx_basic()
TestApp->>EthDev: rte_eth_dev_configure()
EthDev-->>TestApp: success
TestApp->>EthDev: rte_eth_dev_start()
EthDev-->>TestApp: success
TestApp->>BurstGen: generate_packet_burst(mbufs, nb_packets, pkt_len, segs_per_pkt)
BurstGen-->>TestApp: filled_mbufs
TestApp->>EthDev: tx_burst(queue_id, mbufs, nb_packets)
EthDev-->>TestApp: nb_tx
TestApp->>EthDev: rte_eth_stats_get(stats)
EthDev-->>TestApp: updated_stats
TestApp-->>Tester: assert stats and nb_tx, report result
Class diagram for null PMD internals, device info, packet burst generator, and testsclassDiagram
class rte_eth_dev_data {
+void* dev_private
}
class pmd_internals {
+rx_null_queues[]
+tx_null_queues[]
+uint32_t reta_size
}
class rte_eth_dev {
+rte_eth_dev_data* data
+int (*dev_configure)(rte_eth_dev* dev)
+int (*dev_start)(rte_eth_dev* dev)
+int (*dev_stop)(rte_eth_dev* dev)
+int (*dev_close)(rte_eth_dev* dev)
+uint16_t (*rx_burst)(rte_eth_dev* dev, uint16_t queue_id, void** rx_pkts, uint16_t nb_pkts)
+uint16_t (*tx_burst)(rte_eth_dev* dev, uint16_t queue_id, void** tx_pkts, uint16_t nb_pkts)
}
class rte_eth_dev_info {
+uint32_t max_mac_addrs
+uint32_t max_rx_pktlen
+uint16_t max_rx_queues
+uint16_t max_tx_queues
+uint32_t reta_size
+uint64_t tx_offload_capa
}
class null_pmd_driver {
+int eth_dev_info(rte_eth_dev* dev, rte_eth_dev_info* dev_info)
+int eth_rx_queue_setup(rte_eth_dev* dev, uint16_t queue_id)
+int eth_tx_queue_setup(rte_eth_dev* dev, uint16_t queue_id)
+int eth_rx_queue_start(rte_eth_dev* dev, uint16_t queue_id)
+int eth_tx_queue_start(rte_eth_dev* dev, uint16_t queue_id)
}
class packet_burst_generator {
+int init_packets(uint16_t nb_packets, uint32_t pkt_len)
+int generate_packet_burst(void** mbufs, uint16_t nb_packets, uint32_t pkt_len, uint16_t segs_per_pkt)
+int verify_packet_burst(void** mbufs, uint16_t nb_packets, uint32_t expected_pkt_len, uint16_t expected_segs)
}
class test_pmd_null {
+int test_null_pmd_rx_basic()
+int test_null_pmd_tx_basic()
+int test_null_pmd_stats()
+int test_null_pmd_config()
+int test_null_pmd_rss_reta()
+int test_null_pmd_multithread_tx()
}
rte_eth_dev_data --> pmd_internals : dev_private
rte_eth_dev --> rte_eth_dev_data : data
null_pmd_driver --> pmd_internals : uses
null_pmd_driver --> rte_eth_dev : implements_callbacks_for
null_pmd_driver --> rte_eth_dev_info : fills
packet_burst_generator --> test_pmd_null : used_by
test_pmd_null --> null_pmd_driver : calls_via_ethdev_api
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis pull request extends the DPDK null PMD with a comprehensive new test suite, updates function signatures in the packet burst generator to use wider integer types (uint16_t), adds dependencies for the test in the build configuration, and refactors the null PMD device info function for clarity. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 - I've found 2 issues, and left some high level feedback:
- In the packet_burst_generator helpers, consider validating
nb_pkt_segs(e.g., non-zero and not exceeding pkt_len) before using it to computepkt_seg_data_lento avoid potential division-by-zero or nonsensical segment sizes if called with bad parameters. - In
test_mbuf_setup_burst, updatingm->buf_lento the chosen packet length is unusual sincebuf_lendescribes the buffer capacity; it would be safer to leavebuf_lenuntouched and setdata_len/pkt_lenonly so that mbufs remain consistent with their pool configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the packet_burst_generator helpers, consider validating `nb_pkt_segs` (e.g., non-zero and not exceeding pkt_len) before using it to compute `pkt_seg_data_len` to avoid potential division-by-zero or nonsensical segment sizes if called with bad parameters.
- In `test_mbuf_setup_burst`, updating `m->buf_len` to the chosen packet length is unusual since `buf_len` describes the buffer capacity; it would be safer to leave `buf_len` untouched and set `data_len`/`pkt_len` only so that mbufs remain consistent with their pool configuration.
## Individual Comments
### Comment 1
<location> `app/test/test_pmd_null.c:220-223` </location>
<code_context>
+ TEST_ASSERT(nb_tx == BURST_SIZE,
+ "Expected to TX %u packets, but sent %u", BURST_SIZE, nb_tx);
+
+ pool_count_after = rte_mempool_avail_count(mp);
+
+ /* Verify mbufs were freed - pool should have same count */
+ TEST_ASSERT(pool_count_after >= pool_count_before,
+ "Mbufs not freed: before=%u, after=%u",
+ pool_count_before, pool_count_after);
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the TX basic test by asserting exact mempool count rather than `>=`.
Since this test allocates a fixed burst and relies on the null PMD to free exactly those mbufs, the assertion should be stricter. Instead of `pool_count_after >= pool_count_before`, either:
- Assert equality (`pool_count_after == pool_count_before`), or
- Track how many mbufs this test allocates and assert the mempool delta matches that number.
Using `>=` can mask leaks or partial frees and weakens the test’s ability to validate that TX frees all mbufs.
```suggestion
/* Verify mbufs were freed - pool should have same count */
TEST_ASSERT(pool_count_after == pool_count_before,
"Mbuf leak or unexpected mempool delta: before=%u, after=%u",
pool_count_before, pool_count_after);
```
</issue_to_address>
### Comment 2
<location> `app/test/test_pmd_null.c:615-620` </location>
<code_context>
+test_null_reta_config(void)
+{
+ struct rte_eth_dev_info dev_info;
+ struct rte_eth_rss_reta_entry64 reta_conf[RTE_ETH_RSS_RETA_SIZE_128 /
+ RTE_ETH_RETA_GROUP_SIZE];
+ struct rte_eth_conf port_conf = {0};
+ uint16_t reta_port;
+ const uint16_t num_queues = 2;
+ unsigned int i, j, nreta;
+ uint16_t q;
+ int ret;
</code_context>
<issue_to_address>
**issue (bug_risk):** Make RETA tests robust to `dev_info.reta_size` values larger than 128 to avoid potential overflow.
`reta_conf` is sized assuming a maximum RETA size of 128, but `nreta` is computed from `dev_info.reta_size`. If `dev_info.reta_size > 128`, `nreta` will exceed `reta_conf`'s capacity, causing out-of-bounds access during init/update/query. Please either enforce `dev_info.reta_size <= RTE_ETH_RSS_RETA_SIZE_128` (e.g., with an assert) or size `reta_conf` and the associated loop bounds from a value derived from `dev_info.reta_size` (possibly clamped via `RTE_MIN`).
</issue_to_address>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
🤖 Fix all issues with AI agents
In @app/test/test_pmd_null.c:
- Around line 176-195: In test_mbuf_setup_burst, do not modify the mbuf's
buf_len (it is mempool-owned); remove the assignment to m->buf_len and instead
set m->data_len to the chosen length and also set m->pkt_len = m->data_len for
single-segment packets; ensure this change is made after successful
rte_pktmbuf_alloc_bulk in the function test_mbuf_setup_burst and keep the length
computation using rte_pktmbuf_tailroom(m) - RTE_ETHER_MIN_LEN as before.
🧹 Nitpick comments (1)
app/test/packet_burst_generator.c (1)
245-248: Inconsistent type handling compared togenerate_packet_burst_proto.In
generate_packet_burst, the comparison uses an explicit cast(int)(nb_pkt_segs - 1), while ingenerate_packet_burst_proto(line 334), the equivalent comparison isi != nb_pkt_segs - 1without the cast. While both work correctly due to integer promotion rules, the inconsistency is confusing.Consider aligning both functions to use the same style. Removing the cast here would be simpler since the comparison works correctly without it.
♻️ Suggested simplification
- if (i != (int)(nb_pkt_segs - 1)) + if (i != nb_pkt_segs - 1)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/test/meson.buildapp/test/packet_burst_generator.capp/test/packet_burst_generator.happ/test/test_pmd_null.cdrivers/net/null/rte_eth_null.c
🧰 Additional context used
🧬 Code graph analysis (2)
app/test/packet_burst_generator.h (1)
app/test/packet_burst_generator.c (1)
generate_packet_burst_proto(297-424)
app/test/test_pmd_null.c (4)
lib/mbuf/rte_mbuf.c (1)
rte_pktmbuf_pool_create(280-287)lib/ethdev/rte_ethdev.h (2)
rte_eth_rx_burst(6347-6406)rte_eth_tx_burst(6686-6746)lib/mbuf/rte_mbuf.h (2)
rte_pktmbuf_alloc_bulk(1068-1082)rte_pktmbuf_tailroom(1629-1634)lib/mempool/rte_mempool.c (1)
rte_mempool_avail_count(1021-1042)
⏰ 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)
app/test/meson.build (1)
144-144: LGTM!The dependency entry for
test_pmd_null.cfollows the established pattern used by similar PMD tests in this file (e.g.,test_pmd_ring.con line 146). The dependencies['net_ring', 'ethdev', 'bus_vdev']are appropriate for a vdev-based PMD test.drivers/net/null/rte_eth_null.c (1)
301-304: LGTM! Clean simplification of the info_get callback.The changes appropriately:
- Derive
internalsdirectly fromdev->data->dev_private, relying on the ethdev layer to validate arguments before invoking dev_ops callbacks- Use the more explicit
UINT32_MAXconstant instead of(uint32_t)-1formax_rx_pktlenapp/test/packet_burst_generator.h (1)
61-72: LGTM! Type widening aligns with DPDK conventions.The updated function signatures using
uint16_tfornb_pkt_per_burst,pkt_len, andnb_pkt_segsare appropriate:
- Allows burst sizes beyond the previous
uint8_tlimit of 255pkt_lenandnb_pkt_segsnow match the correspondingrte_mbuffield types (data_lenandnb_segs)app/test/test_pmd_null.c (1)
937-968: Well-structured test suite with good coverage.The test suite provides comprehensive coverage of the null PMD functionality including:
- Basic RX/TX operations
- Statistics tracking and reset
- Different PMD modes (copy, no-rx)
- Link status management
- Device info validation
- RSS/RETA configuration
- Multi-threaded TX verification
The test registration and suite structure follow DPDK testing conventions correctly.
app/test/packet_burst_generator.c (2)
209-224: LGTM! Improved segment handling and type consistency.The changes correctly:
- Update signature to use
uint16_tfor all parameters- Compute per-segment data lengths with proper remainder handling for the last segment
- Handle mbuf allocation failures gracefully with the
nomore_mbuflabel pattern
297-313: LGTM!Changes mirror those in
generate_packet_burstwith consistent segment length calculation and error handling.
Comment on lines
+176
to
+195
| static int | ||
| test_mbuf_setup_burst(struct rte_mbuf **bufs, unsigned int burst_size) | ||
| { | ||
| unsigned int i; | ||
|
|
||
| if (rte_pktmbuf_alloc_bulk(mp, bufs, burst_size) != 0) | ||
| return -1; | ||
|
|
||
| for (i = 0; i < burst_size; i++) { | ||
| struct rte_mbuf *m = bufs[i]; | ||
| uint16_t len; | ||
|
|
||
| /* Choose random length between ether min and available space */ | ||
| len = rte_rand_max(rte_pktmbuf_tailroom(m) - RTE_ETHER_MIN_LEN) | ||
| + RTE_ETHER_MIN_LEN; | ||
| m->data_len = len; | ||
| m->buf_len = len; | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Critical bug: Incorrectly modifying buf_len corrupts mbuf metadata.
buf_len represents the total buffer capacity allocated by the mempool and must not be modified. Setting m->buf_len = len corrupts the mbuf metadata, potentially causing buffer overflows or incorrect behavior in downstream code.
Additionally, pkt_len should be set to match data_len for single-segment packets.
🐛 Proposed fix
for (i = 0; i < burst_size; i++) {
struct rte_mbuf *m = bufs[i];
uint16_t len;
/* Choose random length between ether min and available space */
len = rte_rand_max(rte_pktmbuf_tailroom(m) - RTE_ETHER_MIN_LEN)
+ RTE_ETHER_MIN_LEN;
m->data_len = len;
- m->buf_len = len;
+ m->pkt_len = len;
}📝 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
| static int | |
| test_mbuf_setup_burst(struct rte_mbuf **bufs, unsigned int burst_size) | |
| { | |
| unsigned int i; | |
| if (rte_pktmbuf_alloc_bulk(mp, bufs, burst_size) != 0) | |
| return -1; | |
| for (i = 0; i < burst_size; i++) { | |
| struct rte_mbuf *m = bufs[i]; | |
| uint16_t len; | |
| /* Choose random length between ether min and available space */ | |
| len = rte_rand_max(rte_pktmbuf_tailroom(m) - RTE_ETHER_MIN_LEN) | |
| + RTE_ETHER_MIN_LEN; | |
| m->data_len = len; | |
| m->buf_len = len; | |
| } | |
| return 0; | |
| } | |
| static int | |
| test_mbuf_setup_burst(struct rte_mbuf **bufs, unsigned int burst_size) | |
| { | |
| unsigned int i; | |
| if (rte_pktmbuf_alloc_bulk(mp, bufs, burst_size) != 0) | |
| return -1; | |
| for (i = 0; i < burst_size; i++) { | |
| struct rte_mbuf *m = bufs[i]; | |
| uint16_t len; | |
| /* Choose random length between ether min and available space */ | |
| len = rte_rand_max(rte_pktmbuf_tailroom(m) - RTE_ETHER_MIN_LEN) | |
| RTE_ETHER_MIN_LEN; | |
| m->data_len = len; | |
| m->pkt_len = len; | |
| } | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In @app/test/test_pmd_null.c around lines 176 - 195, In test_mbuf_setup_burst,
do not modify the mbuf's buf_len (it is mempool-owned); remove the assignment to
m->buf_len and instead set m->data_len to the chosen length and also set
m->pkt_len = m->data_len for single-segment packets; ensure this change is made
after successful rte_pktmbuf_alloc_bulk in the function test_mbuf_setup_burst
and keep the length computation using rte_pktmbuf_tailroom(m) -
RTE_ETHER_MIN_LEN as before.
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
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=37007"
Summary by Sourcery
Add comprehensive unit tests for the null PMD and adjust related helpers and device info to support more realistic packet and queue characteristics.
New Features:
Bug Fixes:
Tests:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.