| Message ID | 20240321155717.1392787-2-jonah.palmer@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio,vhost: Add VIRTIO_F_IN_ORDER support | expand |
On Thu, Mar 21, 2024 at 4:57 PM Jonah Palmer <jonah.palmer@oracle.com> wrote: > > Define the InOrderVQElement structure for the VIRTIO_F_IN_ORDER > transport feature implementation. > > The InOrderVQElement structure is used to encapsulate out-of-order > VirtQueueElement data that was processed by the host. This data > includes: > - The processed VirtQueueElement (elem) > - Length of data (len) > - VirtQueueElement array index (idx) > - Number of processed VirtQueueElements (count) > > InOrderVQElements will be stored in a buffering mechanism until an > order can be achieved. > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> > --- > include/hw/virtio/virtio.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index b3c74a1bca..c8aa435a5e 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -77,6 +77,13 @@ typedef struct VirtQueueElement > struct iovec *out_sg; > } VirtQueueElement; > > +typedef struct InOrderVQElement { > + const VirtQueueElement *elem; Some subsystems allocate space for extra elements after VirtQueueElement, like VirtIOBlockReq. You can request virtqueue_pop to allocate this extra space by its second argument. Would it work for this? > + unsigned int len; > + unsigned int idx; > + unsigned int count; Now I don't get why these fields cannot be obtained from elem->(len, index, ndescs) ? > +} InOrderVQElement; > + > #define VIRTIO_QUEUE_MAX 1024 > > #define VIRTIO_NO_VECTOR 0xffff > -- > 2.39.3 >
On 3/22/24 5:45 AM, Eugenio Perez Martin wrote: > On Thu, Mar 21, 2024 at 4:57 PM Jonah Palmer <jonah.palmer@oracle.com> wrote: >> >> Define the InOrderVQElement structure for the VIRTIO_F_IN_ORDER >> transport feature implementation. >> >> The InOrderVQElement structure is used to encapsulate out-of-order >> VirtQueueElement data that was processed by the host. This data >> includes: >> - The processed VirtQueueElement (elem) >> - Length of data (len) >> - VirtQueueElement array index (idx) >> - Number of processed VirtQueueElements (count) >> >> InOrderVQElements will be stored in a buffering mechanism until an >> order can be achieved. >> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >> --- >> include/hw/virtio/virtio.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index b3c74a1bca..c8aa435a5e 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -77,6 +77,13 @@ typedef struct VirtQueueElement >> struct iovec *out_sg; >> } VirtQueueElement; >> >> +typedef struct InOrderVQElement { >> + const VirtQueueElement *elem; > > Some subsystems allocate space for extra elements after > VirtQueueElement, like VirtIOBlockReq. You can request virtqueue_pop > to allocate this extra space by its second argument. Would it work for > this? > I don't see why not. Although this may not be necessary due to me missing a key aspect mentioned in your comment below. >> + unsigned int len; >> + unsigned int idx; >> + unsigned int count; > > Now I don't get why these fields cannot be obtained from elem->(len, > index, ndescs) ? > Interesting. I didn't realize that these values are equivalent to a VirtQueueElement's len, index, and ndescs fields. Is this always true? Else I would've expected, for example, virtqueue_push to not need the 'unsigned int len' parameter if this information is already included via. the VirtQueueElement being passed in. >> +} InOrderVQElement; >> + >> #define VIRTIO_QUEUE_MAX 1024 >> >> #define VIRTIO_NO_VECTOR 0xffff >> -- >> 2.39.3 >> >
On Mon, Mar 25, 2024 at 6:08 PM Jonah Palmer <jonah.palmer@oracle.com> wrote: > > > > On 3/22/24 5:45 AM, Eugenio Perez Martin wrote: > > On Thu, Mar 21, 2024 at 4:57 PM Jonah Palmer <jonah.palmer@oracle.com> wrote: > >> > >> Define the InOrderVQElement structure for the VIRTIO_F_IN_ORDER > >> transport feature implementation. > >> > >> The InOrderVQElement structure is used to encapsulate out-of-order > >> VirtQueueElement data that was processed by the host. This data > >> includes: > >> - The processed VirtQueueElement (elem) > >> - Length of data (len) > >> - VirtQueueElement array index (idx) > >> - Number of processed VirtQueueElements (count) > >> > >> InOrderVQElements will be stored in a buffering mechanism until an > >> order can be achieved. > >> > >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> > >> --- > >> include/hw/virtio/virtio.h | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > >> index b3c74a1bca..c8aa435a5e 100644 > >> --- a/include/hw/virtio/virtio.h > >> +++ b/include/hw/virtio/virtio.h > >> @@ -77,6 +77,13 @@ typedef struct VirtQueueElement > >> struct iovec *out_sg; > >> } VirtQueueElement; > >> > >> +typedef struct InOrderVQElement { > >> + const VirtQueueElement *elem; > > > > Some subsystems allocate space for extra elements after > > VirtQueueElement, like VirtIOBlockReq. You can request virtqueue_pop > > to allocate this extra space by its second argument. Would it work for > > this? > > > > I don't see why not. Although this may not be necessary due to me > missing a key aspect mentioned in your comment below. > > >> + unsigned int len; > >> + unsigned int idx; > >> + unsigned int count; > > > > Now I don't get why these fields cannot be obtained from elem->(len, > > index, ndescs) ? > > > > Interesting. I didn't realize that these values are equivalent to a > VirtQueueElement's len, index, and ndescs fields. > > Is this always true? Else I would've expected, for example, > virtqueue_push to not need the 'unsigned int len' parameter if this > information is already included via. the VirtQueueElement being passed in. > The code uses "len" to store the written length values of each used descriptor between virtqueue_fill and virtqueue_flush. But not all devices use these separately, only the ones that batches: virtio-net and SVQ. A smarter / less simpler implementation of virtqueue_push could certainly avoid storing elem->len. But the performance gain is probably tiny, and the code complexity grows. > >> +} InOrderVQElement; > >> + > >> #define VIRTIO_QUEUE_MAX 1024 > >> > >> #define VIRTIO_NO_VECTOR 0xffff > >> -- > >> 2.39.3 > >> > > >
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index b3c74a1bca..c8aa435a5e 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -77,6 +77,13 @@ typedef struct VirtQueueElement struct iovec *out_sg; } VirtQueueElement; +typedef struct InOrderVQElement { + const VirtQueueElement *elem; + unsigned int len; + unsigned int idx; + unsigned int count; +} InOrderVQElement; + #define VIRTIO_QUEUE_MAX 1024 #define VIRTIO_NO_VECTOR 0xffff
Define the InOrderVQElement structure for the VIRTIO_F_IN_ORDER transport feature implementation. The InOrderVQElement structure is used to encapsulate out-of-order VirtQueueElement data that was processed by the host. This data includes: - The processed VirtQueueElement (elem) - Length of data (len) - VirtQueueElement array index (idx) - Number of processed VirtQueueElements (count) InOrderVQElements will be stored in a buffering mechanism until an order can be achieved. Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> --- include/hw/virtio/virtio.h | 7 +++++++ 1 file changed, 7 insertions(+)