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

[Jiri Pirko <jiri@nvidia.com>]

Wed, Mar 08, 2023 at 01:44:18PM CET, stefanha@redhat.com wrote:
>On Wed, Mar 08, 2023 at 11:17:35AM +0100, Jiri Pirko wrote:
>> Tue, Mar 07, 2023 at 08:03:47PM CET, stefanha@redhat.com wrote:
>> >On Tue, Mar 07, 2023 at 04:07:54PM +0100, Jiri Pirko wrote:
>> >> Tue, Mar 07, 2023 at 03:39:11PM CET, stefanha@redhat.com wrote:
>> >> >On Tue, Mar 07, 2023 at 09:03:18AM +0100, Jiri Pirko wrote:
>> >> >> Mon, Mar 06, 2023 at 07:37:31PM CET, mst@redhat.com wrote:
>> >> >> >On Mon, Mar 06, 2023 at 06:03:40AM -0500, Stefan Hajnoczi wrote:
>> >> >> >> On Sun, Mar 05, 2023 at 07:18:24PM -0500, Michael S. Tsirkin wrote:
>> >> >> >> > On Sun, Mar 05, 2023 at 07:03:02PM -0500, Stefan Hajnoczi wrote:
>> >> >> >> > > On Sun, Mar 05, 2023 at 04:38:59AM -0500, Michael S. Tsirkin wrote:
>> >> >> >> > > > On Fri, Mar 03, 2023 at 03:21:33PM -0500, Stefan Hajnoczi wrote:
>> >> >> >> > > > > 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).
>> >> >> >> > > > 
>> >> >> >> > > > Oh I begin to see. Hmm how does e.g. virtio scsi handle this?
>> >> >> >> > > 
>> >> >> >> > > virtio-scsi, virtio-blk, and NVMe requests may complete out of order.
>> >> >> >> > > Several may be processed by the device at the same time.
>> >> >> >> > 
>> >> >> >> > Let's say I submit a write followed by read - is read
>> >> >> >> > guaranteed to return an up to date info?
>> >> >> >> 
>> >> >> >> In general, no. The driver must wait for the write completion before
>> >> >> >> submitting the read if it wants consistency.
>> >> >> >> 
>> >> >> >> Stefan
>> >> >> >
>> >> >> >I see.  I think it's a good design to follow then.
>> >> >> 
>> >> >> Hmm, is it suitable to have this approach for configuration interface?
>> >> >> Storage device is a different beast, having parallel reads and writes
>> >> >> makes complete sense for performance.
>> >> >> 
>> >> >> ->read a req
>> >> >> ->read b req
>> >> >> ->read c req
>> >> >> <-read a rep
>> >> >> <-read b rep
>> >> >> <-read c rep
>> >> >> 
>> >> >> There is no dependency, even between writes.
>> >> >> 
>> >> >> But in case of configuration, does not make any sense to me.
>> >> >> Why is it needed? To pass the burden of consistency of
>> >> >> configuration to driver sounds odd at least.
>> >> >> 
>> >> >> I sense there is no concete idea about what the "admin virtqueue" should
>> >> >> serve for exactly.
>> >> >
>> >> >It's useful for long-running commands because they prevent other
>> >> >commands from executing.
>> >> >
>> >> >An example I've given is that deleting a group member might require
>> >> >waiting for the group member's I/O activity to finish. If that I/O
>> >> >activity cannot be cancelled instantaneously, then it could take an
>> >> >unbounded amount of time to delete the group member. The device would be
>> >> >unable to process futher admin commands.
>> >> 
>> >> I see. Then I believe that the device should handle the dependencies.
>> >> Example 1:
>> >> -> REQ cmd to create group member A
>> >> -> REQ cmd to create group member B
>> >> <- REP cmd to create group member A
>> >> <- REP cmd to create group member B
>> >> 
>> >> The device according to internal implementation can either serialize the
>> >> 2 group member creations or do it in parallel, if it supports it.
>> >> 
>> >> Example 2:
>> >> -> REQ cmd to create group member A
>> >> -> REQ cmd config group member A
>> >> <- REP cmd to create group member A
>> >> <- REP cmd config group member A
>> >> 
>> >> Here the serialization is necessary and the device is the one to take
>> >> care of it.
>> >> 
>> >> Makes sense?
>> >
>> >Yes, I understand. The spec would need to define ordering rules for
>> >specific commands and the device must implement them. It allows the
>> >driver to pipeline commands while also allowing out-of-order completion
>> >(parallelism) in some cases. The disadvantage of this approach is
>> >complexity in the spec and implementations.
>> >
>> >An alternative is unconditional out-of-order completion, where there are
>> >no per-command ordering rules. The driver must wait for a command to
>> >complete if it relies on the results of that command for its next
>> >command. I like this approach because it's less complex in the spec and
>> >for device implementers, while the burden on the driver implementer is
>> >still reasonable.
>> 
>> But isn't this duplicating the burden of maintaining dependencies to
>> both driver and device? I mean, device should not depend on driver doing
>> the right thing, that means it has to check the dependencies for every
>> incoming command anyway. The only difference would be to wait instead of
>> returning "-EBUSY" in case the dependency is not satisfied yet.
>
>The device does not need to reject commands that have dependencies with
>-EBUSY. The result of commands with dependencies is either A -> B or B
>-> A.
>
>For example:
>1. Create Group Member A
>2. Delete Group Member A
>
>Command 2 might succeed or it might fail with -ENOENT because Group
>Member A doesn't exist yet.

Yeah, correct.

>
>> Device knows exactly what are the dependencies. And I believe, those are
>> device implementation specific. For example, some implementation could
>> support parallel VF config cmd execution, some implementation might
>> need to serialize that. Driver has no clue.
>
>Yes, that's up to the device. Out-of-order completion is a superset of
>in-order completion. So the device is allowed to run commands in series
>when it wants. A driver designed for out-of-order completion will work
>fine either way.

Agreed.


>
>> Could you please elaborate a bit more what you mean by "complexity in
>> the spec"?
>
>When adding commands to the spec, the dependency relationships with
>other commands need to be thought about and documented.
>
>Device implementers need to get those relationships right. That means
>they need to remember that command B waits for command A.

That's my point, this is device implementation specific. Spec should not
define anything like this, that might be limiting in some of the cases.

>
>Driver implementers have to understand that command B waits for command
>A but not command C according to the spec.
>
>That seems complex to me.

Sure, I agree this does not belong to the spec, it can't, really.
For the record, I never suggested it.

I think we are in agreement.


>
>Stefan