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

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

On Thu, Mar 24, 2022 at 10:16:00AM +0100, Christian Schoenebeck wrote:
> 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

It does not simplify the driver, does it? It's always an extra resriction on
it.

> (i.e. same motivation you apparently 
> had in mind for blocking indirect tables for network devices)

OK I got this one. Device writers do complain about the complexity.
However the tradeoff in limiting what device accepts is ability
to migrate to a different device. So there's an advantage to
making this a separate feature bit so that it's clear to people it's
a low-end device.

> and to prevent 
> one side from starving because other side filled entire FIFO with large bulk 
> data for a single message.

This last I don't see why would it be the device's problem. If driver
will starve the device it can always do so, if it wants to use
indirect aggressively to avoid filling up the queue it can do so too.

> > > 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,

Well, indirect descriptors can be chained with VIRTQ_DESC_F_NEXT right?
The number of in the chain is not really predictable, it's not always
1.

> no matter which solution is chosen in the 
> end.
> 
> Best regards,
> Christian Schoenebeck


-- 
MST