Conversation
The info_get callback doesn't need to check its args since already done by ethdev. The maximum packet size allowed by this dummy driver is limited only by the maximum values in mbuf fields. 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. 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 and lockless transmit. Original version of test was generated with Claude 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 GuideAdds a comprehensive unit test suite for the null PMD, adjusts the packet burst generator to support larger packet lengths and clearer per-segment sizing, and refines null PMD device info reporting to better match its capabilities, wiring the new test into the app/test build. Sequence diagram for null PMD test using packet burst generatorsequenceDiagram
actor Tester
participant NullPmdTest
participant EAL as EalInit
participant EthDev as rte_eth_dev
participant PBGen as PacketBurstGenerator
participant NullPMD as NullPmdDriver
Tester->>NullPmdTest: run_all_null_pmd_tests()
NullPmdTest->>EAL: rte_eal_init()
EAL-->>NullPmdTest: init_result
NullPmdTest->>EthDev: rte_eth_dev_configure(port_id, rx_conf, tx_conf)
EthDev-->>NullPmdTest: status
NullPmdTest->>EthDev: rte_eth_dev_start(port_id)
EthDev-->>NullPmdTest: status
NullPmdTest->>PBGen: init(total_pktlen, segment_sizes, nb_segs)
PBGen-->>NullPmdTest: status
loop for_each_test_case
NullPmdTest->>PBGen: generate_burst(mbufs, nb_pkts)
PBGen-->>NullPmdTest: mbufs
NullPmdTest->>EthDev: rte_eth_tx_burst(port_id, queue_id, mbufs, nb_pkts)
EthDev->>NullPMD: null_tx_burst()
NullPMD-->>EthDev: nb_tx
EthDev-->>NullPmdTest: nb_tx
NullPmdTest->>EthDev: rte_eth_rx_burst(port_id, queue_id, mbufs, nb_pkts)
EthDev->>NullPMD: null_rx_burst()
NullPMD-->>EthDev: nb_rx(usually_zero)
EthDev-->>NullPmdTest: nb_rx
NullPmdTest->>EthDev: rte_eth_dev_info_get(port_id, dev_info)
EthDev->>NullPMD: eth_dev_info(dev, dev_info)
NullPMD-->>EthDev: dev_info(max_rx_pktlen=UINT32_MAX,...)
EthDev-->>NullPmdTest: dev_info
NullPmdTest->>NullPmdTest: assert_capabilities_and_behavior()
end
NullPmdTest->>EthDev: rte_eth_dev_stop(port_id)
EthDev-->>NullPmdTest: status
NullPmdTest-->>Tester: report_results
Class diagram for null PMD device info and packet burst generator changesclassDiagram
class rte_eth_dev {
+rte_eth_dev_data* data
}
class rte_eth_dev_data {
+void* dev_private
}
class pmd_internals {
+rx_null_queue rx_null_queues[]
+tx_null_queue tx_null_queues[]
+uint16_t reta_size
}
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
+uint64_t tx_offload_capa
+uint16_t reta_size
}
class PacketBurstGeneratorConfig {
+uint32_t total_pktlen
+uint16_t segment_sizes[]
+uint1616_t nb_segs
}
class PacketBurstGenerator {
+PacketBurstGeneratorConfig config
+int init(uint32_t total_pktlen, uint16_t segment_sizes[], uint16_t nb_segs)
+int generate_burst(struct_rte_mbuf* mbufs[], uint16_t nb_pkts)
}
class NullPmdTest {
+int setup()
+int test_basic_tx_rx()
+int test_max_rx_pktlen_limits()
+int test_segmented_packets()
+int teardown()
}
rte_eth_dev --> rte_eth_dev_data : has
rte_eth_dev_data --> pmd_internals : dev_private
pmd_internals --> rte_eth_dev_info : populates
NullPmdTest --> rte_eth_dev : configures
NullPmdTest --> PacketBurstGenerator : uses
PacketBurstGenerator --> PacketBurstGeneratorConfig : has
rte_eth_dev_info <.. NullPmdTest : validates
Architecture/flow diagram for app/test null PMD test wiringflowchart LR
subgraph AppTest["app/test binary"]
direction TB
TestMain["test/main.c: test registry"]
NullPmdTestFile["test_pmd_null.c: null PMD tests"]
PBGenFile["packet_burst_generator.c"]
end
Meson["app/test/meson.build: test_pmd_null integration"]
DpdkCore["DPDK core (EAL, ethdev)"]
NullPMD["drivers/net/null: null PMD"]
Meson --> AppTest
TestMain --> NullPmdTestFile
NullPmdTestFile --> PBGenFile
NullPmdTestFile --> DpdkCore
DpdkCore --> NullPMD
NullPMD --> DpdkCore
DpdkCore --> NullPmdTestFile
PBGenFile --> DpdkCore
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds a comprehensive test suite for the null PMD ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 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
test_mbuf_setup_burst, you're modifyingm->buf_lenand never settingpkt_len; it would be safer and more conventional to leavebuf_lenas allocated by the mempool and set bothdata_lenandpkt_lento the chosen payload length to avoid unexpected behavior in other code paths. - In
test_null_reta_config, thereta_confarray is sized usingRTE_ETH_RSS_RETA_SIZE_128, but you index it usingdev_info.reta_size; to avoid potential overflow if the null PMD'sreta_sizeever differs, consider allocating the array based ondev_info.reta_size(e.g., via dynamic allocation) or asserting thatdev_info.reta_size <= RTE_ETH_RSS_RETA_SIZE_128.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_mbuf_setup_burst`, you're modifying `m->buf_len` and never setting `pkt_len`; it would be safer and more conventional to leave `buf_len` as allocated by the mempool and set both `data_len` and `pkt_len` to the chosen payload length to avoid unexpected behavior in other code paths.
- In `test_null_reta_config`, the `reta_conf` array is sized using `RTE_ETH_RSS_RETA_SIZE_128`, but you index it using `dev_info.reta_size`; to avoid potential overflow if the null PMD's `reta_size` ever differs, consider allocating the array based on `dev_info.reta_size` (e.g., via dynamic allocation) or asserting that `dev_info.reta_size <= RTE_ETH_RSS_RETA_SIZE_128`.
## Individual Comments
### Comment 1
<location> `app/test/test_pmd_null.c:189-192` </location>
<code_context>
+ 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;
</code_context>
<issue_to_address>
**issue (testing):** Random mbuf setup does not set pkt_len, which can make TX/statistics checks misleading
Here `data_len` and `buf_len` are updated but `pkt_len` is left at its previous value (often 0). For tests that rely on TX stats or PMD length handling (e.g., `test_null_tx_basic` and other stats checks), this can cause undercounted bytes and unrealistic mbufs. Please also set `m->pkt_len = len;` so these mbufs correctly represent single-segment packets.
</issue_to_address>
### Comment 2
<location> `app/test/test_pmd_null.c:615-624` </location>
<code_context>
+ struct rte_eth_rss_reta_entry64 reta_conf[RTE_ETH_RSS_RETA_SIZE_128 /
</code_context>
<issue_to_address>
**issue (bug_risk):** RETA test assumes reta_size ≤ RTE_ETH_RSS_RETA_SIZE_128 and may overflow the local array
In `test_null_reta_config`, `reta_conf` is statically sized for `RTE_ETH_RSS_RETA_SIZE_128 / RTE_ETH_RETA_GROUP_SIZE`, but `nreta` is derived from `dev_info.reta_size`. If `dev_info.reta_size` exceeds `RTE_ETH_RSS_RETA_SIZE_128`, `nreta` will be out of bounds and the test will write past `reta_conf`. Please either assert `dev_info.reta_size <= RTE_ETH_RSS_RETA_SIZE_128` before using `nreta`, or allocate `reta_conf` dynamically based on `dev_info.reta_size` (and free it).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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; | ||
|
|
||
| /* Create a new null device for RETA testing with multiple queues */ |
There was a problem hiding this comment.
issue (bug_risk): RETA test assumes reta_size ≤ RTE_ETH_RSS_RETA_SIZE_128 and may overflow the local array
In test_null_reta_config, reta_conf is statically sized for RTE_ETH_RSS_RETA_SIZE_128 / RTE_ETH_RETA_GROUP_SIZE, but nreta is derived from dev_info.reta_size. If dev_info.reta_size exceeds RTE_ETH_RSS_RETA_SIZE_128, nreta will be out of bounds and the test will write past reta_conf. Please either assert dev_info.reta_size <= RTE_ETH_RSS_RETA_SIZE_128 before using nreta, or allocate reta_conf dynamically based on dev_info.reta_size (and free it).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/test/meson.build:
- Line 144: Remove the extraneous 'net_ring' dependency from the test_pmd_null.c
entry in the meson build definition: update the dependency list for
'test_pmd_null.c' so it only contains ['ethdev', 'bus_vdev'] (remove 'net_ring')
to reflect that the test exercises only the null PMD.
In @app/test/test_pmd_null.c:
- Around line 176-195: In test_mbuf_setup_burst, don’t modify the mbuf’s buf_len
(it’s allocator-owned); remove the assignment to m->buf_len and instead set the
packet length metadata correctly by assigning m->pkt_len = len (keeping
m->data_len = len). Leave buf_len untouched so the mempool-managed buffer size
is not corrupted.
📜 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/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)
app/test/packet_burst_generator.h (1)
app/test/packet_burst_generator.c (1)
generate_packet_burst_proto(303-430)
⏰ 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 (16)
drivers/net/null/rte_eth_null.c (1)
301-304: LGTM!The simplification is appropriate: inline initialization of
internalsis cleaner, and usingUINT32_MAXinstead of(uint32_t)-1improves readability while being semantically equivalent. The removal of redundant null checks aligns with DPDK driver conventions wheredevanddev_infoare guaranteed valid by the caller.app/test/packet_burst_generator.h (1)
61-72: LGTM!The parameter type change from
uint8_ttouint16_tforpkt_lencorrectly enables support for packet sizes beyond 255 bytes. This aligns with typical Ethernet MTU requirements and the implementation changes in the corresponding.cfile.app/test/packet_burst_generator.c (2)
219-230: Good refactoring for larger packet support.The updated segment length calculation correctly handles remainder distribution to the last segment. However, there's no guard against
nb_pkt_segs == 0, which would cause a division by zero at line 229.Consider adding a defensive check or documenting the precondition that
nb_pkt_segs >= 1:🛡️ Optional: Add defensive check
/* Calculate per-segment data length */ + if (nb_pkt_segs == 0) + return -1; pkt_seg_data_len = pkt_len / nb_pkt_segs; last_seg_data_len = pkt_seg_data_len + (pkt_len % nb_pkt_segs);
308-319: Consistent withgenerate_packet_burstchanges.The same segment length calculation logic is correctly applied here. The same consideration regarding
nb_pkt_segs == 0applies.app/test/test_pmd_null.c (12)
1-35: Well-structured test header and constants.The includes, constants, and global variables are appropriately defined for the test suite. The use of
RTE_MAX_ETHPORTSas the initial invalid port_id is a good practice.
36-99: LGTM!The helper functions properly encapsulate device creation and configuration. Error handling in
create_null_portcorrectly cleans up on failure. Theconfigure_null_portfunction follows standard DPDK port setup patterns.
101-139: LGTM!Setup and teardown functions properly manage test resources. The teardown sequence correctly stops the device before closing and uninitializing.
231-276: LGTM!The stats test properly resets counters, performs RX/TX operations, and verifies the expected packet counts. Good coverage of the stats_get and stats_reset functionality.
281-319: LGTM!Good test coverage for the custom packet size configuration. The test properly creates a separate device instance, verifies the size, and cleans up.
324-389: LGTM!Both mode tests (
copyandno-rx) properly verify the expected behavior: copy mode allows RX, while no-rx mode returns zero packets on RX but still allows TX. Cleanup is correctly performed for each temporary port.
394-467: LGTM!The link status test properly verifies state transitions and restarts the device for subsequent tests. The device info test validates key capabilities including
MULTI_SEGS,MT_LOCKFREE, and RSS configuration values.
472-522: LGTM!Good stress test for verifying consistent behavior across multiple burst operations. The cumulative stats verification ensures packet counts are accurately tracked.
528-690: LGTM!Comprehensive RSS and RETA tests that properly configure multi-queue devices, update configurations, and verify the changes persist. The tests correctly handle the RSS prerequisite of multiple queues.
695-777: LGTM!The stats reset, MAC address, and promiscuous/allmulticast tests are straightforward and verify expected null PMD behaviors correctly.
836-935: Good MT_LOCKFREE validation test.The test properly verifies concurrent TX to a shared queue. The worker implementation correctly handles allocation failures and tracks transmission counts.
Minor note: The wait loop at lines 908-910 iterates over all worker lcores, not just the ones launched (limited to 4). This is safe but slightly wasteful. Consider breaking after waiting on
num_workerslcores if performance matters.
937-968: LGTM!The test suite is properly structured with setup/teardown hooks and comprehensive test case coverage. The
REGISTER_FAST_TESTmacro correctly registers it for the autotest framework.
| 'test_per_lcore.c': [], | ||
| 'test_pflock.c': [], | ||
| 'test_pie.c': ['sched'], | ||
| 'test_pmd_null.c': ['net_ring', 'ethdev', 'bus_vdev'], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if test_pmd_null.c uses any ring-related functionality
rg -n 'ring|net_ring' app/test/test_pmd_null.c || echo "No ring references found"Repository: ovsrobot/dpdk
Length of output: 78
🏁 Script executed:
# Read the test_pmd_null.c file to see what it imports and uses
cat app/test/test_pmd_null.cRepository: ovsrobot/dpdk
Length of output: 26070
🏁 Script executed:
# Check the meson.build context around line 144
sed -n '140,150p' app/test/meson.buildRepository: ovsrobot/dpdk
Length of output: 604
🏁 Script executed:
# Compare with test_pmd_ring.c dependencies in meson.build
sed -n '145,147p' app/test/meson.buildRepository: ovsrobot/dpdk
Length of output: 251
Remove unnecessary net_ring dependency from test_pmd_null.c.
The test file does not use the ring PMD at all—it exclusively tests the null PMD (net_null). The net_ring dependency was copied from test_pmd_ring.c and should be removed. The correct dependencies are ['ethdev', 'bus_vdev'].
🤖 Prompt for AI Agents
In @app/test/meson.build at line 144, Remove the extraneous 'net_ring'
dependency from the test_pmd_null.c entry in the meson build definition: update
the dependency list for 'test_pmd_null.c' so it only contains ['ethdev',
'bus_vdev'] (remove 'net_ring') to reflect that the test exercises only the null
PMD.
| 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.
Bug: Incorrect modification of buf_len corrupts mbuf metadata.
Line 192 incorrectly sets m->buf_len = len. The buf_len field represents the total allocated buffer size and must not be modified after allocation—it's set by the mempool and remains constant. Only data_len and pkt_len should be modified to indicate actual packet data length.
This corruption could cause memory safety issues when the mbuf is freed or reused.
🐛 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.
| 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,
don’t modify the mbuf’s buf_len (it’s allocator-owned); remove the assignment to
m->buf_len and instead set the packet length metadata correctly by assigning
m->pkt_len = len (keeping m->data_len = len). Leave buf_len untouched so the
mempool-managed buffer size is not corrupted.
2 participants
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36997"
Summary by Sourcery
Add comprehensive unit tests for the null Ethernet PMD and adjust supporting utilities to better handle larger packet sizes and device capabilities.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.