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

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, January 11, 2023 12:51 AM
> 
> 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.
> 
> >
> > > >
> > > > 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.
> 
> > 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
This isn’t explicitly called out. It may be worth to add to the spec.

> - it only helps for debugging
> - may cause issues for migration compatibility
We have this missing for long time regardless of this feature. So let's not mix 
here.

The mlx5 vdpa device can do eventual in-order completion before/at time of 
suspend, so I think we can wait for now to until more advance hw arrives.

> - requires new infrastructure to be invented
> 
> Thanks
> 
> [1] spec said
> 
This doesn’t say that its zero-length completion. Len is a mandatory field to 
tell how many bytes device wrote.
> "
> 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.
> "