[ovs-dev] [PATCH v2 2/2] netdev-dpdk: Add option to configure VF MAC address.

Gaëtan Rivet grive at u256.net
Wed Sep 16 07:51:24 UTC 2020


On 16/09/20 00:08 +0200, Ilya Maximets wrote:
> On 7/7/20 11:18 AM, Gaetan Rivet wrote:
> > In some cloud topologies, using DPDK VF representors in guest requires
> > configuring a VF before it is assigned to the guest.
> > 
> > A first basic option for such configuration is setting the VF MAC
> > address. Add a key 'dpdk-vf-mac' to the 'options' column of the Interface
> > table.
> > 
> > This option can be used as such:
> > 
> >    $ ovs-vsctl add-port br0 dpdk-rep0 -- set Interface dpdk-rep0 type=dpdk \
> >       options:dpdk-vf-mac=00:11:22:33:44:55
> > 
> > Signed-off-by: Gaetan Rivet <grive at u256.net>
> > Suggested-by: Ilya Maximets <i.maximets at ovn.org>
> > Acked-by: Eli Britstein <elibr at mellanox.com>
> > ---
> >  Documentation/topics/dpdk/phy.rst | 12 ++++
> >  NEWS                              |  2 +
> >  lib/netdev-dpdk.c                 | 93 +++++++++++++++++++++++++------
> >  vswitchd/vswitch.xml              |  6 ++
> >  4 files changed, 96 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/topics/dpdk/phy.rst b/Documentation/topics/dpdk/phy.rst
> > index 38e52c8de..44c2b7cd0 100644
> > --- a/Documentation/topics/dpdk/phy.rst
> > +++ b/Documentation/topics/dpdk/phy.rst
> > @@ -379,6 +379,18 @@ an eth device whose mac address is ``00:11:22:33:44:55``::
> >      $ ovs-vsctl add-port br0 dpdk-mac -- set Interface dpdk-mac type=dpdk \
> >         options:dpdk-devargs="class=eth,mac=00:11:22:33:44:55"
> >  
> > +Representor specific configuration
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +In some topologies, a VF must be configured before being assigned to a
> > +guest (VM) machine.  This configuration is done through VF-specific fields
> > +in the ``options`` column of the Interface table.
> > +
> > +- Configure the VF MAC address::
> > +
> > +    $ ovs-vsctl add-port br0 dpdk-rep0 -- set Interface dpdk-rep0 type=dpdk \
> > +       options:dpdk-vf-mac=00:11:22:33:44:55
> > +
> >  Jumbo Frames
> >  ------------
> >  
> > diff --git a/NEWS b/NEWS
> > index 0116b3ea0..e79122968 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -10,6 +10,8 @@ Post-v2.13.0
> >       * Deprecated DPDK pdump packet capture support removed.
> >       * Deprecated DPDK ring ports (dpdkr) are no longer supported.
> >       * Add hardware offload support for VLAN Push/Pop actions (experimental).
> > +     * New 'options:dpdk-vf-mac' field for DPDK interface of VF ports,
> > +       that allows configuring the MAC address of a VF representor.
> >     - Linux datapath:
> >       * Support for kernel versions up to 5.5.x.
> >     - AF_XDP:
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 6324ed96f..a493a45ab 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -1885,6 +1885,42 @@ dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
> >      }
> >  }
> >  
> > +static int
> > +netdev_dpdk_set_etheraddr__(struct netdev_dpdk *dev, const struct eth_addr mac)
> > +    OVS_REQUIRES(dev->mutex)
> > +{
> > +    int err = 0;
> > +
> > +    if (!eth_addr_equals(dev->hwaddr, mac)) {
> > +        if (dev->type == DPDK_DEV_ETH) {
> > +            struct rte_ether_addr ea;
> > +
> > +            memcpy(ea.addr_bytes, mac.ea, ETH_ADDR_LEN);
> > +            err = -rte_eth_dev_default_mac_addr_set(dev->port_id, &ea);
> > +        }
> > +        if (!err) {
> > +            dev->hwaddr = mac;
> > +            netdev_change_seq_changed(&dev->up);
> > +        } else {
> > +            VLOG_WARN("%s: Failed to set requested mac("ETH_ADDR_FMT"): %s",
> > +                      netdev_get_name(&dev->up), ETH_ADDR_ARGS(mac),
> > +                      rte_strerror(err));
> > +        }
> > +    }
> > +
> > +    return err;
> > +}
> > +
> > +static bool
> > +dpdk_port_is_representor(struct netdev_dpdk *dev)
> > +    OVS_REQUIRES(dev->mutex)
> > +{
> > +    struct rte_eth_dev_info dev_info;
> > +
> > +    rte_eth_dev_info_get(dev->port_id, &dev_info);
> > +    return (*dev_info.dev_flags) & RTE_ETH_DEV_REPRESENTOR;
> > +}
> > +
> >  static int
> >  netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> >                         char **errp)
> > @@ -1898,6 +1934,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> >          {RTE_FC_RX_PAUSE, RTE_FC_FULL    }
> >      };
> >      const char *new_devargs;
> > +    const char *vf_mac;
> >      int err = 0;
> >  
> >      ovs_mutex_lock(&dpdk_mutex);
> > @@ -1968,6 +2005,43 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> >          goto out;
> >      }
> >  
> > +    vf_mac = smap_get(args, "dpdk-vf-mac");
> 
> We need to report this config option in netdev_dpdk_get_config() function, so
> we need to store it somewhere in netdev-dpdk.   And since we will store it
> we could check if it actually changed and avoid parsing and configuration
> attempts if not.
> 

