Re: [virtio-dev] packed ring layout proposal

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

On Fri, Sep 16, 2016 at 12:11:59PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/09/2016 11:46, Stefan Hajnoczi wrote:
> >> > The idea is to have a r/w descriptor in a ring structure,
> >> > replacing the used and available ring, index and descriptor
> >> > buffer.
> > This is probably obvious but:
> > 
> > Why is the desc.index field needed?
> > 
> > The index field would be needed if the driver had to jump over a DESC_HW
> > descriptor when adding descriptors, but that doesn't seem necessary
> > since the device is supposed to overwrite descriptors in ascending ring
> > order when they complete out-of-order.
> 
> AIUI, when reading completed descriptors, the driver uses the index in
> order to map the descriptor to the original request.
> 
> It's more of an id; it's certainly not a linked list as in virtio 1.0 rings.

Yes.

> I think we can refine the description of the index, saying that only the
> first descriptor in a request needs an index; descriptors reached via
> VRING_DESC_F_NEXT do not need an index.  The advantage is that the
> device only needs to remember the descriptor of the first descriptor.

OTOH the driver will have to remember the id of the 1st
descriptor in the chain. Maybe require that all
descriptors in a chain have the same id. This seems better to me.

> However...
> 
> - is there any reason for the device or driver to even look at DESC_HW
> if VRING_DESC_F_NEXT is set?  DESC_HW only matters on the first buffer.
> 
> - why not drop VRING_DESC_F_NEXT altogether?
> 
> Because only two cases are strictly necessary:
> 
> - single-buffer requests.  These have some very important cases:
> virtio-rng, virtio-serial, virtio-net with merged rxbufs, virtio-scsi
> and virtio-vsock events, virtio-balloon, virtio-input all use
> single-buffer requests exclusively.
> 
> - indirect requests, where the length of the indirect descriptor can be
> used to figure out the number of buffers
> 
> Let's just drop the added complication of multi-buffer direct requests.
> Yes, this can cause one extra cache miss but 1) as shown above a lot of
> performance-critical cases use single-buffer requests 2) no one has ever
> bothered to use multi-buffer direct requests in Linux drivers, at least
> when indirect descriptors are available.
> 
> This simplifies the device logic like this
> 
>     read lookahead descriptor from ring
>     while (DESC_HW set in descriptor) {
>         advance internal used index
>         if VRING_DESC_F_INDIRECT
>             collect len/16 descriptors from address
>         else
>             use (len,addr,flags & VRING_DESC_F_WRITE) as a single buffer
>         process request
>         read lookahead descriptor from ring
>     }
> 
> and on completion
> 
>     if (next descriptor has DESC_HW set, i.e. ring full) {
>         set broken flag
>     } else {
>         write addr,index from original descriptor into addr,index
>         write number of written bytes into len
>         wmb
>         write flags from original descriptor & ~DESC_HW into flags
>     }
> 
> Paolo
>