DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH] net/bonding: add add/remove mac addrs
@ 2018-06-06 11:52 Alex Kiselev
  2018-06-06 15:36 ` Chas Williams
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Kiselev @ 2018-06-06 11:52 UTC (permalink / raw)
  To: Chas Williams; +Cc: dev

Hi Chas.

Thank you for the review.

>This is fine but you are missing one bit.  If a slave is added after you can configured the MAC
>addresses, you will need to replay the configured MAC addresses into that slave.

Yeah, I've missed that part about adding a slave when a bond is already working.

It looks like that mac_address_slaves_update() is the right place
to configure additional MAC addresses when a slave is being added.
But there is one moment that's very unclear to me.
could you please take a look at my reasoning about it?

mac_address_slaves_update() behaves very straightforward when
the bond mode isn't BONDING_MODE_8023AD, it just sets MAC address,
so I guess adding rte_eth_dev_mac_addr_add() call would be enought.

But when mode is BONDING_MODE_8023AD, mac_address_slaves_update() calls
bond_mode_8023ad_mac_address_update(). first, it doesn't use
rte_eth_dev_default_mac_addr_set() and second, in BONDING_MODE_8023AD
mode promiscuous mode is enabled for every slave.

Those are two moments that are unclear to me.
I would say that there is no need to do any MAC manipulations
(either rte_eth_dev_default_mac_addr_set() or rte_eth_dev_mac_addr_add())
when mode is BONDING_MODE_8023AD since promiscuous mode is enabled. Am I right?


> This is fine but you are missing one bit.  If a slave is added after you can configured the MAC
> addresses, you will need to replay the configured MAC addresses into that slave.
>
> It's not clear to me what the contract is between bonding and DPDK as far as slave
> removal.  Just looking at the behavior with the default MAC address, I would guess
> that bonding also needs to remove extra MAC addresses it added during slave
> remove.  Bonding is a PMD but also an application.

On Fri, May 25, 2018 at 12:21 PM, Alex Kiselev <alex@therouter.net> wrote:

add functions to add/remove MAC addresses

Signed-off-by: Alex Kiselev <alex@therouter.net>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 76 +++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 02d94b1..835b4e3 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)

@@ -2219,7 +2220,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 +2906,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,7 +2987,9 @@ 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,
-       .filter_ctrl          = bond_filter_ctrl
+       .filter_ctrl          = bond_filter_ctrl,
+       .mac_addr_add         = bond_ethdev_mac_addr_add,
+       .mac_addr_remove      = bond_ethdev_mac_addr_remove
 };

 static int
@@ -2954,10 +3016,14 @@ 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;
        }
 
--
2.7.3





--
Alex 

^ permalink raw reply	[flat|nested] 4+ messages in thread
[parent not found: <20180525164451.B3EF22C72@dpdk.org>]
* [dpdk-dev] [PATCH] net/bonding: add add/remove mac addrs
@ 2018-05-25 16:21 Alex Kiselev
  0 siblings, 0 replies; 4+ messages in thread
From: Alex Kiselev @ 2018-05-25 16:21 UTC (permalink / raw)
  To: dev

add functions to add/remove MAC addresses

Signed-off-by: Alex Kiselev <alex@therouter.net>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 76 +++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 02d94b1..835b4e3 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)
 
@@ -2219,7 +2220,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 +2906,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,7 +2987,9 @@ 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,
-	.filter_ctrl          = bond_filter_ctrl
+	.filter_ctrl          = bond_filter_ctrl,
+	.mac_addr_add         = bond_ethdev_mac_addr_add,
+	.mac_addr_remove      = bond_ethdev_mac_addr_remove
 };
 
 static int
@@ -2954,10 +3016,14 @@ 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;
 	}
 
-- 
2.7.3

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

end of thread, other threads:[~2018-06-06 15:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-06 11:52 [dpdk-dev] [PATCH] net/bonding: add add/remove mac addrs Alex Kiselev
2018-06-06 15:36 ` Chas Williams
     [not found] <20180525164451.B3EF22C72@dpdk.org>
2018-06-04 13:23 ` Chas Williams
  -- strict thread matches above, loose matches on Subject: below --
2018-05-25 16:21 Alex Kiselev

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