On Mon, 5 Jan 2026 14:49:41 +0000
Marat Khalili <[email protected]> wrote:
> Thank you for doing this. Having tests for things like this is important both
> to make sure they work and as a form of documentation.
>
> In my humble opinion we should not approach this mechanically though, which
> is
> what AI tools tend to do. It is usually said that tests should test desired
> behavior, not specific implementation. Particularly in settings some of the
> values might not matter, or the whole range might be acceptable, in other
> cases
> current implementation might be questionable and by writing a test we just
> legitimize it.
>
> Please more see comments inline.
I did not see anything obviously wrong with the tests that it generated so
far. And coverage is more important on legacy code.
> > diff --git a/app/test/test_pmd_null.c b/app/test/test_pmd_null.c
> > new file mode 100644
> > index 0000000000..ebc2447c6d
> > --- /dev/null
> > +++ b/app/test/test_pmd_null.c
> > @@ -0,0 +1,827 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2026 Stephen Hemminger
> > + */
> > +
> > +#include <inttypes.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include <rte_bus_vdev.h>
> > +#include <rte_cycles.h>
> > +#include <rte_ethdev.h>
> > +#include <rte_ether.h>
> > +#include <rte_lcore.h>
> > +#include <rte_mbuf.h>
> > +#include <rte_mempool.h>
>
> I do not think it is a problem, but just curious: how does test_pmd_ring.c
> manage to use much fewer includes?
I tend to run the tool iwyu on the generated code. This gives a more correct
set of includes without implied includes.
> > +/* Test device names */
> > +#define NULL_DEV_NAME "net_null_test"
> > +
> > +static struct rte_mempool *mp;
> > +static uint16_t port_id = RTE_MAX_ETHPORTS;
> > +static bool port_created;
> > +static bool port_started;
>
> // snip setup and teardown functions, could find nothing to nitpick there
The code is exercising device in multiple ways so it didn't seem to need them.
Would be better to avoid global variables though. Like the mempool should
be set in setup.
> Default packet size repeats a few times in this file, can we have a global
> constant for it?
>
> > + TEST_ASSERT(bufs[i]->pkt_len == 64,
> > + "Unexpected pkt_len: %u", bufs[i]->pkt_len);
> > + TEST_ASSERT(bufs[i]->data_len == 64,
> > + "Unexpected data_len: %u", bufs[i]->data_len);
> > + }
> > +
Right test should use RTE_ETHER_MIN_LEN
> > + /* Allocate mbufs for TX */
> > + for (i = 0; i < BURST_SIZE; i++) {
> > + bufs[i] = rte_pktmbuf_alloc(mp);
> > + TEST_ASSERT(bufs[i] != NULL, "Failed to allocate mbuf");
> > + bufs[i]->data_len = 64;
> > + bufs[i]->pkt_len = 64;
>
> It would help to test sending packets of various sizes, especially spanning
> multiple buffers or allocating them at the edge of buffer. May even uncover
> some bugs in the copy mode implementation ;)
Good point.
The copy mode is making several assumptions already and I see bugs there.
It is confusing dummy_packet as mbuf versus data.
The test and the driver should also test multi-segments.
> > + /* After start, link should be UP */
> > + TEST_ASSERT(link.link_status == RTE_ETH_LINK_UP,
> > + "Expected link UP after start");
> > + TEST_ASSERT(link.link_speed == RTE_ETH_SPEED_NUM_10G,
> > + "Expected 10G link speed");
>
> I am not sure it is important to test that the link speed is exactly 10G.
> Will
> it be a bug if it becomes 25G tomorrow? Probably any valid value would do.
It is a dummy driver, speed really doesn't matter and should probably be
ignored.
> > +static int
> > +test_null_dev_info(void)
> > +{
> > + struct rte_eth_dev_info dev_info;
> > + int ret;
> > +
> > + ret = rte_eth_dev_info_get(port_id, &dev_info);
> > + TEST_ASSERT(ret == 0, "Failed to get device info");
> > +
> > + /* Verify expected device info values */
> > + TEST_ASSERT(dev_info.max_mac_addrs == 1,
> > + "Expected max_mac_addrs=1, got %u", dev_info.max_mac_addrs);
> > + TEST_ASSERT(dev_info.max_rx_pktlen == (uint32_t)-1,
> > + "Unexpected max_rx_pktlen");
>
> Why is (uint32_t)-1 is a valid packet length for this device? Can at actually
> accept them?
This is awkward way of saying UINT32_MAX.
And it means the driver has no limit on what the receive buffer might be.
This is not really right it does have limits based on the device flags.
>
> > + TEST_ASSERT(dev_info.min_rx_bufsize == 0,
> > + "Expected min_rx_bufsize=0, got %u",
> > dev_info.min_rx_bufsize);
> > +
> > + /* Check TX offload capabilities */
> > + TEST_ASSERT(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_MULTI_SEGS,
> > + "Expected MULTI_SEGS TX offload capability");
>
> It sure sets these values, but are they correct values, and would anything
> else
> be incorrect?
I think test is ok testing for what it expects to work.
> > +/*
> > + * Test: RSS configuration
> > + * Note: RSS requires multi-queue configuration
> > + */
> > +static int
> > +test_null_rss_config(void)
> > +{
> > + struct rte_eth_dev_info dev_info;
> > + struct rte_eth_rss_conf rss_conf;
> > + struct rte_eth_conf port_conf = {0};
> > + uint8_t rss_key[40];
> > + uint16_t rss_port;
> > + const uint16_t num_queues = 2;
> > + uint16_t q;
> > + int ret;
> > +
> > + /* Create a new null device for RSS testing with multiple queues */
> > + ret = create_null_port("net_null_rss_test", NULL, &rss_port);
> > + TEST_ASSERT(ret == 0, "Failed to create null port for RSS test");
> > +
> > + ret = rte_eth_dev_info_get(rss_port, &dev_info);
> > + TEST_ASSERT(ret == 0, "Failed to get device info");
> > +
> > + /* Configure with RSS enabled and multiple queues */
> > + port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_RSS;
> > + port_conf.rx_adv_conf.rss_conf.rss_hf = dev_info.flow_type_rss_offloads;
> > +
> > + ret = rte_eth_dev_configure(rss_port, num_queues, num_queues,
> > &port_conf);
> > + TEST_ASSERT(ret == 0, "Failed to configure RSS port");
> > +
> > + for (q = 0; q < num_queues; q++) {
> > + ret = rte_eth_rx_queue_setup(rss_port, q, RING_SIZE,
> > + rte_eth_dev_socket_id(rss_port),
> > + NULL, mp);
> > + TEST_ASSERT(ret == 0, "Failed to setup RX queue %u", q);
> > +
> > + ret = rte_eth_tx_queue_setup(rss_port, q, RING_SIZE,
> > + rte_eth_dev_socket_id(rss_port),
> > + NULL);
> > + TEST_ASSERT(ret == 0, "Failed to setup TX queue %u", q);
> > + }
> > +
> > + ret = rte_eth_dev_start(rss_port);
> > + TEST_ASSERT(ret == 0, "Failed to start RSS port");
> > +
> > + /* Get current RSS config */
> > + memset(&rss_conf, 0, sizeof(rss_conf));
> > + 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 */
> > + for (unsigned int i = 0; i < sizeof(rss_key); i++) {
> > + TEST_ASSERT(rss_key[i] == 0x55,
> > + "RSS key not updated at byte %u", i);
> > + }
> > +
>
> Can we receive and send something from/to these queues and verify resulting
> statistics? Statistics is one of the most important use cases for the null
> device, and it does benefit from multi-queue.
Use of RSS in Null PMD is odd. It really doesn't do RSS. It is lying about it.
The packets are never hashed on receive. Looks like it was just a way to
exercise
the RSS API's.
My goal is to get some tests for vdev's where there are none now, and fix
some of the obvious wonky stuff in the drivers (like doing stats in odd and
slow ways).
