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

[Chas Williams <3chas3@gmail.com>]

On Wed, Jun 6, 2018 at 7:52 AM, Alex Kiselev <alex@therouter.net> 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 <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
>
>