Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Stephen Hemminger]

On Mon, 18 Jun 2018 15:27:16 +0300
Alex Kiselev <alex@therouter.net> wrote:

> +static const struct ether_addr null_mac_addr;
> +
> +/*
> + * Add additional MAC addresses to the slave
> + */
> +int
> +slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> +		uint16_t slave_port_id)
> +{
> +	int i, ret;
> +	struct ether_addr *mac_addr;
> +
> +	/* add additional MACs to the slave */
> +	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> +		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> +		if (is_same_ether_addr(mac_addr, &null_mac_addr))
> +			break;
> +
> +		ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
You need to unwind if adding MAC address to one of the slave devices
worked, and the second one did not.

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Chas Williams]

On Mon, Jun 18, 2018 at 2:58 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Mon, 18 Jun 2018 15:27:16 +0300
> Alex Kiselev <alex@therouter.net> wrote:
>
> > +static const struct ether_addr null_mac_addr;
> > +
> > +/*
> > + * Add additional MAC addresses to the slave
> > + */
> > +int
> > +slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> > +             uint16_t slave_port_id)
> > +{
> > +     int i, ret;
> > +     struct ether_addr *mac_addr;
> > +
> > +     /* add additional MACs to the slave */
> > +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> > +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> > +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
> > +                     break;
> > +
> > +             ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> You need to unwind if adding MAC address to one of the slave devices
> worked, and the second one did not.
>

Yes, probably.  But that doesn't help with the new slave problem.  If you
add a new slave
and it is unable to add all the MAC addresses, what then?  The only
reasonable thing might
be to put that interface into promiscuous mode.  At some point you need to
draw the line,
where is the PMD and where is the application?

Thankfully, people tend to enslave the same types of PMDs and the
capabilities are
generally similar.

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Matan Azrad]

Hi

From: Chas Williams
> On Mon, Jun 18, 2018 at 2:58 PM Stephen Hemminger <
> stephen@networkplumber.org> wrote:
> 
> > On Mon, 18 Jun 2018 15:27:16 +0300
> > Alex Kiselev <alex@therouter.net> wrote:
> >
> > > +static const struct ether_addr null_mac_addr;
> > > +
> > > +/*
> > > + * Add additional MAC addresses to the slave  */ int
> > > +slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> > > +             uint16_t slave_port_id) {
> > > +     int i, ret;
> > > +     struct ether_addr *mac_addr;
> > > +
> > > +     /* add additional MACs to the slave */
> > > +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> > > +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> > > +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
> > > +                     break;
> > > +
> > > +             ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > You need to unwind if adding MAC address to one of the slave devices
> > worked, and the second one did not.
> >
> 

Agree.

> Yes, probably.  But that doesn't help with the new slave problem.  If you add a
> new slave and it is unable to add all the MAC addresses, what then?  The only
> reasonable thing might be to put that interface into promiscuous mode.

I think that in this case the slave should be rejected.
The reason is that all the slaves should be able to receive\send the same traffic all the time.

>  At some point you need to draw the line, where is the PMD and where is the
> application?
> 
> Thankfully, people tend to enslave the same types of PMDs and the capabilities
> are generally similar.

So, I think we need to enforce it in the bonding PMD.


Please look at the bonding rte_flow design \ RSS conf (also in bonding documentation):

The main principle in the design is that all the time all the slaves able to get\send the same traffic:
So, a new slave which cannot apply all the current bonding flow rules will be rejected.
And a new rule which cannot be applied to all the current slaves will be rejected and deleted from all the slaves.
I think it should be the same for mac address adding and all the other configurations which affect a traffic receiving\sending.

Matan

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Alex Kiselev]

> From: Chas Williams
>> On Mon, Jun 18, 2018 at 2:58 PM Stephen Hemminger <
>> stephen@networkplumber.org> wrote:

