[virtio-comment] RE: [virtio-dev] RE: [PATCH v5 0/7] Introduce device group and device management

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, July 25, 2022 3:44 AM
> > >
> > How did you calculate the cost being negligible?
> 
> For sure each VF needs at least 2 VQs plus the AQ. Each one is 3 64 bit
> pointers, plus some flags and counters, plus at least one MSI vector about
> 200-300 bit.
> We are talking each feature taking up a single bit per VF right now.
> That's below 1%.
>
Because you assumed that new feature bits in this area will never be added.
The AQ and its capabilities are not limited to PF even though first version of the spec has only few bits.

And these 1% at scale results in several Kbytes.
 
> 
> > > > So better to start using AQ for new functionalities.
> > > > What is stopping us to adapt to this modern and optimized way?
> > >
> > > The cost/benefit tradeoff. The benefit is a theorectical gain of a
> > > few bits per VF. The cost is very real engineering time spent.
> > >
> > How is it theoretical? I provided the calculation few times in previous
> emails.
> 
> It's theoretical because we don't know whether anyone will ever add AQ to
> VFs as opposed to a PF.
>
Isnt the scope of AQ now more than just msix+features+livemigration?
Such as managing siov and more?

Things like queue_reset, query counters and other things can be done more efficiently through a vq than some burned bits regardless of SIOV.
So in my view looking at AQ for msix, LM purpose is narrow view to stay on the PF.
 
> > > I can keep poking holes and finding underspecified behaviour in the
> > > proposal.
> > We should fix the wording.
> > > But I don't really see why would anyone spend the time duplicating
> > > what virtio spec is already doing decently.
> > >
> > > > Why cant we keep features bits limited to device startup
> > > > negotiation
> > > scheme?
> > >
> > > Well they are already used for much more. So sure, maybe work on
> > > changing that, but IMO trying it all in a single huge project is just not a
> great idea.
> > >
> > Why would you recommend on changing something historic like this?
> 
> Because that will bring a much bigger gain.
>
That also breaks existing backward compatibility for existing devices.
So it has to be something additional and optional.

 
> > The ask here to improve the new features.
> 
> Let's just improve everything, a much bigger gain.  Simply put, pci transport
> was never designed with saving per device memory in mind.
Hence, new addition shouldn't make it worse when the new optimized methods are available.

> Adding complexity to save a couple of bytes per device will just create bugs
> without solving that. 
I disagree that it creates bugs.
When done right, we have seen in multiple projects (nvme, mlx5 and more) that it doesn't create bugs.

> AQ is already trying to solve grouping devices and that's
> a big project, let's finish that and then decide whether we want to work on
> migration or work on saving memory next.
> 
I assume migration means live migration and not migration from pci to something else.
If its former, it is not either-or project.
Saving memory is attributed as project and it doesn't have to be for the small piece being added here.

> > > > Other unrelated bits are added in past to feature bits  But lets
> > > > follow the
> > > good examples instead when the new choice is already proposed and
> > > defined.
> > >
> > > So IMHO saving a few bits here and there when we are spending tens
> > > of bytes on state makes no sense.
> > >
You missed the multiply factor of VFs. :)

It is not only saving bits, it also enforces certain aspects of how a device is being composed that must expose these bits pretty early.

> > Not a good reason to introduce something inferior.
> 
> Avoiding duplication and focusing on low hanging fruit are all sound
> engineering principles.
> 
It is not a duplication.
I iterate again. The bits we asked to put in the Aq commands are the one that does not need to be negotiated early.
Feature bits were mis-used to put all things in there because in past a generic AQ didn't exists.
And there is no reason for that past decision to influence current proposal.

Feature bits that are so basic for device bring up stage, stays in feature bits area defined today.
Hence, its not a duplication.

> 
> > > Something like transport over AQ (or the transport vq proposal) or
> > > some other concerted effort to save per device memory would be
> > > needed for SIOV, and maybe it's useful for SRIOV too.
> > >
> > AQ proposal is already doing it.
> > Some of the things seems to be lately duplicated in transport vq proposal.
> > We should possibly rename both the queues to mgmt_queue that serves
> both the purposes.
> 
> Quite possibly - transport VQ seems to handle a group of devices from one
> device which seems similar to what AQ does, and you guys should probably
> work together.
>
Yes.
 
> But whether we do or do not unify transport and admin vq, with transport
Without unification, I don't see how SIOV proposal can ever make to the spec.
Once AQ is merged SIOV can use AQ.
I would like to see SIOV to refer to the AQ proposal commands and extend it. Not as separate proposal.

> vq existing feature mechanism stops taking up space in the VQ and this
> opens up possibility to just use that mechanism to discover supported
> commands without paying memory penalty.
> 
This only helps any new SIOV beast which is not even fully defined at various layers of platform.

> 
> > Transport VQ proposal is tunneling some SIOV device feature bits.
> > Here AQ is negotiating its own feature bits. Both are orthogonal.
> 
> I thought the discussion was about dropping the get features command and
> using features instead. 

> The Transport VQ proposal seems to show how we
> can do that and still make things scale.  Yes it's orthogonal to using AQ for
> grouping, and using features is exactly what will keep it orthogonal.
> 
> > And suggesting to some newer RFC to discuss current one doesn't make
> much sense to tangent the discussion at all.
> 
> 
> The reason I bring it up is that it seems like a better solution to saving device
> memory than adding a get capabilities command. And it let you just use
> features for now, and assume whatever we spend will be saved back by
> using vq as transport.
> 
Even for non SIOV devices?

> 
> > Since there is zero technical short comings of negotiating AQ features via
> AQ command, lets please conclude to proceed with it.
> 
> Not 100% sure what you are saing here, so I think you should post a new
> version, yes. Generally iterating faster is a good idea. If you feel you didn't
> get enough feedback on a given version and some time passed try pinging
> people.
Without closing the discussion, I have seen same topic come up again and again in future version.
So even though new series is up for posting, we better close the discussion.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/