From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay-out1.mail.masterhost.ru (relay-out1.mail.masterhost.ru [83.222.12.11]) by dpdk.org (Postfix) with ESMTP id D491F2629 for ; Wed, 6 Jun 2018 13:52:13 +0200 (CEST) Received: from [37.139.80.50] (helo=nw) by relay1.mail.masterhost.ru with esmtpa envelope from authenticated with alex@therouter.net message id 1fQWz6-0007Se-PY; Wed, 06 Jun 2018 14:52:13 +0300 Date: Wed, 6 Jun 2018 14:52:12 +0300 From: Alex Kiselev Message-ID: <468732534.20180606145212@therouter.net> To: Chas Williams <3chas3@gmail.com> CC: dev@dpdk.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-KLMS-Rule-ID: 1 X-KLMS-Message-Action: clean X-KLMS-AntiSpam-Lua-Profiles: 125565 [Jun 06 2018] X-KLMS-AntiSpam-Version: 5.8.1.0 X-KLMS-AntiSpam-Envelope-From: alex@therouter.net X-KLMS-AntiSpam-Rate: 0 X-KLMS-AntiSpam-Status: not_detected X-KLMS-AntiSpam-Method: none X-KLMS-AntiSpam-Info: LuaCore: 138 138 8abb067d7b3403aea7e8bdef3a458f7937a402e9, {rep_avail}, DmarcAF: none X-MS-Exchange-Organization-SCL: -1 X-KLMS-AntiSpam-Interceptor-Info: scan successful X-KLMS-AntiPhishing: not scanned, disabled by settings X-KLMS-AntiVirus: Kaspersky Security for Linux Mail Server, version 8.0.2.16, not scanned, license restriction Subject: Re: [dpdk-dev] [PATCH] net/bonding: add add/remove mac addrs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jun 2018 11:52:14 -0000 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 wrote: add functions to add/remove MAC addresses Signed-off-by: Alex Kiselev --- 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