DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] test/bonding: fix active backup rx test
@ 2024-12-13  1:51 Stephen Hemminger
  2024-12-13  6:57 ` lihuisong (C)
  2024-12-13 17:17 ` [PATCH v2] " Stephen Hemminger
  0 siblings, 2 replies; 6+ messages in thread
From: Stephen Hemminger @ 2024-12-13  1:51 UTC (permalink / raw)
  To: dev
  Cc: Stephen Hemminger, stable, Chas Williams, Min Hu (Connor),
	Chengwen Feng, Bruce Richardson

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@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@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);
-- 
2.45.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test/bonding: fix active backup rx test
  2024-12-13  1:51 [PATCH] test/bonding: fix active backup rx test Stephen Hemminger
@ 2024-12-13  6:57 ` lihuisong (C)
  2024-12-13 16:56   ` Stephen Hemminger
  2024-12-13 17:17 ` [PATCH v2] " Stephen Hemminger
  1 sibling, 1 reply; 6+ messages in thread
From: lihuisong (C) @ 2024-12-13  6:57 UTC (permalink / raw)
  To: Stephen Hemminger, dev
  Cc: stable, Chas Williams, Min Hu (Connor), Chengwen Feng, Bruce Richardson

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@dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen@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);

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test/bonding: fix active backup rx test
  2024-12-13  6:57 ` lihuisong (C)
@ 2024-12-13 16:56   ` Stephen Hemminger
  2024-12-16  1:50     ` lihuisong (C)
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2024-12-13 16:56 UTC (permalink / raw)
  To: lihuisong (C)
  Cc: dev, stable, Chas Williams, Min Hu (Connor),
	Chengwen Feng, Bruce Richardson

On Fri, 13 Dec 2024 14:57:23 +0800
"lihuisong (C)" <lihuisong@huawei.com> wrote:

> 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.


The test has lots of issues that would make it bad to run twice.
Like leaking mbufs, etc.
But these always existed and will leave that to bonding maintainers to fix.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] test/bonding: fix active backup rx test
  2024-12-13  1:51 [PATCH] test/bonding: fix active backup rx test Stephen Hemminger
  2024-12-13  6:57 ` lihuisong (C)
@ 2024-12-13 17:17 ` Stephen Hemminger
  2024-12-16  1:53   ` lihuisong (C)
  1 sibling, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2024-12-13 17:17 UTC (permalink / raw)
  To: dev
  Cc: Stephen Hemminger, stable, Chas Williams, Min Hu (Connor),
	Bruce Richardson, Chengwen Feng

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@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - fix checkpatch warning from long line

 app/test/test_link_bonding.c | 69 ++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index b752a5ecbf..19b064771a 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -2246,49 +2246,48 @@ 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);
-- 
2.45.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] test/bonding: fix active backup rx test
  2024-12-13 16:56   ` Stephen Hemminger
@ 2024-12-16  1:50     ` lihuisong (C)
  0 siblings, 0 replies; 6+ messages in thread
From: lihuisong (C) @ 2024-12-16  1:50 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, stable, Chas Williams, Min Hu (Connor),
	Chengwen Feng, Bruce Richardson


在 2024/12/14 0:56, Stephen Hemminger 写道:
> On Fri, 13 Dec 2024 14:57:23 +0800
> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>
>> 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.
>
> The test has lots of issues that would make it bad to run twice.
> Like leaking mbufs, etc.
> But these always existed and will leave that to bonding maintainers to fix.
ok, these issues aren't related to this patch and can be be fixed later.
> .

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] test/bonding: fix active backup rx test
  2024-12-13 17:17 ` [PATCH v2] " Stephen Hemminger
@ 2024-12-16  1:53   ` lihuisong (C)
  0 siblings, 0 replies; 6+ messages in thread
From: lihuisong (C) @ 2024-12-16  1:53 UTC (permalink / raw)
  To: Stephen Hemminger, dev
  Cc: stable, Chas Williams, Min Hu (Connor), Bruce Richardson, Chengwen Feng

LGTM
Acked-by: Huisong Li <lihuisong@huawei.com>
Tested-by: Huisong Li <lihuisong@huawei.com>
在 2024/12/14 1:17, 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@dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - fix checkpatch warning from long line
>
>   app/test/test_link_bonding.c | 69 ++++++++++++++++++------------------
>   1 file changed, 34 insertions(+), 35 deletions(-)
>
> diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
> index b752a5ecbf..19b064771a 100644
> --- a/app/test/test_link_bonding.c
> +++ b/app/test/test_link_bonding.c
> @@ -2246,49 +2246,48 @@ 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);

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-12-16  1:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-13  1:51 [PATCH] test/bonding: fix active backup rx test Stephen Hemminger
2024-12-13  6:57 ` lihuisong (C)
2024-12-13 16:56   ` Stephen Hemminger
2024-12-16  1:50     ` lihuisong (C)
2024-12-13 17:17 ` [PATCH v2] " Stephen Hemminger
2024-12-16  1:53   ` lihuisong (C)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).