[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