Thanks for the pointer, will check.

> > +    if (vf_mac) {
> 
> There is still a question what to do if 'dpdk-vf-mac' removed from the
> database.  In this case OVS will not do anything and will keep last
> configured MAC on this interface.  This is a bit contr-intuitive.

The only mitigations I see are incomplete and will still have
counter-intuitive edge-cases.  It is still my opinion that keeping it
simple will be easier to follow for users and maintain for OvS devs.

> Also, you're prohibiting setting of MAC address to all zeroes which might
> be the original MAC of the VF (not sure how exactly if works with mlx cards,
> but, IIRC, it works this way for some NICs, i.e. initial MACs for VFs are
> all zeroes). Might be not an issue though since libvirt tries to use differet
> MAC to clear devices that doesn't support all-zeroes as their MAC.

Indeed, MLX drivers will init VFs MAC to all zeroes as well.  Vendors
will have different behaviors regarding reverting back to it.  I thought
it would be better to be aligned on MAC for internal and DPDK ports, but
it makes it impossible to revert back to an original state if needed.

I will remove the check as if it's an issue it will be catched later by
the driver.  Let me know if you'd prefer to keep it as-is.

> 
> Anyway, this must be documented that OVS will leave this MAC configured
> on a VF in case of port removal or clearing the database entry.  Because
> this very likely will produce ussues for users of this feature.
> 

ack.

> > +        struct eth_addr mac;
> > +
> > +        err = EINVAL;
> > +
> > +        if (!dpdk_port_is_representor(dev)) {
> > +            VLOG_ERR_BUF(errp, "'%s' is trying to set the VF MAC '%s' "
> > +                         "but 'options:dpdk-vf-mac' is only supported for "
> > +                         "VF representors.",
> > +                         netdev_get_name(netdev), vf_mac);
> > +            goto out;
> > +        }
> > +        if (!eth_addr_from_string(vf_mac, &mac)) {
> > +            VLOG_ERR_BUF(errp, "interface '%s': cannot parse MAC '%s'",
> 
> Should it be 'VF MAC' insted of just 'MAC'?
> 

ack.

> > +                         netdev_get_name(netdev), vf_mac);
> > +            goto out;
> > +        }
> > +        if (eth_addr_is_multicast(mac)) {
> > +            VLOG_ERR_BUF(errp,
> > +                         "interface '%s': cannot set MAC to multicast address",
> > +                         netdev_get_name(netdev));
> > +            goto out;
> > +        }
> > +        if (eth_addr_is_zero(mac)) {
> > +            VLOG_ERR_BUF(errp,
> > +                         "interface '%s': cannot set MAC to all zero address",
> > +                         netdev_get_name(netdev));
> > +            goto out;
> > +        }
> > +
> > +        err = netdev_dpdk_set_etheraddr__(dev, mac);
> > +        if (err) {
> > +            goto out;
> > +        }
> > +    }
> > +
> >      lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false);
> >      if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
> >          dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
> > @@ -2916,25 +2990,10 @@ netdev_dpdk_set_etheraddr(struct netdev *netdev, const struct eth_addr mac)
> >      int err = 0;
> >  
> >      ovs_mutex_lock(&dev->mutex);
> > -    if (!eth_addr_equals(dev->hwaddr, mac)) {
> > -        if (dev->type == DPDK_DEV_ETH) {
> > -            struct rte_ether_addr ea;
> > -
> > -            memcpy(ea.addr_bytes, mac.ea, ETH_ADDR_LEN);
> > -            err = rte_eth_dev_default_mac_addr_set(dev->port_id, &ea);
> > -        }
> > -        if (!err) {
> > -            dev->hwaddr = mac;
> > -            netdev_change_seq_changed(netdev);
> > -        } else {
> > -            VLOG_WARN("%s: Failed to set requested mac("ETH_ADDR_FMT"): %s",
> > -                      netdev_get_name(netdev), ETH_ADDR_ARGS(mac),
> > -                      rte_strerror(-err));
> > -        }
> > -    }
> > +    err = netdev_dpdk_set_etheraddr__(dev, mac);
> >      ovs_mutex_unlock(&dev->mutex);
> >  
> > -    return -err;
> > +    return err;
> >  }
> >  
> >  static int
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index b6acb34ca..b8a991c23 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -3272,6 +3272,12 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
> >            descriptors will be used by default.
> >          </p>
> >        </column>
> > +
> > +      <column name="options" key="dpdk-vf-mac">
> > +        <p>Ethernet address to set for this VF interface.  If unset then the
> > +        default <ref table="Interface" column="mac"/> address is used.</p>
> 
> This part of the documentation is not correct as 'mac' column will not be used
> for non-internal interfaces and DPDK interfaces are never internal in current
> implementation.
> 

ack.

> > +        <p>This option may only be used with dpdk VF representors.</p>
> > +      </column>
> >      </group>
> >  
> >      <group title="EMC (Exact Match Cache) Configuration">
> > 
> 

-- 
Gaëtan


More information about the dev mailing list