[ovs-dev] [PATCH v2 2/2] netdev-dpdk: Add option to configure VF MAC address.
Ilya Maximets
i.maximets at ovn.org
Wed Sep 16 11:33:55 UTC 2020
On 9/16/20 9:51 AM, Gaëtan Rivet wrote:
> 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.
Yes, I'm not asking to fix that since it's actually unclear how to fix
correctly, I'm just asking to clearly document this behavior.
>
>> 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.
I think we should allow all-zero macs if HW/driver actually allowes it just
because it's not a usual interface and it's a special handling for kind of
"unallocated" vfs.
>
>>
>> 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">
>>>
>>
>
More information about the dev
mailing list