Re: [PATCH v4 2/2] virtio_net: Extend virtio to use VF datapath when available

Samudrala, Sridhar
2018-03-14T00:28:07Z

On 3/12/2018 3:44 PM, Siwei Liu wrote:
> Apologies, still some comments going. Please see inline.
>
> On Thu, Mar 1, 2018 at 12:08 PM, Sridhar Samudrala
> <sridhar.samudrala@intel.com> wrote:
>> This patch enables virtio_net to switch over to a VF datapath when a VF
>> netdev is present with the same MAC address. It allows live migration
>> of a VM with a direct attached VF without the need to setup a bond/team
>> between a VF and virtio net device in the guest.
>>
>> The hypervisor needs to enable only one datapath at any time so that
>> packets don't get looped back to the VM over the other datapath. When a VF
>> is plugged, the virtio datapath link state can be marked as down. The
>> hypervisor needs to unplug the VF device from the guest on the source host
>> and reset the MAC filter of the VF to initiate failover of datapath to
>> virtio before starting the migration. After the migration is completed,
>> the destination hypervisor sets the MAC filter on the VF and plugs it back
>> to the guest to switch over to VF datapath.
>>
>> When BACKUP feature is enabled, an additional netdev(bypass netdev) is
>> created that acts as a master device and tracks the state of the 2 lower
>> netdevs. The original virtio_net netdev is marked as 'backup' netdev and a
>> passthru device with the same MAC is registered as 'active' netdev.
>>
>> This patch is based on the discussion initiated by Jesse on this thread.
>> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> ---
>> drivers/net/virtio_net.c | 683 ++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 682 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index bcd13fe906ca..f2860d86c952 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -30,6 +30,8 @@
>> #include <linux/cpu.h>
>> #include <linux/average.h>
>> #include <linux/filter.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/pci.h>
>> #include <net/route.h>
>> #include <net/xdp.h>
>>
>> @@ -206,6 +208,9 @@ struct virtnet_info {
>> u32 speed;
>>
>> unsigned long guest_offloads;
>> +
>> + /* upper netdev created when BACKUP feature enabled */
>> + struct net_device *bypass_netdev;
>> };
>>
>> struct padded_vnet_hdr {
>> @@ -2236,6 +2241,22 @@ static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>> }
>> }
>>
>> +static int virtnet_get_phys_port_name(struct net_device *dev, char *buf,
>> + size_t len)
>> +{
>> + struct virtnet_info *vi = netdev_priv(dev);
>> + int ret;
>> +
>> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_BACKUP))
>> + return -EOPNOTSUPP;
>> +
>> + ret = snprintf(buf, len, "_bkup");
>> + if (ret >= len)
>> + return -EOPNOTSUPP;
>> +
>> + return 0;
>> +}
>> +
>> static const struct net_device_ops virtnet_netdev = {
>> .ndo_open = virtnet_open,
>> .ndo_stop = virtnet_close,
>> @@ -2253,6 +2274,7 @@ static const struct net_device_ops virtnet_netdev = {
>> .ndo_xdp_xmit = virtnet_xdp_xmit,
>> .ndo_xdp_flush = virtnet_xdp_flush,
>> .ndo_features_check = passthru_features_check,
>> + .ndo_get_phys_port_name = virtnet_get_phys_port_name,
>> };
>>
>> static void virtnet_config_changed_work(struct work_struct *work)
>> @@ -2647,6 +2669,653 @@ static int virtnet_validate(struct virtio_device *vdev)
>> return 0;
>> }
>>
>> +/* START of functions supporting VIRTIO_NET_F_BACKUP feature.
>> + * When BACKUP feature is enabled, an additional netdev(bypass netdev)
>> + * is created that acts as a master device and tracks the state of the
>> + * 2 lower netdevs. The original virtio_net netdev is registered as
>> + * 'backup' netdev and a passthru device with the same MAC is registered
>> + * as 'active' netdev.
>> + */
>> +
>> +/* bypass state maintained when BACKUP feature is enabled */
>> +struct virtnet_bypass_info {
>> + /* passthru netdev with same MAC */
>> + struct net_device __rcu *active_netdev;
>> +
>> + /* virtio_net netdev */
>> + struct net_device __rcu *backup_netdev;
>> +
>> + /* active netdev stats */
>> + struct rtnl_link_stats64 active_stats;
>> +
>> + /* backup netdev stats */
>> + struct rtnl_link_stats64 backup_stats;
>> +
>> + /* aggregated stats */
>> + struct rtnl_link_stats64 bypass_stats;
>> +
>> + /* spinlock while updating stats */
>> + spinlock_t stats_lock;
>> +};
>> +
>> +static void virtnet_bypass_child_open(struct net_device *dev,
>> + struct net_device *child_netdev)
>> +{
>> + int err = dev_open(child_netdev);
>> +
>> + if (err)
>> + netdev_warn(dev, "unable to open slave: %s: %d\n",
>> + child_netdev->name, err);
>> +}
> Why ignoring the error?? i.e. virtnet_bypass_child_open should have
> return value. I don't believe the caller is in a safe context to
> guarantee the dev_open always succeeds.

