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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 09, 2023 at 02:30:52PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, March 9, 2023 9:23 AM
> > 
> > On Thu, Mar 09, 2023 at 02:11:54PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: David Edmondson <david.edmondson@oracle.com>
> > > > Sent: Thursday, March 9, 2023 9:03 AM
> > > >
> > > > Parav Pandit <parav@nvidia.com> writes:
> > > >
> > > > >> From: Jiri Pirko <jiri@nvidia.com>
> > > > >> Sent: Thursday, March 9, 2023 2:31 AM
> > > > >>
> > > > >> Wed, Mar 08, 2023 at 10:25:32PM CET, parav@nvidia.com wrote:
> > > > >> >
> > > > >> >> From: virtio-comment@lists.oasis-open.org
> > > > >> >> <virtio-comment@lists.oasis- open.org> On Behalf Of David
> > > > >> >> Edmondson
> > > > >> >
> > > > >> >> In support of live migration, might we end up moving large
> > > > >> >> amounts of device state through the admin queue?
> > > > >> >>
> > > > >> >Correct.
> > > > >> >
> > > > >> >> If so, that would seem to have some performance requirements,
> > > > >> >> though I don't know if it would justify multiple admin queues.
> > > > >> >DMA of the data through the proposed AQ is supported.
> > > > >> >
> > > > >> >If I understood Max correctly when Max said " This AQ is not
> > > > >> >aimed for performance ", he means that AQ doesn't have
> > > > >> >performance requirements as
> > > > >> io/network queues to complete millions of ops/sec.
> > > > >> >
> > > > >> >it is several hundred to maybe (on the higher side) thousand
> > > > >> >ops/sec during
> > > > >> LM, provisioning use case.
> > > > >>
> > > > >> But isn't it good to design it for performance from start? I
> > > > >> mean, state transfer of thousands of VFs at a time is definitelly
> > > > >> performance
> > > > related, isn't it?
> > > > >>
> > > > > It is. Which part of the proposed AQ doesn't cover this aspect?
> > > > > The only issue that I see today is, that a given GET family of
> > > > > commands q
> > > > contains the read-only and write-only descriptors which require
> > > > multiple dma allocations on driver side.
> > > >
> > > > I don't think that there is any assertion that the current proposal
> > > > doesn't cover this.
> > > >
> > > > It was intended as an illustration to try and help determine whether
> > > > multiple AQs for a single device was a requirement given existing
> > > > anticipated usage, or a safeguard against potential future uses.
> > >
> > > Got it.
> > >
> > > So far my understanding is:
> > > 1. Single AQ is enough for currently cited use cases
> > 
> > FYI transport VQ work started off with multiple vqs.
> > 
> Ok. so there may be some use case for transport VQ.
> Worth to mention in the cover letter and things will be fine.
> Actual review of the transport VQ and its use will be in the respective patchset.
> 
> > > 2. Having the infrastructure to support multiple AQ is also good.
> > > The extra cost of this infra is only one read-only field = aq count as Michael
> > proposed.
> > >
> > > 3. Ability to send multiple outstanding commands and execute them out of
> > order by device is/should be supported as long as IN_ORDER is not negotiated.
> > > Should it be relaxed for AQ that even if IN_ORDER is negotiated, AQ by
> > default can be out of order queue?
> > 
> > I feel there's no need: driver can just avoid negotiating IN_ORDER.
> I didn't understand the "no need" part.
> Do you mean to make use of out-of-order AQ commands, driver should skip IN_ORDER negotiation for the data path queues?
> If yes, it may not be a big loss for the PF.
> 
> I am considering that since AQ is so fresh, it can keep itself detached from the IN_ORDER from the beginning.
> This way AQ and data path queues stay on their own course.

You can make this claim for different types of data queues too.
I'd rather try to solve it for all queues than special-case AQ.

> > Generally I'm not happy with IN_ORDER and have some ideas to replace it with
> > PARTIAL_ORDER, that will also address this.
> > 
> > > 4. Driver is the sole owner to decide if it is modifying an object through AQ, it
> > should synchronize with ongoing AQ command on the same object or not.
> > > 5. If the command gets stuck for a very long time, the driver can recover the
> > AQ using the existing Q RESET facility.
> > >
> > > Do we agree with the above design thoughts?
> > > Or there is a better design than this discussed, and I missed it?
> > >