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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Mittwoch, 23. März 2022 13:35:00 CET Michael S. Tsirkin wrote:
> On Wed, Mar 23, 2022 at 11:20:07AM +0100, Christian Schoenebeck wrote:
> > On Montag, 21. März 2022 23:13:55 CET Michael S. Tsirkin wrote:
> > > 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:
> > > > > 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?
> > 
> > As far as I understand your suggestion, yes, it would cover:
> > 
> > A. My use case [1]: allowing indirect table length > queue size.
> > B. Stefan's use case [2]: forcing indirect table length < queue size.
> > C. Your use case: forcing chain (within FIFOs) length < queue size.
> > 
> > [1]
> > https://lore.kernel.org/netdev/cover.1640870037.git.linux_oss@crudebyte.c
> > om/ [2]
> > https://lore.kernel.org/all/YXpmwP6RtvY0BmSM@stefanha-x1.localdomain/
> > 
> > However it would not cover:
> > 
> > D. Your other use case: blocking indirect tables.
> 
> Well, the practical use-case I have in mind is actually for when using
> mergeable buffers with the network device, in practice there's never a
> s/g but there's no way for device to know that. So maybe we can declare
> that indirect must not be used when there's a single descriptor in the
> table (esp when the new feature is accepted), and then just limiting s/g
> to 1 will do the trick in this specific case.
> 
> > E. Potential other people's need: max. chain length (within FIFOs) != max.
> > 
> >    indirect table length.
> 
> I don't understand this one, sorry.

Example: say you would want to allow large indirect tables, but OTOH want to 
block any chained (pre-allocated) direct descriptors in the vring FIFOs 
themselfes, e.g.:

	Queue Size (max. # "buffers" in FIFO) = 127
	Indirect Size (max. # segments in indirect descr. table) = 65535
	Direct Size (max. # chained descr. per "buffer" in FIFO) = 1
	Total Size (max. # sum of all descr. per "buffer") = 65536

Your suggestion would not cover this example. Why would somebody want that? 
Because of device/driver simplification (i.e. same motivation you apparently 
had in mind for blocking indirect tables for network devices) and to prevent 
one side from starving because other side filled entire FIFO with large bulk 
data for a single message.

> > Instead of yet again mixing different limits again with each other, what
> > about using 3 distinct fields for those limits instead:
> > 
> > 1. max. indirect table length (any descriptor in the table)
> > 2. max. chain length (currently: descriptors within FIFOs)
> > 3. max. total descriptors per buffer (sum of all descriptors, both from
> > FIFOs> 
> >    and indirect tables, including descriptor pointing to a table).
> 
> So this doesn't actually address at least what you called "your
> use-case" above. Well it almost does, but not exactly because
> of the extra indirect descriptor.

Of course it would cover it. And of course you would have to either subtract 
or add one descriptor somewhere, no matter which solution is chosen in the 
end.

Best regards,
Christian Schoenebeck