Re: [virtio-comment] Re: [PATCH v2 0/2] Add VIRTIO_RING_F_LARGE_INDIRECT_DESC

[Cornelia Huck <cohuck@redhat.com>]

On Tue, Dec 07 2021, Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Mon, Dec 06, 2021 at 08:12:14PM +0100, Christian Schoenebeck wrote:
>> On Montag, 6. Dezember 2021 12:52:07 CET Cornelia Huck wrote:
>> > On Fri, Dec 03 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > > On Donnerstag, 2. Dezember 2021 11:27:17 CET Cornelia Huck wrote:
>> > >> On Tue, Nov 30 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > >> > On Dienstag, 30. November 2021 14:48:50 CET Cornelia Huck wrote:
>> > >> >> On Tue, Nov 30 2021, Christian Schoenebeck <qemu_oss@crudebyte.com>
>> > >> > Another issue I just realized: there is also an ambiguity in this v2
>> > >> > what
>> > >> > the maximum descriptor count actually relates to. Should it be
>> > >> > 
>> > >> > 1. max. indirect descriptor count per indirect descriptor table
>> > >> > 
>> > >> > or
>> > >> > 
>> > >> > 2. max. indirect descriptor count per vring slot (i.e. the sum from
>> > >> > multiple indirect descriptor tables within the same message)
>> > >> > 
>> > >> > Case 2 applies to QEMU's implementation right now AFAICS. The max.
>> > >> > possible
>> > >> > bulk transfer size is lower in case 2 accordingly.
>> > > 
>> > > After reviewing virtio code on QEMU side again, I suggest to go for (2.).
>> > > Otherwise a large portion of QEMU's virtio code would need quite a bunch
>> > > of
>> > > changes to support (1.). I assume that resistence for such changes in QEMU
>> > > would be high, and I currently don't care enough to work on and defend
>> > > those changes that (1.) would need.
>> > > 
>> > > In practice that would mean for many devices: the theoretical absolute
>> > > max.
>> > > virtio transfer size might be cut into half with (2.) in comparison to
>> > > (1.), which is (2^16 * PAGE_SIZE) / 2 = 128 MB with a typical page size
>> > > of 4k, because one indirect descriptor table is usually used for sending
>> > > to device and another table for receiving from device. But that's use
>> > > case dependent and (1.) is still a huge step forward IMO.
>> > 
>> > If the variant that is easier for QEMU to implement still gives you
>> > enough of what you need, I'm fine with going with that. (Is it
>> > future-proof enough?)
>> 
>> No crystal ball here, sorry. :)

:)

>> 
>> Just to give you a feeling what I am talking about here for QEMU, you might
>> have a quick glimpse on the hw/virtio/virtio.c changes of following patch. It
>> is not exactly how the final changes would look like, but it should give a
>> rough idea of what is involved:
>> https://lore.kernel.org/all/c9dea2e27ae19b2b9a51e8f08687067bf3e47bd5.1633376313.git.qemu_oss@crudebyte.com/
>> 
>> As you can see, QEMU first reserves the max. expected descriptor count as
>> array memory on stack, then it gathers *all* descriptors from all indirect
>> descriptor tables of a vring slot all together into that array and finally
>> the vring slot's message is processed on device level:
>> https://github.com/qemu/qemu/blob/99fc08366b06282614daeda989d2fde6ab8a707f/hw/virtio/virtio.c#L1475
>> 
>> So a limit per vring slot would be much easier to implement in QEMU, as it is
>> more or less just refactoring of QEMU's current compile-time constant
>> VIRTQUEUE_MAX_SIZE into a runtime variable.
>> Implementing a limit per table instead would require substantial changes to
>> its current program flow.
>> 
>> Back to your question ...
>> 
>> Assuming that most devices have one or two tables per vring slot, and
>> considering that almost nobody cared for virtio's current descriptor count
>> limit so far, I would not expect that the new, much higher limit to be
>> questioned in the next few years or so. And if it was, you would probably also
>> start to question all those 16-bit fields in virtio as well and then this
>> specific aspect here would probably be the smallest issue to worry about.
>> 
>> OTOH if there are devices with like 10 descriptor tables or more per vring
>> slot, then they obviously would hit this limit much sooner. No idea if there
>> is any such device though.
>
> Other device implementations probably also care about the total number
> of descriptors per vring slot instead of the number of descriptors per
> indirect table. The limitation on the device side is the resource
> requirement and/or maximum supported by the underlying I/O mechanism, so
> the total number of descriptors is likely to matter.

Thanks to you both; going with the total number seems to be best.