>> > On Mon, 18 Jun 2018 15:27:16 +0300
>> > Alex Kiselev <alex@therouter.net> wrote:
>> >
>> > > +static const struct ether_addr null_mac_addr;
>> > > +
>> > > +/*
>> > > + * Add additional MAC addresses to the slave  */ int
>> > > +slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
>> > > +             uint16_t slave_port_id) {
>> > > +     int i, ret;
>> > > +     struct ether_addr *mac_addr;
>> > > +
>> > > +     /* add additional MACs to the slave */
>> > > +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
>> > > +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
>> > > +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
>> > > +                     break;
>> > > +
>> > > +             ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
>> > > +             if (ret < 0)
>> > > +                     return ret;
>> > > +     }
>> > > +
>> > > +     return 0;
>> > You need to unwind if adding MAC address to one of the slave devices
>> > worked, and the second one did not.
>> >
Done in patch v3.
Also, I've added the same rollback to bond_ethdev_mac_addr_add().


> Agree.

>> Yes, probably.  But that doesn't help with the new slave problem.  If you add a
>> new slave and it is unable to add all the MAC addresses, what then?  The only
>> reasonable thing might be to put that interface into promiscuous mode.

> I think that in this case the slave should be rejected.
> The reason is that all the slaves should be able to receive\send the same traffic all the time.

>>  At some point you need to draw the line, where is the PMD and where is the
>> application?

>> Thankfully, people tend to enslave the same types of PMDs and the capabilities
>> are generally similar.

> So, I think we need to enforce it in the bonding PMD.


> Please look at the bonding rte_flow design \ RSS conf (also in bonding documentation):

> The main principle in the design is that all the time all the
> slaves able to get\send the same traffic:
> So, a new slave which cannot apply all the current bonding flow rules will be rejected.
> And a new rule which cannot be applied to all the current slaves
> will be rejected and deleted from all the slaves.
> I think it should be the same for mac address adding and all the
> other configurations which affect a traffic receiving\sending.


-- 
Alex

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Stephen Hemminger]

On Mon, 18 Jun 2018 15:27:16 +0300
Alex Kiselev <alex@therouter.net> wrote:

> +/*
> + * Remove additional MAC addresses from the slave
> + */
> +int
> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> +		uint16_t slave_port_id)
> +{
> +	int i, ret;
> +	struct ether_addr *mac_addr;
> +
> +	/* add additional MACs to the slave */
> +	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> +		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> +		if (is_same_ether_addr(mac_addr, &null_mac_addr))
> +			break;
> +
> +		ret = rte_eth_dev_mac_addr_remove(slave_port_id, mac_addr);
> +		if (ret < 0)
> +			return ret;
> +	}

Not sure this is the best semantic if remove fails on one of many
slaves. Perhaps it should always remove it from all slaves.

Or maybe a first pass to see if the address exists, then
a no-fail removal pass.

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Chas Williams]

On Mon, Jun 18, 2018 at 3:00 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Mon, 18 Jun 2018 15:27:16 +0300
> Alex Kiselev <alex@therouter.net> wrote:
>
> > +/*
> > + * Remove additional MAC addresses from the slave
> > + */
> > +int
> > +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> > +             uint16_t slave_port_id)
> > +{
> > +     int i, ret;
> > +     struct ether_addr *mac_addr;
> > +
> > +     /* add additional MACs to the slave */
>

And this should say remove as well.


> > +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> > +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> > +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
> > +                     break;
> > +
> > +             ret = rte_eth_dev_mac_addr_remove(slave_port_id, mac_addr);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
>
> Not sure this is the best semantic if remove fails on one of many
> slaves. Perhaps it should always remove it from all slaves.
>
> Or maybe a first pass to see if the address exists, then
> a no-fail removal pass.
>

Re: [dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Alex Kiselev]

> On Mon, 18 Jun 2018 15:27:16 +0300
> Alex Kiselev <alex@therouter.net> wrote:

>> +/*
>> + * Remove additional MAC addresses from the slave
>> + */
>> +int
>> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
>> +             uint16_t slave_port_id)
>> +{
>> +     int i, ret;
>> +     struct ether_addr *mac_addr;
>> +
>> +     /* add additional MACs to the slave */
>> +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
>> +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
>> +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
>> +                     break;
>> +
>> +             ret = rte_eth_dev_mac_addr_remove(slave_port_id, mac_addr);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }

