From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-it0-f67.google.com (mail-it0-f67.google.com [209.85.214.67]) by dpdk.org (Postfix) with ESMTP id 476EA1B70F for ; Wed, 6 Jun 2018 17:36:27 +0200 (CEST) Received: by mail-it0-f67.google.com with SMTP id v83-v6so8606067itc.3 for ; Wed, 06 Jun 2018 08:36:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=IyuGsfExufQcvnlAXZ5rh34tgfSmOCzQMMzFLrElFLY=; b=n2rxFkCya17yzMEcj+khQ2jBxvA3CnSxY5trZV3+lJknuACmWMMaXtjzjFuEegTjfJ ctWnus8FoPraUO8ialCZUTxr7L1pp3lzFr+HvSrt/EIzgfVyGblUopG41rQSneyIQqRA 7L/X/RUKlyf0BEzbw5UJS4nyZnpaaW6bKg3WpaDgv/f0OiA2NRGr08aAOMypRLyinCRx J998TnWKenZ51n+l6CqHyMuMDHFE+89qCag0sT0y74jJstOV/TGDvgTIQVx+At4TqbXi Vx17NpJQJEZY6c74kNKEPFRawSdx8LGxE+t8IQt2jovFQG42K3PjxEkvJySOI9sdJ8Yy uK6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=IyuGsfExufQcvnlAXZ5rh34tgfSmOCzQMMzFLrElFLY=; b=nkdQPClIIzIRNudb+cr26cCiN0guAO1FApL5ys4FNvFJlRYTt6hfHEhu9BWvz6oXCH mcH0NIdIY0aWFAKqt49xq9IElF9r9w1fWe41p11YM+HVgJuA2vkAX8IRetME3lfFg9G1 gPfhuYYEsS5IcTIzrJ07vNsroT+d14HAJHLB9UCDP8tUMEGxin/5xVwrrDLIa5sNE0jn qZzmdiAfW1o7I64A9vOUhRM/eaWRPBH8JZqRM0ubcwKNtE5dikEdSE5U9dKMCH7U6TYh XYQt96MgDIIohhPlETozTGnmi5OVsnnklMRttk0lEIA1ZltxAwm61TqNvwS1sky1nwkI lX9A== X-Gm-Message-State: APt69E25G/NqHw0zDkXjyw+f0bdlwSV0bc2Qv5PXLgmU7fcyrHyD6GA7 uw4fGSNeqw5viyC92YJ+4BtHcjihIeWEcI6ePtpaaA== X-Google-Smtp-Source: ADUXVKJHHbcW+UILyCSP+HEU7g0zlPLvHJUxFFnaDmd00QlModqKLvjgKKydu49cAZnlZCsVv1axChu7PRIiuFiZI9o= X-Received: by 2002:a24:350c:: with SMTP id k12-v6mr2958393ita.95.1528299386678; Wed, 06 Jun 2018 08:36:26 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:e90e:0:0:0:0:0 with HTTP; Wed, 6 Jun 2018 08:36:25 -0700 (PDT) In-Reply-To: <468732534.20180606145212@therouter.net> References: <468732534.20180606145212@therouter.net> From: Chas Williams <3chas3@gmail.com> Date: Wed, 6 Jun 2018 11:36:25 -0400 Message-ID: To: Alex Kiselev Cc: dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 15:36:27 -0000 On Wed, Jun 6, 2018 at 7:52 AM, Alex Kiselev wrote: > 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. > Yes, that seems correct. Don't forget the remove if a slave is removed. > 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. > Sorry this part is a bit hazy to me. I need to review the specification to see what's going on here exactly and why. Our local implementation does something a little different here. > 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? > One reason would be that the LAC PDUs arrive on the ETYPE_TYPE_SLOW multicast group and so we need a way to receive those packets. Since adding a multicast group address probably didn't exist when this was written, promiscuous was used. It would also be the fallback if the PMD didn't support adding MAC addresses. If we wanted to switch away from using promiscuous, bonding would need to use the same interface but offset those application MAC addresses away from the MAC addresses that bonding would need to add for basic operation. Doing this seems a bit tricky and unnecessary at this point. So just ignore 802.3ad for now. Just comment about it in the code in ..._slaves_update(). "802.3ad already uses promiscuous, so nothing to do here." > > 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 > >