[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: VIRTIO_RING_F_INDIRECT_SIZE status
On Wed, Mar 01, 2023 at 01:55:14PM +0100, Christian Schoenebeck wrote: > On Tuesday, February 28, 2023 1:05:21 PM CET Michael S. Tsirkin wrote: > > On Tue, Feb 28, 2023 at 12:49:00PM +0100, Christian Schoenebeck wrote: > > > On Monday, February 27, 2023 6:41:12 PM CET Michael S. Tsirkin wrote: > > > > On Mon, Feb 27, 2023 at 05:13:12PM +0100, Christian Schoenebeck wrote: > > > > > On Monday, February 27, 2023 4:45:45 PM CET Stefan Hajnoczi wrote: > > > > > > On Mon, Feb 27, 2023 at 02:53:55PM +0000, Afsa, Baptiste wrote: > > > > > > > The issue that we have now, is that this limitation does not seem to be enforced > > > > > > > in Linux virtio drivers today. I came across: > > > > > > > > > > > > > > https://github.com/oasis-tcs/virtio-spec/issues/122 > > > > > > > > > > > > > > which looks like a good base for us to build upon, but I'm not sure what is the > > > > > > > status with this issue. Do you know whether there is any plan regarding this? > > > > > > > > > > > > CCing Christian regarding extending queue size limits. > > > > > > > > > > Status on this issue: it was suspended in May last year. AFAICR Michael > > > > > expressed the need to give some more thought about it, and a new virtio spec > > > > > release was just ahead at that time. > > > > > > > > > > Michael, suggestions how to bring this forward? > > > > > > > > > > Best regards, > > > > > Christian Schoenebeck > > > > > > > > > > > > > How about a summary letter listing various options, pros and cons? > > > > > > Actually I had submitted a draft for this feature (latest version linked > > > above), and AFAICR Michael was the only person who expressed concerns from > > > design perspective. Comments by other people were already just aboout precise > > > wording, but not about the design itself. > > > > > > We stopped the discussion at the point where Michael expressed the need to > > > think more about it, but as his expressed concerns were a bit vague, I still > > > don't see how I could bring this issue forward. > > > > > > Best regards, > > > Christian Schoenebeck > > > > > > > > > So the last time there were two things: > > > > > > 1. bugs introduced during the packed ring work. For example: > > > > > > > > I don't think so: > > > > > > 2.6.5.3.1 Driver Requirements: Indirect Descriptors > > > .. > > > "A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT in > > > flags." > > > So as far as I can see it, the amount of direct descriptors is currently > > > always exactly one if an indirect table is used. > > > > > > Best regards, > > > Christian Schoenebeck > > > > > > > Oh. You are right. Weirdly this text is not in packed-ring.tex - I > > think this and a bunch of other cases like this are an oversight, > > however we need to fix them first before adding features that assume > > they are fixed ... > > > > we need to get them fixed before we poke at core ring otherwise it's a > > mess - maybe the TC will accept just fixing this > > without a feature bit, or maybe people will feel a feature bit > > is required. > > It's been a while. Looking at v1.2 it says: > > 2.8 Packed Virtqueues > ... > 2.8.5 Scatter-Gather Support [1] > ... > While unusual (most implementations either create all lists solely using > non-indirect descriptors, or always use a single indirect element), if both > features have been negotiated, mixing indirect and non-indirect descriptors > in a ring is valid, as long as each list only contains descriptors of a > given type. > > [1] https://docs.oasis-open.org/virtio/virtio/v1.2/cs01/virtio-v1.2-cs01.html#x1-770005 > > To avoid misapprehensions: the way I understand it, same restrictions apply to > packed queues as split queues, in the sense that you may neither chain several > tables in a single message, nor multi-level nest tables, nor mix a list of > direct descriptors and indirect descriptors on the same level within one > message. So the explicit exception described here, only means you may use > *one* indirect table in one message, while using chained direct descriptors in > another message. But that's it, right? Hi Christian, Yes, for packed virtqueues it is forbidden to mix normal and indirect descriptors in a single buffer. Further support for this interpretation is in 2.8.19 Driver Requirements: Indirect Descriptors: A driver MUST NOT write direct descriptors with VIRTQ_DESC_F_INDIRECT set in a scatter-gather list linked by VIRTQ_DESC_F_NEXT. flags. VIRTQ_DESC_F_INDIRECT cannot be set if the descriptor chain uses VIRTQ_DESC_F_NEXT, therefore it's not possible to mix normal and indirect descriptors in a single buffer. However, the Split Virtqueues section documents an exception where mixing normal and indirect descriptors in a single buffer is allowed, but only in a specific order. 2.7.5.3.2 Device Requirements: Indirect Descriptors says: The device MUST handle the case of zero or more normal chained descriptors followed by a single descriptor with flags&VIRTQ_DESC_F_INDIRECT. Note: While unusual (most implementations either create a chain solely using non-indirect descriptors, or use a single indirect element), such a layout is valid. So what degree of mixing normal and indirect descriptors is allowed depends on the virtqueue format. Stefan > > 2. Given this is a lot of work I am trying to find a way to > > make the impact bigger. In particular to cover the use-case > > of limiting s/g to 1k while making queues deeper (with > > or without indirect). For this I proposed: > > > > 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? > > > > people seemed to be ok with this idea? > > IIUIC it would not make a difference from design perspective from what I > proposed, as virtio currently neither allows to mix, chain or mult-level nest > indirect descriptor tables within a single message), and hence it would just > boil down to adjusting the wording. So yes, it would therefore cover my > intended use case. > > Best regards, > Christian Schoenebeck > >
Attachment:
signature.asc
Description: PGP signature
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]