Re: [virtio-dev] packed ring layout proposal

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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

Attachment: signature.asc
Description: OpenPGP digital signature