Re: [virtio-comment] [PATCH v3 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE

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

On Mon, Mar 21, 2022 at 10:23:07AM +0100, Christian Schoenebeck wrote:
> On Sonntag, 20. März 2022 22:52:16 CET Michael S. Tsirkin wrote:
> > On Sun, Mar 20, 2022 at 06:43:53PM +0100, Christian Schoenebeck wrote:
> > > To be honest, I don't feel like discussing precise wordings at this point
> > > when you are questioning the proposal on design level already.
> > > 
> > > Maybe you make some more thorough thoughts on what you actually want this
> > > to be on design level before continueing to argue about precise
> > > terminology, which you are not using either BTW when you articulating
> > > your criticism.
> > > 
> > > Or even better: come up with your own proposol with the precise wording
> > > you
> > > feel appropriate.
> > 
> > OK let's go back and agree on what we are trying to achieve.  The github
> > issue and the cover letter imply that while indirect descriptors would
> > normally allow huge tables, we artificially limit them to queue size,
> > and you want to be able to relax that.
> 
> Correct, that's my motivation for all of this.

Okay. So I think that given this, we can limit the total number
of non-indirect descriptors, including non-indirect ones
in a chain + all the ones in indirect pointer table if any,
and excluding the indirect descriptor itself, and this
will address the issue you are describing here, right?

Question:
- I am thinking about bi-directional descriptors such as
  block and scsi have. it looks like they have a separate limit for
  read and write parts of the chain. Should we have two limits
  then? Or should we just make driver use the lower of the two,
  i.e. the per vq limit applies to the total # of elements
  in a buffer, and read/write sgs are controlled separately
  by the per device control?
  

Stefan, any comments?



> > Fair enough.
> > 
> > However, I feel trying to talk about indirect descriptor is too narrow a
> > use-case, simply because the issue is not indirect at all.  Why do we
> > limit number of segments? I think it's really because of backend
> > limitations. And indirect is only used by the frontend.  So limiting
> > that is really going about it wrong.
> 
> I am only aware about current implementation situation in QEMU and Linux 
> kernel. As for those two: yes, it is not a limitation on Linux kernel side, 
> but on QEMU side.
> 
> As for other implementations: no idea.
> 
> > So block for example has seg_max already. What should happen
> > if that exceeds queue size is not defined.
> > 
> > So maybe we can generalize that making it device independent?
> > The litmus paper for this is the block and scsi devices,
> > we should be able to use the new feature as a super-set.
> > 
> > Before we discuss solutions, did I formulate the problem correctly?
> 
> Keep in mind that I never worked on virtio code or virtio spec before. I just 
> started to review virtio implementation of QEMU and Linux kernel and the 
> virtio spec in November, specifically in context of 9p. I definitely don't 
> know all the other virtioo device classes out there.

I think it's great that we have someone taking care of 9p btw!

> 
> In other words: I can't help you on fitting this appropriately into a superset 
> picture.
> 
> Best regards,
> Christian Schoenebeck

I think we need some cleanups in the spec to make what you are trying to
do possible to do cleanly, specifically move this description:

	A buffer consists of zero or more device-readable physically-contiguous
	elements followed by zero or more physically-contiguous
	device-writable elements (each buffer has at least one element).

out to the generic part from packed ring part, drop corresponding
text from split ring and make it refer to the term "element".

After this, the new field will just be "a number of elements per
buffer" (note that indirect descriptors do not themselves
describe elements and so won't be included in the math).

Christian, you mentioned you don't like the term buffer generally,
changing that can be done before or after this feature but IMHO
best not as part of it.

I think it's good in that it will fit better in the superset picture
addressing in addition to your requirement also the requirement to have
huge rings while limiting descriptors.

Does above sound like it addresses your requirements of having
a longer descriptor chain than queue size? If not what is not
addressed?


Thanks,
-- 
MST