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

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

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