[PATCH] test/bonding: fix active backup rx test

[lihuisong (C)]

Hi Stephen,

This patch looks good to me.

But this test case still runs failure when I run "link_bonding_autotest" 
twice. like:
-->
...
EAL: Test assert configure_ethdev line 181 failed: rte_eth_dev_configure 
for port 7 failed
EAL: Test assert initialize_bonding_device_with_members line 1138 
failed: Failed to configure bonding port (7) in mode 3 with (4) members.
EAL: Test assert 
test_broadcast_verify_member_link_status_change_behaviour line 4060 
failed: Failed to initialise bonding device
  + TestCase [62] : 
test_broadcast_verify_member_link_status_change_behaviour failed
ETHDEV: Invalid port_id=7
EAL: Test assert configure_ethdev line 181 failed: rte_eth_dev_configure 
for port 7 failed
EAL: Test assert test_reconfigure_bonding_device line 4162 failed: 
failed to reconfigure bonding device
  + TestCase [63] : test_reconfigure_bonding_device failed
ETHDEV: Invalid port_id=7
  + TestCase [64] : test_close_bonding_device succeeded
ETHDEV: Invalid port_id=7
EAL: Test assert remove_members_and_stop_bonding_device line 659 failed: 
Failed to stop bonding port 7
  + ------------------------------------------------------- +
  + Test Suite Summary : Link Bonding Unit Test Suite
  + ------------------------------------------------------- +
  + Tests Total :       65
  + Tests Skipped :      0
  + Tests Executed :    65
  + Tests Unsupported:   0
  + Tests Passed :       5
  + Tests Failed :      60
  + ------------------------------------------------------- +
Test Failed


I guess the bonding use case needs to close all bonding devices.


/Huisong

在 2024/12/13 9:51, Stephen Hemminger 写道:
> The test had incorrect assumptions about how active backup
> should work. When in active backup mode, the secondary (not primary)
> ports should be ignored. The test was always broken since initially
> written but earlier bug was masking the part of the test which
> tested non-primary ports.
>
> Bugzilla ID: 1589
> Fixes: 112ce3917674 ("test/bonding: fix loop on members")
> Cc: stable at dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> ---
>   app/test/test_link_bonding.c | 68 +++++++++++++++++-------------------
>   1 file changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
> index b752a5ecbf..512e79365f 100644
> --- a/app/test/test_link_bonding.c
> +++ b/app/test/test_link_bonding.c
> @@ -2246,49 +2246,47 @@ test_activebackup_rx_burst(void)
>   		virtual_ethdev_add_mbufs_to_rx_queue(test_params->member_port_ids[i],
>   				&gen_pkt_burst[0], burst_size);
>   
> +		/* Expect burst if this was the active port, zero otherwise */
> +		unsigned int rx_expect = (test_params->member_port_ids[i] == primary_port) ? burst_size : 0;
> +
>   		/* Call rx burst on bonding device */
> -		TEST_ASSERT_EQUAL(rte_eth_rx_burst(test_params->bonding_port_id, 0,
> -				&rx_pkt_burst[0], MAX_PKT_BURST), burst_size,
> -				"rte_eth_rx_burst failed");
> +		unsigned int rx_count = rte_eth_rx_burst(test_params->bonding_port_id, 0,
> +							 &rx_pkt_burst[0], MAX_PKT_BURST);
> +		TEST_ASSERT_EQUAL(rx_count, rx_expect,
> +				  "rte_eth_rx_burst (%u) not as expected (%u)",
> +				  rx_count, rx_expect);
>   
> -		if (test_params->member_port_ids[i] == primary_port) {
> -			/* Verify bonding device rx count */
> -			rte_eth_stats_get(test_params->bonding_port_id, &port_stats);
> -			TEST_ASSERT_EQUAL(port_stats.ipackets, (uint64_t)burst_size,
> -					"Bonding Port (%d) ipackets value (%u) not as expected (%d)",
> +		/* Verify bonding device rx count */
> +		rte_eth_stats_get(test_params->bonding_port_id, &port_stats);
> +		TEST_ASSERT_EQUAL(port_stats.ipackets, rx_expect,
> +				  "Bonding Port (%d) ipackets value (%u) not as expected (%u)",
>   					test_params->bonding_port_id,
> -					(unsigned int)port_stats.ipackets, burst_size);
> +				  (unsigned int)port_stats.ipackets, rx_expect);
>   
> -			/* Verify bonding member devices rx count */
> -			for (j = 0; j < test_params->bonding_member_count; j++) {
> -				rte_eth_stats_get(test_params->member_port_ids[j], &port_stats);
> -				if (i == j) {
> -					TEST_ASSERT_EQUAL(port_stats.ipackets, (uint64_t)burst_size,
> -							"Member Port (%d) ipackets value (%u) not as "
> -							"expected (%d)",
> -							test_params->member_port_ids[i],
> -							(unsigned int)port_stats.ipackets,
> -							burst_size);
> -				} else {
> -					TEST_ASSERT_EQUAL(port_stats.ipackets, 0,
> -							"Member Port (%d) ipackets value (%u) not as "
> -							"expected (%d)\n",
> -							test_params->member_port_ids[i],
> -							(unsigned int)port_stats.ipackets, 0);
> -				}
> -			}
> -		} else {
> -			for (j = 0; j < test_params->bonding_member_count; j++) {
> -				rte_eth_stats_get(test_params->member_port_ids[j], &port_stats);
> +		for (j = 0; j < test_params->bonding_member_count; j++) {
> +			rte_eth_stats_get(test_params->member_port_ids[j], &port_stats);
> +			if (i == j) {
> +				TEST_ASSERT_EQUAL(port_stats.ipackets, rx_expect,
> +					  "Member Port (%d) ipackets (%u) not as expected (%d)",
> +					  test_params->member_port_ids[i],
> +					  (unsigned int)port_stats.ipackets, rx_expect);
> +
> +				/* reset member device stats */
> +				rte_eth_stats_reset(test_params->member_port_ids[j]);
> +			} else {
>   				TEST_ASSERT_EQUAL(port_stats.ipackets, 0,
> -						"Member Port (%d) ipackets value (%u) not as expected "
> -						"(%d)", test_params->member_port_ids[i],
> -						(unsigned int)port_stats.ipackets, 0);
> +					  "Member Port (%d) ipackets (%u) not as expected (%d)",
> +					  test_params->member_port_ids[i],
> +					  (unsigned int)port_stats.ipackets, 0);
>   			}
>   		}
>   
> -		/* free mbufs */
> -		rte_pktmbuf_free_bulk(rx_pkt_burst, burst_size);
> +		/* extract packets queued to inactive member */
> +		if (rx_count == 0)
> +			rx_count = rte_eth_rx_burst(test_params->member_port_ids[i], 0,
> +						    rx_pkt_burst, MAX_PKT_BURST);
> +		if (rx_count > 0)
> +			rte_pktmbuf_free_bulk(rx_pkt_burst, rx_count);
>   
>   		/* reset bonding device stats */
>   		rte_eth_stats_reset(test_params->bonding_port_id);