RE: [virtio-dev] [PATCH v10 13/13] split-ring: in order feature

[Lars Ganrot <lga@napatech.com>]

Missed replying to the lists. Sorry.

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: 28. marts 2018 16:39
> 
> On Wed, Mar 28, 2018 at 08:23:38AM +0000, Lars Ganrot wrote:
> > Hi Michael et al
> >
> > > Behalf Of Michael S. Tsirkin
> > > Sent: 9. marts 2018 22:24
> > >
> > > For a split ring, require that drivers use descriptors in order too.
> > > This allows devices to skip reading the available ring.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > [snip]
> > >
> > > +If VIRTIO_F_IN_ORDER has been negotiated, and when making a
> > > +descriptor with VRING_DESC_F_NEXT set in \field{flags} at offset
> > > +$x$ in the table available to the device, driver MUST set
> > > +\field{next} to $0$ for the last descriptor in the table (where $x
> > > += queue\_size - 1$) and to $x + 1$ for the rest of the descriptors.
> > > +
> > >  \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of
> > > a Virtio Device / Virtqueues / The Virtqueue Descriptor Table /
> > > Indirect Descriptors}
> > >
> > >  Some devices benefit by concurrently dispatching a large number @@
> > > -247,6
> > > +257,10 @@ chained by \field{next}. An indirect descriptor without a
> > > +valid
> > > \field{next}  A single indirect descriptor  table can include both
> > > device- readable and device-writable descriptors.
> > >
> > > +If VIRTIO_F_IN_ORDER has been negotiated, indirect descriptors use
> > > +sequential indices, in-order: index 0 followed by index 1 followed
> > > +by index 2, etc.
> > > +
> > >  \drivernormative{\paragraph}{Indirect Descriptors}{Basic Facilities
> > > of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table /
> > > Indirect Descriptors} The driver MUST NOT set the
> VIRTQ_DESC_F_INDIRECT flag unless the
> > >  VIRTIO_F_INDIRECT_DESC feature was negotiated.   The driver MUST
> NOT
> > > @@ -259,6 +273,10 @@ the device.
> > >  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and
> > > VIRTQ_DESC_F_NEXT  in \field{flags}.
> > >
> > > +If VIRTIO_F_IN_ORDER has been negotiated, indirect descriptors MUST
> > > +appear sequentially, with \field{next} taking the value of 1 for
> > > +the 1st descriptor, 2 for the 2nd one, etc.
> > > +
> > >  \devicenormative{\paragraph}{Indirect Descriptors}{Basic Facilities
> > > of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table /
> > > Indirect Descriptors} The device MUST ignore the write-only flag
> > > (\field{flags}\&VIRTQ_DESC_F_WRITE) in the descriptor that refers to
> > > an indirect table.
> > >
> >
> > The use of VIRTIO_F_IN_ORDER for split-ring can eliminate some accesses
> to the virtq_avail.ring and virtq_used.ring. However I'm wondering if the
> proposed descriptor ordering for multi-element buffers couldn't be tweaked
> to be more HW friendly.  Currently even with the VIRTIO_F_IN_ORDER
> negotiated, there is no way of knowing if, or how many chained descriptors
> follow the descriptor pointed to by the virtq_avail.idx. A chain has to be
> inspected one descriptor at a time until
> virtq_desc.flags[VIRTQ_DESC_F_NEXT]=0. This is awkward for HW offload,
> where you want to DMA all available descriptors in one shot, instead of
> iterating based on the contents of received DMA data. As currently defined,
> HW would have to find a compromise between likely chain length, and cost
> of additional DMA transfers. This leads to a performance penalty for all
> chained descriptors, and in case the length assumption is wrong the impact
> can be significant.
> >
> > Now, what if the VIRTIO_F_IN_ORDER instead required chained buffers to
> place the last element at the lowest index, and the head-element (to which
> virtq_avail.idx points) at the highest index? Then all the chained element
> descriptors would be included in a DMA of the descriptor table from the
> previous virtq_avail.idx+1 to the current virtq_avail.idx. The "backward"
> order of the chained descriptors shouldn't pose an issue as such (at least not
> in HW).
> >
> > Best Regards,
> >
> > -Lars
> 
> virtq_avail.idx is still an index into the available ring.
> 
> I don't really see how you can use virtq_avail.idx to guess the placement of a
> descriptor.
> 
> I suspect the best way to optimize this is to include the relevant data with the
> VIRTIO_F_NOTIFICATION_DATA feature.
> 

Argh, naturally. 

For HW offload I'd want to avoid notifications for buffer transfer from host to device, and hoped to just poll virtq_avail.idx directly.

A split virtqueue with VITRIO_F_IN_ORDER will maintain virtq_avail.idx==virtq_avail.ring[idx] as long as there is no chaining. It would be nice to allow negotiating away chaining, i.e add a VIRTIO_F_NO_CHAIN. If negotiated, the driver agrees not to use chaining, and as a result (of IN_ORDER and NO_CHAIN) both device and driver can ignore the virtq_avail.ring[].

> 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org