> Not sure this is the best semantic if remove fails on one of many
> slaves. Perhaps it should always remove it from all slaves.

> Or maybe a first pass to see if the address exists, then
> a no-fail removal pass.

I think to always remove is the right thing.
Done in the patch v3

-- 
Alex

[dpdk-dev] [PATCH v2] net/bonding: add add/remove mac addrs

[Alex Kiselev]

add functions to add/remove MAC addresses
Signed-off-by: Alex Kiselev <alex@therouter.net>

---
 drivers/net/bonding/rte_eth_bond_api.c     |   8 +-
 drivers/net/bonding/rte_eth_bond_pmd.c     | 123 ++++++++++++++++++++++++++++-
 drivers/net/bonding/rte_eth_bond_private.h |   8 ++
 3 files changed, 134 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index d558df8b9..43c148ba5 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -387,9 +387,12 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
 	/* Add slave details to bonded device */
 	slave_eth_dev->data->dev_flags |= RTE_ETH_DEV_BONDED_SLAVE;
 
-	/* Update all slave devices MACs*/
+	/* Update all slave devices MACs */
 	mac_address_slaves_update(bonded_eth_dev);
 
+	/* Add additional MAC addresses to the slave */
+	slave_add_mac_addresses(bonded_eth_dev, slave_port_id);
+
 	/* Register link status change callback with bonded device pointer as
 	 * argument*/
 	rte_eth_dev_callback_register(slave_port_id, RTE_ETH_EVENT_INTR_LSC,
@@ -491,6 +494,9 @@ __eth_bond_slave_remove_lock_free(uint16_t bonded_port_id,
 	rte_eth_dev_default_mac_addr_set(slave_port_id,
 			&(internals->slaves[slave_idx].persisted_mac_addr));
 
+	/* remove additional MAC addresses from the slave */
+	slave_remove_mac_addresses(bonded_eth_dev, slave_port_id);
+
 	/*
 	 * Remove bond device flows from slave device.
 	 * Note: don't restore flow isolate mode.
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 02d94b1b1..51fc4840f 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -25,6 +25,7 @@
 
 #define REORDER_PERIOD_MS 10
 #define DEFAULT_POLLING_INTERVAL_10_MS (10)
+#define BOND_MAX_MAC_ADDRS 16
 
 #define HASH_L4_PORTS(h) ((h)->src_port ^ (h)->dst_port)
 
@@ -1588,6 +1589,56 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 	return 0;
 }
 
+static const struct ether_addr null_mac_addr;
+
+/*
+ * Add additional MAC addresses to the slave
+ */
+int
+slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id)
+{
+	int i, ret;
+	struct ether_addr *mac_addr;
+
+	/* add additional MACs to the slave */
+	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
+		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
+		if (is_same_ether_addr(mac_addr, &null_mac_addr))
+			break;
+
+		ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Remove additional MAC addresses from the slave
+ */
+int
+slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id)
+{
+	int i, ret;
+	struct ether_addr *mac_addr;
+
+	/* add additional MACs to the slave */
+	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
+		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
+		if (is_same_ether_addr(mac_addr, &null_mac_addr))
+			break;
+
+		ret = rte_eth_dev_mac_addr_remove(slave_port_id, mac_addr);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 int
 mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 {
@@ -2219,7 +2270,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	uint16_t max_nb_rx_queues = UINT16_MAX;
 	uint16_t max_nb_tx_queues = UINT16_MAX;
 
-	dev_info->max_mac_addrs = 1;
+	dev_info->max_mac_addrs = BOND_MAX_MAC_ADDRS;
 
 	dev_info->max_rx_pktlen = internals->candidate_max_rx_pktlen ?
 			internals->candidate_max_rx_pktlen :
@@ -2905,6 +2956,65 @@ bond_filter_ctrl(struct rte_eth_dev *dev __rte_unused,
 	return -ENOTSUP;
 }
 
+static int
+bond_ethdev_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+				__rte_unused uint32_t index, uint32_t vmdq)
+{
+	struct rte_eth_dev *slave_eth_dev;
+	struct bond_dev_private *internals = dev->data->dev_private;
+	int ret, i;
+
+	rte_spinlock_lock(&internals->lock);
+
+	for (i = 0; i < internals->slave_count; i++) {
+		slave_eth_dev = &rte_eth_devices[internals->slaves[i].port_id];
+		if (*slave_eth_dev->dev_ops->mac_addr_add == NULL) {
+			ret = -ENOTSUP;
+			goto end;
+		}
+	}
+
+	for (i = 0; i < internals->slave_count; i++) {
+		ret = rte_eth_dev_mac_addr_add(internals->slaves[i].port_id,
+				mac_addr, vmdq);
+		if (ret < 0)
+			goto end;
+	}
+
+	ret = 0;
+end:
+	rte_spinlock_unlock(&internals->lock);
+	return ret;
+}
+
+static void
+bond_ethdev_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+{
+	struct rte_eth_dev *slave_eth_dev;
+	struct bond_dev_private *internals = dev->data->dev_private;
+	int ret, i;
+
+	rte_spinlock_lock(&internals->lock);
+
+	for (i = 0; i < internals->slave_count; i++) {
+		slave_eth_dev = &rte_eth_devices[internals->slaves[i].port_id];
+		if (*slave_eth_dev->dev_ops->mac_addr_remove == NULL)
+			goto end;
+	}
+
+	struct ether_addr *mac_addr = &dev->data->mac_addrs[index];
+
+	for (i = 0; i < internals->slave_count; i++) {
+		ret = rte_eth_dev_mac_addr_remove(internals->slaves[i].port_id,
+				mac_addr);
+		if (ret < 0)
+			goto end;
+	}
+
+end:
+	rte_spinlock_unlock(&internals->lock);
+}
+
 const struct eth_dev_ops default_dev_ops = {
 	.dev_start            = bond_ethdev_start,
 	.dev_stop             = bond_ethdev_stop,
@@ -2927,6 +3037,8 @@ const struct eth_dev_ops default_dev_ops = {
 	.rss_hash_conf_get    = bond_ethdev_rss_hash_conf_get,
 	.mtu_set              = bond_ethdev_mtu_set,
 	.mac_addr_set         = bond_ethdev_mac_address_set,
+	.mac_addr_add         = bond_ethdev_mac_addr_add,
+	.mac_addr_remove      = bond_ethdev_mac_addr_remove,
 	.filter_ctrl          = bond_filter_ctrl
 };
 
@@ -2954,10 +3066,13 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	eth_dev->data->nb_rx_queues = (uint16_t)1;
 	eth_dev->data->nb_tx_queues = (uint16_t)1;
 
-	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN, 0,
-			socket_id);
+	/* Allocate memory for storing MAC addresses */
+	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN *
+			BOND_MAX_MAC_ADDRS, 0, socket_id);
 	if (eth_dev->data->mac_addrs == NULL) {
-		RTE_BOND_LOG(ERR, "Unable to malloc mac_addrs");
+		RTE_BOND_LOG(ERR,
+			     "Failed to allocate %u bytes needed to store MAC addresses",
+			     ETHER_ADDR_LEN * BOND_MAX_MAC_ADDRS);
 		goto err;
 	}
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 65445b863..43e0e448d 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -230,6 +230,14 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr);
 int
 mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev);
 
+int
+slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id);
+
+int
+slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id);
+
 int
 bond_ethdev_mode_set(struct rte_eth_dev *eth_dev, int mode);
 
-- 
2.16.1.windows.1