Re: [dpdk-dev] [PATCH] bond: fix for mac assignment to slaves device

["Jiajia, SunX" <sunx.jiajia@intel.com>]

Tested-by: Jiajia, SunX <sunx.jiajia@intel.com>
- Tested Commit: 29d03f7aa33edc3292bf75730ec684dd4cbe5054
- OS: Fedora20 3.11.10-301.fc20.x86_64 
- GCC: gcc version 4.8.3
- CPU: Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
- NIC: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection [8086:10fb]
- Default x86_64-native-linuxapp-gcc configuration
- Total 1 cases, 1 passed, 0 failed

TOPO:
* Connections ports between tester/ixia and DUT
  - TESTER(Or IXIA)-------DUT
  - portA------------------port0
  - portB------------------port1
  - portC------------------port2
  - portD------------------port3

Test Setup#1 for Functional test
================================

Tester has 4 ports(portA--portD), and DUT has 4 ports(port0--port3), 
then connect portA to port0, portB to port1, portC to port2, portD to port3.

- Case: Basic bonding--MAC Address Test
  Description: 
		Use Setup#1.
		Create bonded device and add some ports as slaves of bonded device,
            Check that the changes of  the bonded device and slave MAC
  Expected test result:
            Verify the behavior of bonded device and slave according to the mode.



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Declan Doherty
> Sent: Saturday, December 06, 2014 1:35 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] bond: fix for mac assignment to slaves
> device
> 
> Adding call to mac_address_slaves_update from the lsc handler when the
> first slave become active to propagate any mac changes made while
> devices are inactive
> 
> Changed removing slave logic to use memmove instead of memcpy to move
> data within the same array, as this was corrupting the slave array.
> 
> Adding unit test to cover failing assignment scenarios
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  app/test/test_link_bonding.c           | 192
> ++++++++++++++++++++++++++++++++-
>  app/test/virtual_pmd.c                 |   1 -
>  lib/librte_pmd_bond/rte_eth_bond_pmd.c |  14 ++-
>  3 files changed, 199 insertions(+), 8 deletions(-)
> 
> diff --git a/app/test/test_link_bonding.c
> b/app/test/test_link_bonding.c
> index f62c490..4523de6 100644
> --- a/app/test/test_link_bonding.c
> +++ b/app/test/test_link_bonding.c
> @@ -454,7 +454,7 @@ test_remove_slave_from_bonded_device(void)
> 
>  	mac_addr = (struct ether_addr *)slave_mac;
>  	mac_addr->addr_bytes[ETHER_ADDR_LEN-1] =
> -			test_params->slave_port_ids[test_params-
> >bonded_slave_count-1];
> +			test_params->bonded_slave_count-1;
> 
>  	rte_eth_macaddr_get(
>  			test_params->slave_port_ids[test_params-
> >bonded_slave_count-1],
> @@ -810,8 +810,7 @@ test_set_primary_slave(void)
>  				test_params->bonded_port_id);
> 
>  		expected_mac_addr = (struct ether_addr *)&slave_mac;
> -		expected_mac_addr->addr_bytes[ETHER_ADDR_LEN-1] =
> -				test_params->slave_port_ids[i];
> +		expected_mac_addr->addr_bytes[ETHER_ADDR_LEN-1] = i;
> 
>  		/* Check primary slave MAC */
>  		rte_eth_macaddr_get(test_params->slave_port_ids[i],
> &read_mac_addr);
> @@ -928,6 +927,192 @@ test_set_explicit_bonded_mac(void)
>  	return 0;
>  }
> 
> +#define BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT (3)
> +
> +static int
> +test_set_bonded_port_initialization_mac_assignment(void)
> +{
> +	int i, slave_count, bonded_port_id;
> +
> +	uint8_t slaves[RTE_MAX_ETHPORTS];
> +	int slave_port_ids[BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT];
> +
> +	struct ether_addr slave_mac_addr, bonded_mac_addr, read_mac_addr;
> +
> +	/* Initialize default values for MAC addresses */
> +	memcpy(&slave_mac_addr, slave_mac, sizeof(struct ether_addr));
> +	memcpy(&bonded_mac_addr, slave_mac, sizeof(struct ether_addr));
> +
> +	/*
> +	 * 1. a - Create / configure  bonded / slave ethdevs
> +	 */
> +	bonded_port_id = rte_eth_bond_create("ethdev_bond_mac_ass_test",
> +			BONDING_MODE_ACTIVE_BACKUP, rte_socket_id());
> +	TEST_ASSERT(bonded_port_id > 0, "failed to create bonded device");
> +
> +	TEST_ASSERT_SUCCESS(configure_ethdev(bonded_port_id, 0, 0),
> +				"Failed to configure bonded ethdev");
> +
> +	for (i = 0; i < BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT; i++) {
> +		char pmd_name[RTE_ETH_NAME_MAX_LEN];
> +
> +		slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = i + 100;
> +
> +		snprintf(pmd_name, RTE_ETH_NAME_MAX_LEN, "eth_slave_%d", i);
> +
> +		slave_port_ids[i] = virtual_ethdev_create(pmd_name,
> +				&slave_mac_addr, rte_socket_id(), 1);
> +
> +		TEST_ASSERT(slave_port_ids[i] >= 0,
> +				"Failed to create slave ethdev %s", pmd_name);
> +
> +		TEST_ASSERT_SUCCESS(configure_ethdev(slave_port_ids[i], 1,
> 0),
> +				"Failed to configure virtual ethdev %s",
> +				pmd_name);
> +	}
> +
> +
> +	/*
> +	 * 2. Add slave ethdevs to bonded device
> +	 */
> +	for (i = 0; i < BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT; i++) {
> +		TEST_ASSERT_SUCCESS(rte_eth_bond_slave_add(bonded_port_id,
> +				slave_port_ids[i]),
> +				"Failed to add slave (%d) to bonded port (%d).",
> +				slave_port_ids[i], bonded_port_id);
> +	}
> +
> +	slave_count = rte_eth_bond_slaves_get(bonded_port_id, slaves,
> +			RTE_MAX_ETHPORTS);
> +	TEST_ASSERT_EQUAL(BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT,
> slave_count,
> +			"Number of slaves (%d) is not as expected (%d)",
> +			slave_count, BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT);
> +
> +
> +	/*
> +	 * 3. Set explicit MAC address on bonded ethdev
> +	 */
> +	bonded_mac_addr.addr_bytes[ETHER_ADDR_LEN-2] = 0xFF;
> +	bonded_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 0xAA;
> +
> +	TEST_ASSERT_SUCCESS(rte_eth_bond_mac_address_set(
> +			bonded_port_id, &bonded_mac_addr),
> +			"Failed to set MAC address on bonded port (%d)",
> +			bonded_port_id);
> +
> +
> +	/* 4. a - Start bonded ethdev
> +	 *    b - Enable slave devices
> +	 *    c - Verify bonded/slaves ethdev MAC addresses
> +	 */
> +	TEST_ASSERT_SUCCESS(rte_eth_dev_start(bonded_port_id),
> +			"Failed to start bonded pmd eth device %d.",
> +			bonded_port_id);
> +
> +	for (i = 0; i < BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT; i++) {
> +		virtual_ethdev_simulate_link_status_interrupt(
> +				slave_port_ids[i], 1);
> +	}
> +
> +	rte_eth_macaddr_get(bonded_port_id, &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&bonded_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"bonded port mac address not as expected");
> +
> +	rte_eth_macaddr_get(slave_port_ids[0], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&bonded_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 0 mac address not as expected");
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 1 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[1], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 1 mac address not as expected");
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 2 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[2], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 2 mac address not as expected");
> +
> +
> +	/* 7. a - Change primary port
> +	 *    b - Stop / Start bonded port
> +	 *    d - Verify slave ethdev MAC addresses
> +	 */
> +	TEST_ASSERT_SUCCESS(rte_eth_bond_primary_set(bonded_port_id,
> +			slave_port_ids[2]),
> +			"failed to set primary port on bonded device.");
> +
> +	rte_eth_dev_stop(bonded_port_id);
> +	TEST_ASSERT_SUCCESS(rte_eth_dev_start(bonded_port_id),
> +				"Failed to start bonded pmd eth device %d.",
> +				bonded_port_id);
> +
> +	rte_eth_macaddr_get(bonded_port_id, &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&bonded_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"bonded port mac address not as expected");
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 0 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[0], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 0 mac address not as expected");
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 1 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[1], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 1 mac address not as expected");
> +
> +	rte_eth_macaddr_get(slave_port_ids[2], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&bonded_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 2 mac address not as expected");
> +
> +	/* 6. a - Stop bonded ethdev
> +	 *    b - remove slave ethdevs
> +	 *    c - Verify slave ethdevs MACs are restored
> +	 */
> +	rte_eth_dev_stop(bonded_port_id);
> +
> +	for (i = 0; i < BONDED_INIT_MAC_ASSIGNMENT_SLAVE_COUNT; i++) {
> +
> 	TEST_ASSERT_SUCCESS(rte_eth_bond_slave_remove(bonded_port_id,
> +				slave_port_ids[i]),
> +				"Failed to remove slave %d from bonded port
> (%d).",
> +				slave_port_ids[i], bonded_port_id);
> +	}
> +
> +	slave_count = rte_eth_bond_slaves_get(bonded_port_id, slaves,
> +			RTE_MAX_ETHPORTS);
> +
> +	TEST_ASSERT_EQUAL(slave_count, 0,
> +			"Number of slaves (%d) is great than expected (%d).",
> +			slave_count, 0);
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 0 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[0], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 0 mac address not as expected");
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 1 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[1], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 1 mac address not as expected");
> +
> +	slave_mac_addr.addr_bytes[ETHER_ADDR_LEN-1] = 2 + 100;
> +	rte_eth_macaddr_get(slave_port_ids[2], &read_mac_addr);
> +	TEST_ASSERT_SUCCESS(memcmp(&slave_mac_addr, &read_mac_addr,
> +			sizeof(read_mac_addr)),
> +			"slave port 2 mac address not as expected");
> +
> +	return 0;
> +}
> +
> 
>  static int
>  initialize_bonded_device_with_slaves(uint8_t bonding_mode, uint8_t
> bond_en_isr,
> @@ -4368,6 +4553,7 @@ static struct unit_test_suite
> link_bonding_test_suite  = {
>  		TEST_CASE(test_set_bonding_mode),
>  		TEST_CASE(test_set_primary_slave),
>  		TEST_CASE(test_set_explicit_bonded_mac),
> +
> 	TEST_CASE(test_set_bonded_port_initialization_mac_assignment),
>  		TEST_CASE(test_status_interrupt),
>  		TEST_CASE(test_adding_slave_after_bonded_device_started),
>  		TEST_CASE(test_roundrobin_tx_burst),
> diff --git a/app/test/virtual_pmd.c b/app/test/virtual_pmd.c
> index ade6cb0..9fac95d 100644
> --- a/app/test/virtual_pmd.c
> +++ b/app/test/virtual_pmd.c
> @@ -588,7 +588,6 @@ virtual_ethdev_create(const char *name, struct
> ether_addr *mac_addr,
> 
>  	memcpy(eth_dev->data->mac_addrs, mac_addr,
>  			sizeof(*eth_dev->data->mac_addrs));
> -	eth_dev->data->mac_addrs->addr_bytes[5] = eth_dev->data->port_id;
> 
>  	eth_dev->data->dev_started = 0;
>  	eth_dev->data->promiscuous = 0;
> diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> index 539baa4..1f2bd03 100644
> --- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> +++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> @@ -985,12 +985,16 @@ slave_remove(struct bond_dev_private *internals,
>  	int i, found = 0;
> 
>  	for (i = 0; i < internals->slave_count; i++) {
> -		if (internals->slaves[i].port_id ==	slave_eth_dev->data-
> >port_id)
> +		if (internals->slaves[i].port_id ==
> +				slave_eth_dev->data->port_id)
>  			found = 1;
> 
> -		if (found && i < (internals->slave_count - 1))
> -			memcpy(&internals->slaves[i], &internals->slaves[i+1],
> -					sizeof(internals->slaves[i]));
> +		if (found && (i < (internals->slave_count - 1))) {
> +			memmove(&internals->slaves[i], &internals-
> >slaves[i+1],
> +					(sizeof(internals->slaves[0]) *
> +					internals->slave_count - i - 1));
> +			break;
> +		}
>  	}
> 
>  	internals->slave_count--;
> @@ -1501,6 +1505,8 @@ bond_ethdev_lsc_event_callback(uint8_t port_id,
> enum rte_eth_event_type type,
>  			internals->current_primary_port = port_id;
>  			lsc_flag = 1;
> 
> +			mac_address_slaves_update(bonded_eth_dev);
> +
>  			/* Inherit eth dev link properties from first active
> slave */
>  			link_properties_set(bonded_eth_dev,
>  					&(slave_eth_dev->data->dev_link));
> --
> 1.7.12.2