|SUCCESS| test/latency: fix intermittent failure on slow platforms

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164697

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-31

55a75e9c test/latency: fix intermittent failure on slow platforms
Author: Stephen Hemminger <stephen at networkplumber.org>

test/latency: fix intermittent failure on slow platforms

The forwarding loop was bounded by a fixed interval of 0.5ms but on slow
or emulated platforms with a low-frequency timebase (e.g. RISC-V rdtime)
this fails because the loop only ran once. The test needs two iterations
to get any samples.

Rearrange the forwarding loop so that a minimum number of iterations are
required. The loop still has an upper bound on packets and time interval
which is expanded to 10 ms.

If no samples are collected, mark the test as skipped. Refactor the
forwarding loop test so that cleanup happens on failure.

> diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c
> index e0483e3e4f..76498b66b4 100644
> --- a/app/test/test_latencystats.c
> +++ b/app/test/test_latencystats.c

[ ... ]

> @@ -16,6 +16,10 @@
>  
>  #define NUM_STATS 5
>  #define LATENCY_NUM_PACKETS 10
> +#define LATENCY_FORWARD_ITERATIONS 10000u
> +#define LATENCY_FORWARD_MS 10u
> +#define MIN_ITERATIONS	10u

Is this line using tabs to align the macro value? The coding style
forbids aligned whitespace for variable initializations and macro
definitions. Can these macros use consistent spacing (single space or
tab) to comply with the style guide?

> @@ -140,28 +144,13 @@ static void test_latency_ring_free(void)
>  	test_vdev_uninit("net_ring_net_ringa");
>  }
>  
> -static int test_latency_packet_forward(void)
> +static int test_forward_loop(uint16_t portid, struct rte_mbuf *pbuf[LATENCY_NUM_PACKETS])
>  {
> -	unsigned int i;
> -	int ret;
> -	struct rte_mbuf *pbuf[LATENCY_NUM_PACKETS] = { };
> -	struct rte_mempool *mp;
> -	char poolname[] = "mbuf_pool";
> +	unsigned int i, iters = 0;
>  	uint64_t end_cycles;
>  	struct rte_metric_value values[NUM_STATS] = { };
>  	struct rte_metric_name names[NUM_STATS] = { };
> -
> -	ret = test_get_mbuf_from_pool(&mp, pbuf, poolname);
> -	if (ret < 0) {
> -		printf("allocate mbuf pool Failed\n");
> -		return TEST_FAILED;
> -	}
> -	ret = test_dev_start(portid, mp);
> -	if (ret < 0) {
> -		printf("test_dev_start(%hu, %p) failed, error code: %d\n",
> -			portid, mp, ret);
> -		return TEST_FAILED;
> -	}
> +	int ret;

[ ... ]

> @@ -170,37 +159,65 @@ static int test_latency_packet_forward(void)
>  	TEST_ASSERT(ret == NUM_STATS, "Test failed to get results before forwarding");
>  	TEST_ASSERT(values[4].value == 0, "Samples not zero at start of test");
>  
> -	/*
> -	 * Want test to run long enough to collect sufficient samples
> -	 * but not so long that scheduler decides to reschedule it (1000 hz).
> -	 */
> -	end_cycles = rte_rdtsc() + rte_get_tsc_hz() / 2000;
> -	do {
> +	/* Want test to run long enough to collect sufficient samples */
> +	end_cycles = rte_rdtsc() + rte_get_tsc_hz() / MS_PER_S * LATENCY_FORWARD_MS;

Where is MS_PER_S defined? Does this file include the header that defines
this constant? If not, does this code introduce an implicit dependency on
a global constant without appropriate includes?

> +	for (i = 0; i < LATENCY_FORWARD_ITERATIONS; i++) {
>  		ret = test_packet_forward(pbuf, portid, QUEUE_ID);
>  		if (ret < 0)
>  			printf("send pkts Failed\n");
> -	} while (rte_rdtsc() < end_cycles);
> +
> +		if (++iters >= MIN_ITERATIONS && rte_rdtsc() >= end_cycles)
> +			break;
> +	}

Does this code properly count iterations? The loop increments both i and
iters - is iters needed or does i provide the count already?

> +	if (values[4].value == 0) {
> +		printf("No latency samples collected after %u iterations, skipping\n", iters);
> +		return TEST_SKIPPED;
> +	}

[ ... ]

> @@ -16,6 +16,10 @@
>  
>  #define NUM_STATS 5
>  #define LATENCY_NUM_PACKETS 10
> +#define LATENCY_FORWARD_ITERATIONS 10000u
> +#define LATENCY_FORWARD_MS 10u
> +#define MIN_ITERATIONS	10u

Does this code follow the DPDK style for constant suffixes? The coding
style specifies using uppercase U for unsigned constants. Are these
lowercase u suffixes compliant?