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

[Lars Ganrot <lga@napatech.com>]

> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: 29. marts 2018 16:42
> To: Lars Ganrot <lga@napatech.com>
> Cc: virtio@lists.oasis-open.org; virtio-dev@lists.oasis-open.org
> Subject: Re: [virtio-dev] [PATCH v10 13/13] split-ring: in order feature
> 
> On Wed, Mar 28, 2018 at 04:12:10PM +0000, Lars Ganrot wrote:
> > 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.
> 
> BTW, for split rings VIRTIO_F_NOTIFICATION_DATA just copies the index
> right now.
> 
> Do you have an opinion on whether we should change that for in-order?
> 

Maybe I should think more about this, however adding the last element descriptor index, would be useful to accelerate interfaces that frequently use chaining (from a HW DMA perspective at least).

> > 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[].
> 
> My point was that device can just assume no chains, and then fall back on
> doing extra reads upon encountering a chain.
>  

Yes, you are correct that the HW can speculatively use virtq_avail.idx as the direct index to the descriptor table, and if it encounters a chain, revert to using the virtq_avail.ring[] in the traditional way, and this would work without the feature-bit. 

However the driver would not be able to optimize away the writing of the virtq_avail.ring[] (=cache miss) unless a NO_CHAIN feature has been negotiated. The IN_ORDER by itself has already eliminated the need to maintain the TX virtq_used.ring[], since the buffer order is always known by the driver. With a NO_CHAIN feature-bit both RX and TX virtq_avail.ring[] related cache-misses could be eliminated. I.e. looping a packet over a split virtqueue would just experience 7 driver cache misses, down from 10 in Virtio v1.0. Multi-element buffers would still be possible provided INDIRECT is negotiated.

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