[dpdk-dev] 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(-)
  

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