OK.  Will handle this in the next revision.

>
>> +
>> +static int virtnet_bypass_open(struct net_device *dev)
>> +{
>> + struct virtnet_bypass_info *vbi = netdev_priv(dev);
>> + struct net_device *child_netdev;
>> +
>> + netif_carrier_off(dev);
>> + netif_tx_wake_all_queues(dev);
>> +
>> + child_netdev = rtnl_dereference(vbi->active_netdev);
>> + if (child_netdev)
>> + virtnet_bypass_child_open(dev, child_netdev);
> Handle the error?
>
>> +
>> + child_netdev = rtnl_dereference(vbi->backup_netdev);
>> + if (child_netdev)
>> + virtnet_bypass_child_open(dev, child_netdev);
> Handle the error and unwind?

Sure.

<snip>

> +
> +static int virtnet_bypass_register_child(struct net_device *child_netdev)
> +{
> + struct virtnet_bypass_info *vbi;
> + struct net_device *dev;
> + bool backup;
> + int ret;
> +
> + if (child_netdev->addr_len != ETH_ALEN)
> + return NOTIFY_DONE;
> +
> + /* We will use the MAC address to locate the virtnet_bypass netdev
> + * to associate with the child netdev. If we don't find a matching
> + * bypass netdev, move on.
> + */
> + dev = get_virtnet_bypass_bymac(dev_net(child_netdev),
> + child_netdev->perm_addr);
> + if (!dev)
> + return NOTIFY_DONE;
> +
> + vbi = netdev_priv(dev);
> + backup = (child_netdev->dev.parent == dev->dev.parent);
> + if (backup ? rtnl_dereference(vbi->backup_netdev) :
> + rtnl_dereference(vbi->active_netdev)) {
> + netdev_info(dev,
> + "%s attempting to join bypass dev when %s already present\n",
> + child_netdev->name, backup ? "backup" : "active");
> + return NOTIFY_DONE;
> + }
> +
> + /* Avoid non pci devices as active netdev */
> + if (!backup && (!child_netdev->dev.parent ||
> + !dev_is_pci(child_netdev->dev.parent)))
> + return NOTIFY_DONE;
> +
> There's a problem here in terms of error (particularly on the active
> slave, e.g. VF), see below:
>
>> + ret = netdev_rx_handler_register(child_netdev,
>> + virtnet_bypass_handle_frame, dev);
>> + if (ret != 0) {
>> + netdev_err(child_netdev,
>> + "can not register bypass receive handler (err = %d)\n",
>> + ret);
>> + goto rx_handler_failed;
>> + }
>> +
>> + ret = netdev_upper_dev_link(child_netdev, dev, NULL);
>> + if (ret != 0) {
>> + netdev_err(child_netdev,
>> + "can not set master device %s (err = %d)\n",
>> + dev->name, ret);
>> + goto upper_link_failed;
>> + }
>> +
>> + child_netdev->flags |= IFF_SLAVE;
>> +
>> + if (netif_running(dev)) {
>> + ret = dev_open(child_netdev);
>> + if (ret && (ret != -EBUSY)) {
>> + netdev_err(dev, "Opening child %s failed ret:%d\n",
>> + child_netdev->name, ret);
>> + goto err_interface_up;
>> + }
>> + }
>> +
>> + /* Align MTU of child with master */
>> + ret = dev_set_mtu(child_netdev, dev->mtu);
>> + if (ret) {
>> + netdev_err(dev,
>> + "unable to change mtu of %s to %u register failed\n",
>> + child_netdev->name, dev->mtu);
>> + goto err_set_mtu;
>> + }
> If any of the above calls returns non-zero, the current code steps
> back to undo what's being done on that spefic slave previously. For
> instance, if the netdev_rx_handler_register returns non-zero because
> of busy rx handler, this register_child function would give up
> enslaving the VF and leave the upper virtio_bypass interface behind
> once it returns.

virtio_bypass interface is the upper master netdev and it is always present when
BACKUP feature is enabled.
If there is failure with enslaving VF, there will be 2 netdevs,
master virtio_bypass and slave virtio_net and the VM can be migrated.

> I am not sure if it's a good idea to leave the
> virtio_bypass around if running into failure: the guest is not
> migratable as the VF doesn't have a backup path,

Are you talking about a failure when registering backup netdev?  This should not
happen, but i guess we can improve error handling in such scenario.

> And perhaps the worse
> part is that, it now has two interfaces with identical MAC address but
> one of them is invalid (user cannot use the virtio interface as it has
> a dampened datapath). IMHO the virtio_bypass and its lower netdev
> should be destroyed at all when it fails to bind the VF, and
> technically, there should be some way to propogate the failure status
> to the hypervisor/backend, indicating that the VM is not migratable
> because of guest software errors (maybe by clearing out the backup
> feature from the guest virtio driver so host can see/learn it).

In BACKUP mode, user can only use the upper virtio_bypass netdev and that will
always be there. Any failure to enslave VF netdev is not fatal, but i will see
if we can improve the error handling of failure to enslave backup netdev.
Also, i don't think the BACKUP feature bit is negotiable with the host.

Thanks
Sridhar