Re: [Qemu-devel] [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net

[Siwei Liu]

On Thu, Jun 21, 2018 at 11:14 AM, Michael S. Tsirkin <address@hidden> wrote:
> On Wed, Jun 13, 2018 at 01:40:59PM +0800, Jason Wang wrote:
>>
>>
>> On 2018年06月13日 12:24, Samudrala, Sridhar wrote:
>> > On 6/12/2018 7:38 PM, Jason Wang wrote:
>> > >
>> > >
>> > > On 2018年06月12日 19:54, Michael S. Tsirkin wrote:
>> > > > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote:
>> > > > >
>> > > > > On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
>> > > > > > I don't think this is sufficient.
>> > > > > >
>> > > > > > If both primary and standby devices are present, a
>> > > > > > legacy guest without
>> > > > > > support for the feature might see two devices with same mac and get
>> > > > > > confused.
>> > > > > >
>> > > > > > I think that we should only make primary visible after
>> > > > > > guest acked the
>> > > > > > backup feature bit.
>> > > > > I think we want exactly the reverse? E.g fail the
>> > > > > negotiation when guest
>> > > > > does not ack backup feature.
>> > > > >
>> > > > > Otherwise legacy guest won't even have the chance to see
>> > > > > primary device in
>> > > > > the guest.
>> > > > That's by design.
>> > >
>> > > So management needs to know the capability of guest to set the
>> > > backup feature. This looks a chicken or egg problem to me.
>> >
>> > I don't think so. If the tenant requests 'accelerated datapath feature',
>> > the management
>> > will set 'standby' feature bit on virtio-net interface and if the guest
>> > virtio-net driver
>> > supports this feature, then the tenant VM will get that capability via a
>> > hot-plugged
>> > primary device.
>>
>> Ok, I thought exactly the reverse because of the commit title is "enable
>> virtio_net to act as a standby for a passthru device". But re-read the
>> commit log content, I understand the case a little bit. Btw, VF is not
>> necessarily faster than virtio-net, especially consider virtio-net may have
>> a lot of queues.
>
> Don't do that then, right?

I don't understand. Where did the standby feature come to imply the
"accelerated datapath" thing? Isn't failover/standby a generic high
availblity term, rather than marry it to the concept of device model
specifics? Do we expect scsi to work exactly the same way with
"accelerated datapath"?

-Siwei


>
>> >
>> >
>> > >
>> > > >
>> > > > > > And on reset or when backup is cleared in some other way, unplug 
>> > > > > > the
>> > > > > > primary.
>> > > > > What if guest does not support hotplug?
>> > > > It shouldn't ack the backup feature then and it's a good point.
>> > > > We should both document this and check kernel config has
>> > > > hotplug support. Sridhar could you take a look pls?
>> > > >
>> > > > > > Something like the below will do the job:
>> > > > > >
>> > > > > > Primary device is added with a special "primary-failover" flag.
>> > > > > > A virtual machine is then initialized with just a standby virtio
>> > > > > > device. Primary is not yet added.
>> > > > > A question is how to do the matching? Qemu knows nothing about e.g 
>> > > > > mac
>> > > > > address of a pass-through device I believe?
>> > > > Supply a flag to the VFIO when it's added, this way QEMU will know.
>> > > >
>> > > > > > Later QEMU detects that guest driver device set DRIVER_OK.
>> > > > > > It then exposes the primary device to the guest, and triggers
>> > > > > > a device addition event (hot-plug event) for it.
>> > > > > Do you mean we won't have primary device in the initial qemu cli?
>> > > > No, that's not what I mean.
>> > > >
>> > > > I mean management will supply a flag to VFIO and then
>> > > >
>> > > >
>> > > > - VFIO defers exposing
>> > > > primary to guest until guest acks the feature bit.
>> > > > - When we see guest ack, initiate hotplug.
>> > > > - On reboot, hide it again.
>> > > > - On reset without reboot, request hot-unplug and on eject hide
>> > > > it again.
>> > >
>> > > This sounds much like a kind of bonding in qemu.
>> > >
>> > > >
>> > > > > > If QEMU detects guest driver removal, it initiates a
>> > > > > > hot-unplug sequence
>> > > > > > to remove the primary driver.  In particular, if QEMU detects guest
>> > > > > > re-initialization (e.g. by detecting guest reset) it
>> > > > > > immediately removes
>> > > > > > the primary device.
>> > > > > I believe guest failover module should handle this gracefully?
>> > > > It can't control enumeration order, if primary is enumerated before
>> > > > standby then guest will load its driver and it's too late
>> > > > when failover driver is loaded.
>> > >
>> > > Well, even if we can delay the visibility of primary until
>> > > DRIVER_OK, there still be a race I think? And it looks to me it's
>> > > still a bug of guest:
>> > >
>> > > E.g primary could be probed before failover_register() in guest.
>> > > Then we will miss the enslaving of primary forever.
>> >
>> > That is not an issue. Even if the primary is probed before failover
>> > driver, it will
>> > enslave the primary via the call to failover_existing_slave_register()
>> > as part of
>> > failover_register() routine.
>>
>> Aha I get it. So the enumeration order is not an issue.
>>
>> Consider primary may still be seen by guest kernel even if we delay its
>> visibility, I wonder whether we can control the lifecycle of primary through
>> driver but not qemu. This can simplify a lot of things.
>>
>> Thanks
>>
>> >
>> > >
>> > > Thanks
>> > >
>> > > >
>> > > > > Thanks
>> > > > >
>> > > > > > We can move some of this code to management as well,
>> > > > > > architecturally it
>> > > > > > does not make too much sense but it might be easier
>> > > > > > implementation-wise.
>> > > > > >
>> > > > > > HTH
>> > > > > >
>> > > > > > On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
>> > > > > > > Ping on this patch now that the kernel patches are
>> > > > > > > accepted into davem's net-next tree.
>> > > > > > > https://patchwork.ozlabs.org/cover/920005/
>> > > > > > >
>> > > > > > >
>> > > > > > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>> > > > > > > > This feature bit can be used by hypervisor to
>> > > > > > > > indicate virtio_net device to
>> > > > > > > > act as a standby for another device with the same MAC address.
>> > > > > > > >
>> > > > > > > > I tested this with a small change to the patch
>> > > > > > > > to mark the STANDBY feature 'true'
>> > > > > > > > by default as i am using libvirt to start the VMs.
>> > > > > > > > Is there a way to pass the newly added feature
>> > > > > > > > bit 'standby' to qemu via libvirt
>> > > > > > > > XML file?
>> > > > > > > >
>> > > > > > > > Signed-off-by: Sridhar Samudrala <address@hidden>
>> > > > > > > > ---
>> > > > > > > >     hw/net/virtio-net.c                         | 2 ++
>> > > > > > > >     include/standard-headers/linux/virtio_net.h | 3 +++
>> > > > > > > >     2 files changed, 5 insertions(+)
>> > > > > > > >
>> > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> > > > > > > > index 90502fca7c..38b3140670 100644
>> > > > > > > > --- a/hw/net/virtio-net.c
>> > > > > > > > +++ b/hw/net/virtio-net.c
>> > > > > > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] 
>> > > > > > > > = {
>> > > > > > > >                          true),
>> > > > > > > >         DEFINE_PROP_INT32("speed", VirtIONet,
>> > > > > > > > net_conf.speed, SPEED_UNKNOWN),
>> > > > > > > >         DEFINE_PROP_STRING("duplex", VirtIONet,
>> > > > > > > > net_conf.duplex_str),
>> > > > > > > > +    DEFINE_PROP_BIT64("standby", VirtIONet,
>> > > > > > > > host_features, VIRTIO_NET_F_STANDBY,
>> > > > > > > > +                      false),
>> > > > > > > >         DEFINE_PROP_END_OF_LIST(),
>> > > > > > > >     };
>> > > > > > > > diff --git
>> > > > > > > > a/include/standard-headers/linux/virtio_net.h
>> > > > > > > > b/include/standard-headers/linux/virtio_net.h
>> > > > > > > > index e9f255ea3f..01ec09684c 100644
>> > > > > > > > --- a/include/standard-headers/linux/virtio_net.h
>> > > > > > > > +++ b/include/standard-headers/linux/virtio_net.h
>> > > > > > > > @@ -57,6 +57,9 @@
>> > > > > > > >                          * Steering */
>> > > > > > > >     #define VIRTIO_NET_F_CTRL_MAC_ADDR 23    /* Set MAC 
>> > > > > > > > address */
>> > > > > > > > +#define VIRTIO_NET_F_STANDBY      62    /* Act
>> > > > > > > > as standby for another device
>> > > > > > > > +                                         * with the same MAC.
>> > > > > > > > +                                         */
>> > > > > > > >     #define VIRTIO_NET_F_SPEED_DUPLEX 63    /*
>> > > > > > > > Device set linkspeed and duplex */
>> > > > > > > >     #ifndef VIRTIO_NET_NO_LEGACY
>> > >
>> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: address@hidden
> For additional commands, e-mail: address@hidden
>