Re: [PATCH v3 0/2] virtio: introduce STOP status bit

[Jason Wang <jasowang@redhat.com>]

On Thu, Nov 25, 2021 at 5:01 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Nov 25, 2021 at 8:38 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Nov 25, 2021 at 3:25 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Nov 25, 2021 at 4:05 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > > å 2021/11/24 äå12:19, Eugenio Perez Martin åé:
> > > > > On Tue, Nov 23, 2021 at 12:33 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > > >> On Thu, Nov 18, 2021 at 05:49:11PM +0100, Eugenio Perez Martin wrote:
> > > > >>> On Thu, Nov 18, 2021 at 3:45 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > > >>>> On Thu, Nov 11, 2021 at 07:58:10PM +0100, Eugenio PÃrez wrote:
> > > > >>>>> This patch introduces a new status bit STOP. This can be used by the
> > > > >>>>> driver to stop the device in order to safely fetch used descriptors
> > > > >>>>> status, making sure the device will not fetch new available ones.
> > > > >>>>>
> > > > >>>>> Its main use case is live migration, although it has other orthogonal
> > > > >>>>> use cases. It can be used to safely discard requests that have not been
> > > > >>>>> used: in other words, to rewind available descriptors.
> > > > >>>> This sounds non-trivial and would require more explanation.
> > > > >>>>
> > > > >>> You are right and it's not well explained here, I will try to develop
> > > > >>> it better for the next version. It's in the cover letter explaining
> > > > >>> one use case proposed by MST [1] and the answer by Jason [2].
> > > > >>>
> > > > >>> When the VQ is stopped, it is forced to flush all the descriptors (in
> > > > >>> this version) and the used index in the case of split. With that
> > > > >>> information, the driver can modify any available descriptor that has
> > > > >>> not been used by the device, and to rewind the virtqueue available
> > > > >>> index to an extent.
> > > > >>>
> > > > >>> If we add Jason's virtqueue state (first patch of previous version),
> > > > >>> which I need to recover in later versions, the driver can move freely
> > > > >>> available and used index prior to queue resetting.
> > > > >>>
> > > > >>> The (in comments) proposed solution for the device that cannot flush
> > > > >>> its descriptor timely is to provide it with a way to report in-flight
> > > > >>> descriptors. As a reference, vhost-user has done it before, but with a
> > > > >>> memory region shared by a file descriptor [3]. If we add something
> > > > >>> similar, the driver still knows what file descriptors is able to
> > > > >>> rewind.
> > > > >>>
> > > > >>> Does that explanation make the driver rewind use case more clear?
> > > > >> I understand the use case but it's not clear how the mechanism is
> > > > >> supposed to work. Let's continue discussing in the sub-thread where I
> > > > >> posted details.
> > > > >>
> > > > > Ok!
> > > > >
> > > > >>>>> Stopping the device in the live migration context is done by per-device
> > > > >>>>> operations in vhost backends, but the introduction of STOP as a basic
> > > > >>>>> virtio facility comes with advantages:
> > > > >>>>> * All the device virtio-specific state is summarized in a single entity,
> > > > >>>>>    making easier to reason about it.
> > > > >>>> This point isn't clear to me. I think it's saying that using STOP
> > > > >>>> somehow unifies things compared to the way that vhost devices are
> > > > >>>> stopped today. Given that vhost already syncs state back to the VMM's
> > > > >>>> emulated VIRTIO device, I'm not sure how STOP is different.
> > > > >>>>
> > > > >>> It also achieves that, but it's more related to the fact that the
> > > > >>> current way of getting the index through vhost net, user, ... is not
> > > > >>> reusable by others methods to expose the device to VMM.
> > > > >> ->vhost_get_vring_base() is a common interface across vhost-kernel,
> > > > >> vhost-user, etc and all VIRTIO Device Types. Is there a problem with it
> > > > >> that I'm missing?
> > > > >>
> > > > > It is usable by all VIRTIO Device types *exposed through vhost*, so it
> > > > > fails to address the case when we cannot use vhost to expose the
> > > > > device. It can happen with the cases you explained better than me in
> > > > > [1], or when exposing a vdpa device as a pure virtio device using
> > > > > virtio_vdpa.
> > > >
> > > >
> > > > I don't get this and may miss something. The stop is a basic facility
> > > > that is required for the building block of live migration. We need that
> > > > regardless what kind of software technology or framework is used.
> > > >
> > >
> > > Yes, those are just examples using vdpa, to expose the actual
> > > limitation that vhost protocol has. But there can be other examples
> > > with other frameworks for sure.
> > >
> > > And they are valid for both getting vq index and stop.
> > >
> > > > For the case of virtio_vdpa, it's not because it can't use STOP but
> > > > because we don't have a valid use case for that.
> > > >
> > >
> > > I had rewinding descriptors in mind actually.
> >
> > Yes, STOP could be one of the building blocks for this. But it
> > actually requires more. e.g the ability to let the driver to change
> > the last_avail_idx in the device.
>
> I think it can be done without changing last_avail_idx for the guest
> to be able to stop and rewind.

At least the driver needs to know the exact value of last_avail_idx in
this case.

>
> The guest must already know what buffers are available for its normal
> operation. In case of stop & reset, the driver knows that the next
> last_avail_idx will be 0, so it just needs to set the available
> descriptors it does *not* want to rewind in the virtqueue and change
> avail_idx accordingly.

I may miss something, but in this case the device may start processing
[0, avail_idx), is it safe?

>
> Then we have the opposite problem of the live migration: What do "in
> flight" descriptors mean here? For example, for block, is it valid to
> rewind a write descriptor that is in-flight?

I think if we don't care about rewind it would be much easier. (Anyhow
we know we will support setting/getting index states from driver).

Thanks

>
> > And it depends on if we had other
> > requirements during the rewind, e.g can we afford to stop the whole
> > device or just a specific virtqueue. If it's the latter, we had
> > another special example (rewind to 0), Ali is proposing per virtqueue
> > reset which could be used to death unused buffers safely from a
> > specific queue (then it's something unrelated to STOP itself).
> >
> > So I think STOP + virtqueue index state should be sufficient for
> > rewind if we can afford to stop the whole device (which might not be
> > the case for Ali's proposal).
> >
>
> Totally agree on this part.
>
> Thanks!
>
> > Thanks
> >
> > >
> > > Thanks!
> > >
> > > > Thanks
> > > >
> > >
> >
>