[virtio-dev] Re: [virtio] Re: [PATCH v10 04/10] admin: introduce virtio admin virtqueues

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6006 bytes --]

On Fri, Mar 03, 2023 at 08:37:31AM -0500, Michael S. Tsirkin wrote:
> On Fri, Mar 03, 2023 at 08:28:40AM -0500, Stefan Hajnoczi wrote:
> > On Thu, Mar 02, 2023 at 07:05:14PM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Mar 02, 2023 at 03:40:07PM -0500, Stefan Hajnoczi wrote:
> > > > On Thu, Mar 02, 2023 at 08:05:06AM -0500, Michael S. Tsirkin wrote:
> > > > > The admin virtqueues will be the first interface to issue admin commands.
> > > > > 
> > > > > Currently virtio specification defines control virtqueue to manipulate
> > > > > features and configuration of the device it operates on. However,
> > > > > control virtqueue commands are device type specific, which makes it very
> > > > > difficult to extend for device agnostic commands.
> > > > > 
> > > > > To support this requirement in a more generic way, this patch introduces
> > > > > a new admin virtqueue interface.
> > > > 
> > > > Is this referring to the existing virtio-net, virtio-scsi, etc control
> > > > virtqueues?
> > > > 
> > > > I see the admin virtqueue as the virtqueue equivalent to transport
> > > > feature bits. The admin queue does nothing device type-specific (net,
> > > > scsi, etc) and instead focusses on transport and core virtio tasks.
> > > > 
> > > > Keeping the device-specific virtqueue separate from the admin virtqueue
> > > > is simpler and has fewer potential problems. I don't think creating
> > > > common infrastructure for device-specific control virtqueues across
> > > > device types worthwhile or within the scope of this patch series.
> > > 
> > > yes this commit log is outdated. referred to original
> > > proposal by Max which also planned to replace cvq.
> > > will update.
> > > 
> > > 
> > > > > We also support more than one admin virtqueue, for QoS and
> > > > > scalability requirements.
> > > > > 
> > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  admin.tex   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  content.tex |  7 +++--
> > > > >  2 files changed, 79 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/admin.tex b/admin.tex
> > > > > index 7e28b77..3ffac2e 100644
> > > > > --- a/admin.tex
> > > > > +++ b/admin.tex
> > > > > @@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > > > >  \field{command_specific_data} and \field{command_specific_result}
> > > > >  depends on these structures and is described separately or is
> > > > >  implicit in the structure description.
> > > > > +
> > > > > +\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
> > > > > +
> > > > > +An administration virtqueue of an owner device is used to submit
> > > > > +group administration commands. An owner device can have more
> > > > > +than one administration virtqueue.
> > > > > +
> > > > > +If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
> > > > > +or more adminstration virtqueues. The number and locations of the
> > > > > +administration virtqueues are exposed by the owner device in a transport
> > > > > +specific manner.
> > > > > +
> > > > > +The driver submits commands by queueing them to an arbitrary
> > > > > +administration virtqueue, and they are processed by the
> > > > > +device in the order in which they are queued. It is the
> > > > > +responsibility of the driver to ensure ordering for commands
> > > > > +placed on different administration virtqueues, because they will
> > > > > +be executed with no order constraints.
> > > > 
> > > > Does "they are processed by the device in the order in which they are
> > > > queued" mean only 1 admin command can be running at any given time and
> > > > therefore they will complete in order? This would allow pipelining
> > > > dependent commands but prevent long-running commands because the
> > > > virtqueue is blocked while executing a command.
> > > 
> > > we have multiple vqs for that.
> > 
> > This reminds me of the async challenges with QEMU's QMP monitor.
> > 
> > Will it ever be possible to abort an in-flight command? I guess the
> > abort command would need to be submitted on another virtqueue, but how
> > do you identify the in-flight command that you wish to cancel?
> > 
> > Please clarify the blocking semantics in the spec because it wasn't
> > clear to me.
> > 
> > Thanks,
> > Stefan
> 
> I don't really get what does "blocking" mean. Nothing is required to
> block I think.
> I guess for abort it is still in order but a different vq
> should not be needed.
> 
> Maybe we can use ID of buffer to cancel commands?
> 
> 1. driver submits command as ID = 1
> 2. driver submits abort as ID 2
> 3. device sees the abort and cancels ID 2
> 4. device uses buffer 1
> 5. device uses buffer 3 - abort is complete
> 
> Device gets commands and processes them in order. It's just like scsi:
> multiple queues, if you break it you get to keep both pieces.

Now I'm confused because your earlier answer that "we have multiple vqs
for that" implied that a command that takes 1 second will stop further
processing of that vq (what I called "blocking") and the driver will
have to resort to another vq if it wishes to run other commands right
away. But your cancel example seems to use a single virtqueue, so that
would mean commands don't block after all.

My question rephrased:
What happens if a command takes 1 second to complete, is the device
allowed to process the next command from the virtqueue during this time,
possibly completing it before the first command?

This requires additional clarification in the spec because "they are
processed by the device in the order in which they are queued" does not
explain whether commands block the virtqueue (in order completion) or
not (out of order completion).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]