Re: [virtio-dev] packed ring layout proposal

[Paolo Bonzini <pbonzini@redhat.com>]

On 16/09/2016 00:39, Michael S. Tsirkin wrote:
> #define DESC_HW 0x0080

0x8000 seems easier to spot when looking at dumps.

> virtio 1.0 allows passing a batch of descriptors in both directions, by
> incrementing the used/avail index by values > 1.  We can support this by
> tagging first/middle/last descriptors in the flags field. For
> comptibility, polarity is reversed so bit is set for non-first and
> non-last descriptors in a batch:
> 
> #define BATCH_NOT_FIRST 0x0010
> #define BATCH_NOT_LAST  0x0020

What are the intended uses of this in the device and driver?  (Spoiler:
I think it's unnecessary)

- I cannot think of the desired meaning of BATCH_NOT_FIRST, it seems
like it shouldn't have any effect.

- For BATCH_NOT_LAST, you might not know that another descriptor will
have come at the time of the kick.  So, when the second descriptor in
the batch comes you have to go back and add BATCH_NOT_LAST to the
previous descriptor.  This is problematic because:

  * if you had already flipped DESC_HW, you aren't allowed to do that

  * if you do not flip DESC_HW as early as possible you have introduced
  additional latency for a polling-mode device

So BATCH_NOT_LAST is not necessary either, *but* when the device finds a
terminating descriptor (!VRING_DESC_F_NEXT or VRING_DESC_F_INDIRECT) the
device must look one element ahead in the ring to see if there's another
DESC_HW descriptor.  But hey, such lookahead is not any more inefficient
than separate memory locations for used index so it should be okay.  The
logic would be:

    read lookahead descriptor from ring
    while (DESC_HW set in descriptor) {
        do {
            advance internal used index
            if (not processing indirect desc. and VRING_DESC_F_INDIRECT)
                prepare to read first indirect descriptor
            else if (inside indirect descriptor) {
                check that VRING_DESC_F_INDIRECT is clear
                check that descriptor is valid
                add descriptor to request
                /* next test done based on legnth field of indirect
                   descriptor */
                if (last descriptor)
                    break
            } else {
                /* for polling mode device: wait until DESC_HW is set */
                check that DESC_HW is set
                check that descriptor is valid
                add descriptor to request
                if (!VRING_DESC_F_NEXT)
                    break
            }
            read next descriptor
        }
        process request
        read lookahead descriptor from ring
    }

BATCH_NOT_LAST is not necessary in the device->driver direction either.
And in this case, the driver knows the index of the last descriptor it
wrote, so it can avoid the lookahead.

<bad-idea>
I thought of adding BATCH_LAST for the driver->device direction only.
It would only be valid for a terminating descriptor, and it would tell
the device "Once I set DESC_HW in the following descriptor, there's
going to be a virtqueue kick".  BATCH_LAST would eliminate lookahead in
the device.  However, when the virtqueue kick is processed
asynchronously (this is the case when using KVM ioeventfd) it has a
great potential for introducing races.  So I don't like it either and
discourage introducing it.
</bad-idea>

> Also, length in the indirect descriptor should mark
> the list of the chain.
> 
> virtio 1.0 seems to allow a s/g entry followed by
> an indirect descriptor. This does not seem useful,
> so let's not allow that anymore.

Agreed.

> In the descriptors in the indirect buffers, I think we should drop index
> field altogether, just put s/g entries one after the other.

That causes worse packing and alignment (14 bytes per entry), so I'm not
sure it's a good idea.

Also, DESC_HW is not used by indirect descriptors, right?

Paolo