Conversation
Add a test for null PMD. 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>
The dev_info_get callback does not need to check for invalid arguments since that is already done at ethdev layer. Make the min/max MTU values reflect the most an Ethernet type PMD would allow. 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 net PMD and updates the driver’s device info reporting to expose realistic MTU and max RX packet length values required by the new tests. Sequence diagram for null PMD eth_dev_info test interactionsequenceDiagram
actor TestRunner
participant TestPMDNullSuite
participant NullEthDriver
participant rte_eth_dev
participant pmd_internals
participant rte_eth_dev_info
TestRunner ->> TestPMDNullSuite: run_eth_dev_info_tests()
TestPMDNullSuite ->> NullEthDriver: eth_dev_info(dev, dev_info)
NullEthDriver ->> rte_eth_dev: access data
rte_eth_dev ->> pmd_internals: get dev_private
NullEthDriver ->> rte_eth_dev_info: set max_mac_addrs = 1
NullEthDriver ->> rte_eth_dev_info: set max_rx_queues, max_tx_queues
NullEthDriver ->> rte_eth_dev_info: set tx_offload_capa
NullEthDriver ->> rte_eth_dev_info: set max_rx_pktlen = RTE_ETHER_MAX_JUMBO_FRAME_LEN
NullEthDriver ->> rte_eth_dev_info: set max_mtu = RTE_ETHER_MAX_JUMBO_FRAME_LEN - RTE_ETHER_HDR_LEN - RTE_ETHER_CRC_LEN
NullEthDriver ->> rte_eth_dev_info: set reta_size, flow_type_rss_offloads, hash_key_size
NullEthDriver -->> TestPMDNullSuite: return 0
TestPMDNullSuite ->> TestRunner: assert reported values match expectations
Class diagram for null PMD eth_dev_info data structuresclassDiagram
class NullEthDriver {
+int eth_dev_info(rte_eth_dev* dev, rte_eth_dev_info* dev_info)
}
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
+uint64_t flow_type_rss_offloads
+uint8_t rss_key[]
}
class rte_eth_dev_info {
+uint32_t max_mac_addrs
+uint16_t max_rx_queues
+uint16_t max_tx_queues
+uint32_t max_rx_pktlen
+uint32_t max_mtu
+uint32_t reta_size
+uint64_t flow_type_rss_offloads
+size_t hash_key_size
+uint64_t tx_offload_capa
}
NullEthDriver ..> rte_eth_dev : uses
NullEthDriver ..> rte_eth_dev_info : fills
rte_eth_dev "1" o-- "1" rte_eth_dev_data
rte_eth_dev_data "1" o-- "1" pmd_internals
NullEthDriver ..> pmd_internals : reads
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughA new comprehensive PMD null device unit test suite is introduced in test_pmd_null.c with build configuration updates. The null driver's device info function is refactored to use proper jumbo frame constants and simplify null-safety handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Framework
participant Setup as test_null_setup()
participant PMD as PMD Null Device
participant Mempool as Mempool
participant Tests as Test Cases
participant Teardown as test_null_teardown()
Test->>Setup: Initialize resources
Setup->>Mempool: Create mempool
Setup->>PMD: Create null port (vdev)
Setup->>PMD: Configure port (queues, mtu)
Setup->>PMD: Start port
Setup->>Test: Return success
Test->>Tests: Run battery of unit tests
Tests->>PMD: RX/TX operations, stats, config queries
PMD->>Mempool: Allocate/free mbufs
Tests->>Tests: Assert counts, sizes, capabilities
Tests->>Test: Return test results
Test->>Teardown: Cleanup resources
Teardown->>PMD: Stop and close port
Teardown->>Mempool: Free mempool
Teardown->>Test: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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 directly settingm->data_lenandm->buf_lenwithout touchingpkt_lenor using helpers likerte_pktmbuf_append(), which violates mbuf invariants; it would be safer to keepbuf_lenunchanged and setpkt_lenalong withdata_lenvia the standard APIs so TX and stats behave as expected. - The
eth_dev_info()change now only setsmax_rx_pktlenandmax_mtubut leavesmin_mtuuntouched while the new test asserts a specificmin_mtuvalue; consider explicitly initializingmin_mtu(and any other fields the tests assume) in the PMD to avoid relying on default-initialized values from the core.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_mbuf_setup_burst()` you’re directly setting `m->data_len` and `m->buf_len` without touching `pkt_len` or using helpers like `rte_pktmbuf_append()`, which violates mbuf invariants; it would be safer to keep `buf_len` unchanged and set `pkt_len` along with `data_len` via the standard APIs so TX and stats behave as expected.
- The `eth_dev_info()` change now only sets `max_rx_pktlen` and `max_mtu` but leaves `min_mtu` untouched while the new test asserts a specific `min_mtu` value; consider explicitly initializing `min_mtu` (and any other fields the tests assume) in the PMD to avoid relying on default-initialized values from the core.
## Individual Comments
### Comment 1
<location> `app/test/test_pmd_null.c:448-449` </location>
<code_context>
+
+ TEST_ASSERT(dev_info.max_mtu == jumbo_mtu,
+ "Unexpected max_mtu: %u", dev_info.max_mtu);
+ TEST_ASSERT(dev_info.min_mtu == min_mtu,
+ "Unexpected min_mtu: %u", dev_info.max_mtu);
+ TEST_ASSERT(dev_info.max_rx_pktlen == RTE_ETHER_MAX_JUMBO_FRAME_LEN,
+ "Unexpected max_rx_pktlen: %u", dev_info.max_rx_pktlen);
</code_context>
<issue_to_address>
**issue (testing):** The min_mtu assertion may be too strict / checking the wrong value and could be brittle across implementations.
This test asserts `dev_info.min_mtu == min_mtu`, but the PMD never sets `min_mtu`, so the value likely comes from ethdev defaults and may change independently of the null driver. The error message also prints `dev_info.max_mtu` instead of `dev_info.min_mtu`, which both hinders debugging and suggests this path hasn’t been exercised. Consider either relaxing the assertion to only check properties guaranteed by the null PMD, or explicitly setting `min_mtu` in the PMD and then validating/printing `dev_info.min_mtu` here.
</issue_to_address>
### Comment 2
<location> `app/test/test_pmd_null.c:592-599` </location>
<code_context>
+ rss_conf.rss_key = rss_key;
+ rss_conf.rss_key_len = sizeof(rss_key);
+
+ ret = rte_eth_dev_rss_hash_conf_get(rss_port, &rss_conf);
+ TEST_ASSERT(ret == 0, "Failed to get RSS hash config");
+
+ /* Update RSS config with new key */
+ memset(rss_key, 0x55, sizeof(rss_key));
+ rss_conf.rss_key = rss_key;
+ rss_conf.rss_key_len = sizeof(rss_key);
+ rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
+
+ ret = rte_eth_dev_rss_hash_update(rss_port, &rss_conf);
+ TEST_ASSERT(ret == 0, "Failed to update RSS hash config");
+
+ /* Verify the update */
+ memset(rss_key, 0, sizeof(rss_key));
+ rss_conf.rss_key = rss_key;
+
+ ret = rte_eth_dev_rss_hash_conf_get(rss_port, &rss_conf);
+ TEST_ASSERT(ret == 0, "Failed to get RSS hash config after update");
+
+ /* Verify key was updated */
</code_context>
<issue_to_address>
**suggestion (testing):** The RSS config test only verifies the key bytes, not the hash function flags that were updated.
In `test_null_rss_config`, you set `rss_conf.rss_hf = dev_info.flow_type_rss_offloads` before `rte_eth_dev_rss_hash_update()`, but after the second `rte_eth_dev_rss_hash_conf_get()` you only check the key bytes. Please also assert that `rss_conf.rss_hf` matches the expected value (or is at least non-zero) so the test validates both the key and the hash function flags.
```suggestion
ret = rte_eth_dev_rss_hash_conf_get(rss_port, &rss_conf);
TEST_ASSERT(ret == 0, "Failed to get RSS hash config after update");
/* Verify RSS hash function flags were updated */
TEST_ASSERT(rss_conf.rss_hf == dev_info.flow_type_rss_offloads,
"RSS hash function flags not updated as expected");
/* Verify key was updated */
for (unsigned int i = 0; i < sizeof(rss_key); i++) {
TEST_ASSERT(rss_key[i] == 0x55,
"RSS key not updated at byte %u", i);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @app/test/meson.build:
- Line 144: The test entry for 'test_pmd_null.c' declares the wrong dependency:
replace 'net_ring' with 'net_null' in the dependency list for 'test_pmd_null.c'
so the Meson target depends on the null PMD (net_null) rather than the ring PMD;
update the array ['net_ring', 'ethdev', 'bus_vdev'] to ['net_null', 'ethdev',
'bus_vdev'] for the 'test_pmd_null.c' target.
In @app/test/test_pmd_null.c:
- Around line 448-449: The assertion message incorrectly prints dev_info.max_mtu
instead of the actual min value; update the TEST_ASSERT call that compares
dev_info.min_mtu to min_mtu so the format argument and message reference the
correct value (dev_info.min_mtu or min_mtu) — e.g., change the "%u",
dev_info.max_mtu argument to "%u", dev_info.min_mtu (or to print both expected
and actual use "%u (expected %u)", dev_info.min_mtu, min_mtu) so the error text
matches the checked variable.
- Around line 448-449: The TEST_ASSERT for min_mtu uses the wrong value in the
format arguments: update the assertion calling TEST_ASSERT(dev_info.min_mtu ==
min_mtu, ...) to pass dev_info.min_mtu (not dev_info.max_mtu) as the value
inserted into the "%u" format string so the error message shows the actual
min_mtu when the check fails; locate the TEST_ASSERT that compares
dev_info.min_mtu and min_mtu and replace the format argument dev_info.max_mtu
with dev_info.min_mtu.
- Around line 181-190: The loop incorrectly overwrites the mbuf's buf_len
(actual allocated buffer size) which must not be modified; instead, after
computing len using rte_pktmbuf_tailroom(m) and RTE_ETHER_MIN_LEN, assign
m->data_len = len and also set m->pkt_len = len (do not touch m->buf_len),
ensuring burst_size and bufs[] are otherwise unchanged so packet metadata
remains consistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/test/meson.buildapp/test/test_pmd_null.cdrivers/net/null/rte_eth_null.c
🧰 Additional context used
🧬 Code graph analysis (1)
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 (10)
drivers/net/null/rte_eth_null.c (2)
297-316: LGTM on the refactoredeth_dev_infofunction.The simplified access pattern for
dev_privateis consistent with other functions in this file (e.g.,eth_stats_get). Removing redundant null checks that are handled at the ethdev layer is a reasonable cleanup.
308-310: No action required.RTE_ETHER_MAX_JUMBO_FRAME_LENis not deprecated and remains available in DPDK. The constant is defined inlib/net/rte_ether.has0x3F00and is actively used throughout the codebase, including in the same file at lines 308-309. The code is correct.Likely an incorrect or invalid review comment.
app/test/test_pmd_null.c (8)
21-31: LGTM on constants and test configuration.The buffer pool size (256 mbufs) is sufficient for the test workloads, which use a maximum of ~64 mbufs concurrently during the stats test.
125-136: LGTM on teardown function.Ignoring return values during teardown is acceptable since cleanup should proceed regardless of individual errors.
33-96: LGTM on port creation and configuration helpers.Error handling is properly implemented with cleanup on failure in
create_null_port.
278-386: LGTM on configuration-specific test cases.The custom size, copy mode, and no-rx mode tests are well-structured with proper port lifecycle management and cleanup.
530-692: LGTM on RSS and RETA configuration tests.Comprehensive testing of RSS hash configuration and RETA table operations with proper multi-queue setup.
781-811: LGTM on test suite structure and registration.The test suite follows standard DPDK testing patterns with proper setup/teardown and case registration.
742-760: LGTM on MAC address test.The test appropriately validates that the MAC address operations don't return errors. Given that the null PMD's
eth_mac_address_setis a no-op that returns success, verification of the actual value isn't meaningful.
141-273: LGTM on core RX/TX and statistics tests.Comprehensive testing of basic RX/TX functionality and statistics tracking with proper verification of expected values.
| '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:
# First, let's check if the net_null driver exists in the repository
fd -t d 'null' drivers/net/ 2>/dev/null || echo "Searching for null driver..."
find . -path '*/drivers/net/null*' -type d 2>/dev/nullRepository: ovsrobot/dpdk
Length of output: 93
🏁 Script executed:
# Check for the net_null driver registration and meson configuration
rg -n "net_null" drivers/ --type=c --type=meson 2>/dev/null | head -20Repository: ovsrobot/dpdk
Length of output: 295
🏁 Script executed:
# Look at the meson.build file around line 144 to understand the pattern
sed -n '130,160p' app/test/meson.buildRepository: ovsrobot/dpdk
Length of output: 1468
🏁 Script executed:
# Check what test_pmd_null.c actually tests
head -30 app/test/test_pmd_null.cRepository: ovsrobot/dpdk
Length of output: 646
Incorrect dependency: should use net_null instead of net_ring.
This test file tests the null PMD (net_null), not the ring PMD. The dependency should match the driver being tested.
🔎 Proposed fix
- 'test_pmd_null.c': ['net_ring', 'ethdev', 'bus_vdev'],
+ 'test_pmd_null.c': ['net_null', 'ethdev', 'bus_vdev'],📝 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.
| 'test_pmd_null.c': ['net_ring', 'ethdev', 'bus_vdev'], | |
| 'test_pmd_null.c': ['net_null', 'ethdev', 'bus_vdev'], |
🤖 Prompt for AI Agents
In @app/test/meson.build at line 144, The test entry for 'test_pmd_null.c'
declares the wrong dependency: replace 'net_ring' with 'net_null' in the
dependency list for 'test_pmd_null.c' so the Meson target depends on the null
PMD (net_null) rather than the ring PMD; update the array ['net_ring', 'ethdev',
'bus_vdev'] to ['net_null', 'ethdev', 'bus_vdev'] for the 'test_pmd_null.c'
target.
| 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; | ||
| } |
There was a problem hiding this comment.
Incorrect modification of buf_len and missing pkt_len assignment.
buf_len represents the actual buffer size allocated by the mempool and should not be modified. Setting it to a smaller value corrupts the mbuf metadata. Additionally, pkt_len should be set for proper packet handling.
🔎 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.
| 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; | |
| } | |
| 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; | |
| } |
🤖 Prompt for AI Agents
In @app/test/test_pmd_null.c around lines 181 - 190, The loop incorrectly
overwrites the mbuf's buf_len (actual allocated buffer size) which must not be
modified; instead, after computing len using rte_pktmbuf_tailroom(m) and
RTE_ETHER_MIN_LEN, assign m->data_len = len and also set m->pkt_len = len (do
not touch m->buf_len), ensuring burst_size and bufs[] are otherwise unchanged so
packet metadata remains consistent.
| TEST_ASSERT(dev_info.min_mtu == min_mtu, | ||
| "Unexpected min_mtu: %u", dev_info.max_mtu); |
There was a problem hiding this comment.
Typo in assertion message: prints max_mtu instead of min_mtu.
The assertion checks dev_info.min_mtu but the error message incorrectly references dev_info.max_mtu.
🔎 Proposed fix
TEST_ASSERT(dev_info.min_mtu == min_mtu,
- "Unexpected min_mtu: %u", dev_info.max_mtu);
+ "Unexpected min_mtu: %u", dev_info.min_mtu);📝 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.
| TEST_ASSERT(dev_info.min_mtu == min_mtu, | |
| "Unexpected min_mtu: %u", dev_info.max_mtu); | |
| TEST_ASSERT(dev_info.min_mtu == min_mtu, | |
| "Unexpected min_mtu: %u", dev_info.min_mtu); |
🤖 Prompt for AI Agents
In @app/test/test_pmd_null.c around lines 448 - 449, The assertion message
incorrectly prints dev_info.max_mtu instead of the actual min value; update the
TEST_ASSERT call that compares dev_info.min_mtu to min_mtu so the format
argument and message reference the correct value (dev_info.min_mtu or min_mtu) —
e.g., change the "%u", dev_info.max_mtu argument to "%u", dev_info.min_mtu (or
to print both expected and actual use "%u (expected %u)", dev_info.min_mtu,
min_mtu) so the error text matches the checked variable.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the test file around lines 448-449
find . -name "test_pmd_null.c" -type f | head -5Repository: ovsrobot/dpdk
Length of output: 83
🏁 Script executed:
# Look at the driver implementation
find . -path "*/drivers/net/null/rte_eth_null.c" -type f | head -5Repository: ovsrobot/dpdk
Length of output: 90
🏁 Script executed:
# Search for min_mtu initialization in ethdev
find . -path "*/lib/ethdev/*.c" -type f | head -10Repository: ovsrobot/dpdk
Length of output: 362
🏁 Script executed:
# Examine the test file around lines 448-449 with context
sed -n '430,460p' ./app/test/test_pmd_null.cRepository: ovsrobot/dpdk
Length of output: 1292
🏁 Script executed:
# Look at the driver's eth_dev_info function
rg -n "eth_dev_info|min_mtu|min_rx_bufsize" ./drivers/net/null/rte_eth_null.c -A 3 -B 1Repository: ovsrobot/dpdk
Length of output: 441
🏁 Script executed:
# Check ethdev layer for min_mtu initialization
rg -n "min_mtu\s*=" ./lib/ethdev/rte_ethdev.c -B 2 -A 2Repository: ovsrobot/dpdk
Length of output: 259
🏁 Script executed:
# Get the full eth_dev_info function from the driver
sed -n '297,330p' ./drivers/net/null/rte_eth_null.cRepository: ovsrobot/dpdk
Length of output: 1204
🏁 Script executed:
# Also check for any min_mtu/min_rx_bufsize assignments in the driver
rg -n "dev_info\->" ./drivers/net/null/rte_eth_null.c | head -30Repository: ovsrobot/dpdk
Length of output: 621
🏁 Script executed:
# Check line 453 to see if it has the same format string bug
sed -n '450,455p' ./app/test/test_pmd_null.cRepository: ovsrobot/dpdk
Length of output: 339
Fix format string error in min_mtu test assertion.
Line 449 has a format string bug: the assertion checks min_mtu but prints dev_info.max_mtu instead of dev_info.min_mtu. This causes error messages to display the wrong value if the assertion fails.
Affected code
TEST_ASSERT(dev_info.min_mtu == min_mtu,
"Unexpected min_mtu: %u", dev_info.max_mtu);
Should be:
TEST_ASSERT(dev_info.min_mtu == min_mtu,
"Unexpected min_mtu: %u", dev_info.min_mtu);
Line 453 is correct and does not have this issue.
🤖 Prompt for AI Agents
In @app/test/test_pmd_null.c around lines 448 - 449, The TEST_ASSERT for min_mtu
uses the wrong value in the format arguments: update the assertion calling
TEST_ASSERT(dev_info.min_mtu == min_mtu, ...) to pass dev_info.min_mtu (not
dev_info.max_mtu) as the value inserted into the "%u" format string so the error
message shows the actual min_mtu when the check fails; locate the TEST_ASSERT
that compares dev_info.min_mtu and min_mtu and replace the format argument
dev_info.max_mtu with dev_info.min_mtu.
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36974"
Summary by Sourcery
Add comprehensive unit test coverage for the null Ethernet PMD and align its reported device info with standard Ethernet MTU and jumbo frame limits.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.