Re: [RFC PATCH for 8.0 10/13] virtio-net: Migrate vhost inflight descriptors

[Jason Wang]

On Tue, Jan 17, 2023 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jan 11, 2023 at 01:51:06PM +0800, Jason Wang wrote:
> > On Wed, Jan 11, 2023 at 12:40 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, January 10, 2023 11:35 PM
> > > >
> > > > On Tue, Jan 10, 2023 at 11:02 AM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > Hi Jason,
> > > > >
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Monday, December 5, 2022 10:25 PM
> > > > >
> > > > > >
> > > > > > A dumb question, any reason we need bother with virtio-net? It looks
> > > > > > to me it's not a must and would complicate migration compatibility.
> > > > >
> > > > > Virtio net vdpa device is processing the descriptors out of order.
> > > > > This vdpa device doesn’t offer IN_ORDER flag.
> > > > >
> > > > > And when a VQ is suspended it cannot complete these descriptors as 
> > > > > some
> > > > dummy zero length completions.
> > > > > The guest VM is flooded with [1].
> > > >
> > > > Yes, but any reason for the device to do out-of-order for RX?
> > > >
> > > For some devices it is more optimal to process them out of order.
> > > And its not limited to RX.
> >
> > TX should be fine, since the device can anyhow pretend to send all
> > packets, so we won't have any in-flight descriptors.
>
> And drop them all?

Depends on how many inflight descriptors. This is the worst case and
actually this is how software vhost backends did since it can't
validate whether or not the packet is sent to the wire. And it's not
worse than RX in which a lot of packets will be dropped for sure
during live migration.

> You end up with multisecond delays for things like
> DHCP.

Well, if DHCP is done during the live migration this is somehow
unavoidable, a lot of things needs to be recovered not only from the
migration downtime. E.g it may suffer from delay of gARP packet and
others.

> Yes theoretically packets can be dropped at any time, but
> practically people expect this to happen on busy systems, not randomly
> out of the blue.

The problem is that we never validate whether or not the packet is
sent for a software device. Various layers could be placed under the
vhost, so there's no guarantee that the packet won't be lost.

>
> > >
> > > > >
> > > > > So it is needed for the devices that doesn’t offer IN_ORDER feature.
> > > > >
> > > > > [1]
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > > > > e/drivers/net/virtio_net.c?h=v6.2-rc3#n1252
> > > >
> > > > It is only enabled in a debug kernel which should be harmless?
> > > it is KERN_DEBUG log level. Its is not debug kernel, just the debug log 
> > > level.
> >
> > Ok, but the production environment should not use that level anyhow.
>
> It's just one example.  And it's enough in my eyes to prove we really
> can't start sending zero length RX buffers to drivers and expect all to be
> well. If we want to we need to negotiate a new feature bit.

Ok.

>
>
> > > And regardless, generating zero length packets for debug kernel is even 
> > > more confusing.
> >
> > Note that it is allowed in the virtio-spec[1] (we probably can fix
> > that in the driver) and we have pr_debug() all over this drivers and
> > other places. It doesn't cause any side effects except for the
> > debugging purpose.
> >
> > So I think having inflight tracking is useful, but I'm not sure it's
> > worth bothering with virtio-net (or worth to bothering now):
> >
> > - zero length is allowed
> > - it only helps for debugging
> > - may cause issues for migration compatibility
> > - requires new infrastructure to be invented
> >
> > Thanks
> >
> > [1] spec said
> >
> > "
> > Note: len is particularly useful for drivers using untrusted buffers:
> > if a driver does not know exactly how much has been written by the
> > device, the driver would have to zero the buffer in advance to ensure
> > no data leakage occurs.
> > "
>
> I don't think this talks about zero length at all.
> Let me try to explain what this talk about in my opinion.
>
>
> There are cases where device does not know exactly how much
> data it wrote into buffer.

Actually, I think the inflight could be one case. Or do you have any
other case when the device doesn't know how much data it wrote?

> Should it over-estimate
> such that driver can be sure that buffer after the reported
> length is unchanged?

I can't think of a case when such over-estimation can help for any
logic. (Filling magic value into the buffer and deduce the actual
length that is written by the device?)

> Or should it instead under-estimate
> such that driver can be sure that the reported length has
> been initialized by device?
>
> What this text in the spec says is that it must always
> under-estimate and not over-estimate. And it attempts to
> explain why this is useful: imagine driver that trusts the
> device and wants to make sure buffer is initialized.
> With the definition in the spec, it only needs to initialize
> data after the reported length.

Just to make sure I understand, such initialization can happen only
after the buffer is completed by the device. But after that the buffer
doesn't belong to the device anymore so drivers are free to do any
initialization they want. Or anything makes this special?

Thanks

> Initialize how? It's up to the
> driver but for example it can zero this buffer.
>
>
> In short, all the text says is "do not over-report length,
> only set it to part of buffer you wrote".
>
> Besides that, the text itself is from the original spec and it did not
> age well:
>
> 1)- no one actually relies on this
>
> 2)- rather than untrusted "buffers" what we commonly have is untrusted
>   devices so length can't be trusted either
>
> 3)- writes on PCI are posted and if your security model
>   depends on buffer being initialized and you want to
>   recover from errors you really can't expect device to
>   give you this info. Luckily no one cares see 1) above.
>
>
> --
> MST
>