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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 29, 2018 at 06:23:28PM +0000, Lars Ganrot wrote:
> 
> 
> > -----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.

Sorry that was not my idea.

Device should not need to read the ring at all.
It reads the descriptor table and counts the descriptors
without the next bit. Once the count reaches the available
index, it stops.


> However the driver would not be able to optimize away the writing of
> the virtq_avail.ring[] (=cache miss)


BTW writing is a separate question (there is no provision in
the spec to skip writes) but device does not have to read the ring.

Without device accesses ring will not be invaliated in cache
so no misses hopefully.

> 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.


NO_CHAIN might be a valid optimization, it is just unfortunately
somewhat narrow in that devices that need to mix write and read descriptors in
the same ring (e.g. storage) can not use this feature.


> >
> >
> > > >
> > > > > ------------------------------------------------------------------
> > > > > --- 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
> Disclaimer: This email and any files transmitted with it may contain confidential information intended for the addressee(s) only. The information is not to be surrendered or copied to unauthorized persons. If you have received this communication in error, please notify the sender immediately and delete this e-mail from your system.