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

True. I note that dpdk used direct descriptors exclusively until
very recently.
    commit 6dc5de3a6aefba3946fe04368d93994db3f7a5fd
    Author: Stephen Hemminger <stephen@networkplumber.org>
    Date:   Fri Mar 4 10:19:19 2016 -0800

        virtio: use indirect ring elements

I'd like to make sure removing these doesn't cause regressions.
Stephen, could you please confirm that
1. with your patch, each packet takes at most 1 slot
2. your patch was a win for most workloads
   (as opposed to e.g. enabling/disabling this depending on workload)


Also, it does not look like vhost in dpdk supports
indirect descriptors.


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