Re: [virtio-dev] Re: [PATCH v8 08/16] packed virtqueues: more efficient virtqueue layout

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 02/27/2018 11:23 AM, Jens Freimann wrote:
> On Mon, Feb 26, 2018 at 11:05:14PM +0200, Michael S. Tsirkin wrote:
>> On Mon, Feb 26, 2018 at 06:19:21PM +0100, Halil Pasic wrote:
>>> > +                vq->driver_event.flags = 0x1;
>>> > +                memory_barrier();
>>> > +
>>> > +                flags = d->flags;
>>> > +                bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
>>> > +                bool used = flags & (1 << VIRTQ_DESC_F_USED);
>>> > +                if (avail != used) {
>>> > +                        break;
>>> > +                }
>>> > +
>>> > +                vq->driver_event.flags = 0x2;
>>> > +        }
>>> > +
>>> > +    read_memory_barrier();
>>>
>>> Now with the condition avail != used a freshly (that is zero initialized)
>>> ring would appear all used. And we would do process_buffer(d) for the
>>> whole ring if this code happens to get executed. Do we have to make
>>> sure that this does not happen?
>>
>> I'll have to think about this.
> 
> With the wrap counter initialized to 1 descriptors would not be seen
> as used.

Looking at the code... In vhost:

+static bool desc_is_avail(struct vhost_virtqueue *vq,
+			  struct vring_desc_packed *desc)
+{
+	if (vq->used_wrap_counter)
+		if ((desc->flags & DESC_AVAIL) && !(desc->flags & DESC_USED))
+			return true;
+	if (vq->used_wrap_counter == false)
+		if (!(desc->flags & DESC_AVAIL) && (desc->flags & DESC_USED))
+			return true;
+
+	return false;
+}

Here the bit pattern corresponding to available depends on the
value of the wrap counter. I kind of anticipated this, but I could not
find it defined in this spec.

OTOH in guest:

+static inline bool more_used_packed(const struct vring_virtqueue *vq)
+{
+	u16 last_used, flags;
+	bool avail, used;
+
+	if (vq->vq.num_free == vq->vring.num)
+		return false;
+
+	last_used = vq->last_used_idx;
+	flags = virtio16_to_cpu(vq->vq.vdev,
+				vq->vring_packed.desc[last_used].flags);
+	avail = flags & VRING_DESC_F_AVAIL(1);
+	used = flags & VRING_DESC_F_USED(1);
+
+	return avail == used;
+}

if the next to be used descriptor is actually used does not depend on
any wrap counter, but has vq->vq.num_free == vq->vring.num as an extra
condition. This extra condition is basically 'there are outstanding
descriptors' and those are obviously either 'available' or yet to be observed
'used' descriptors. Right after initialization is covered by this extra
condition. And obviously if the descriptor in question is still available
flags & VRING_DESC_F_AVAIL(1) != flags & VRING_DESC_F_USED(1) holds, so
with the extra condition we are right there where we want to be.

But I could not find the extra condition in the spec.

With that said, I also want to point out that I don't understand
your statement Jens. I don't see a way to express the condition corresponding
to more_used_packed() using the driver wrap counter (avail_wrap_count).
Of course a wrap counter that wraps when last_used wraps could be used
to (but no such counter is mentioned here AFAIU).

>>
>>
>>> I was under the impression that this whole wrap counter exercise is
>>> to be able to distinguish these cases.
>>>
>>> BTW tools/virtio/ringtest/ring.c has a single flag bit to indicate
>>> available/used and does not have these wrap counters AFAIR.
>>
>> A single flag is fine if there's not s/g support and all descriptors are
>> written out.  Wrap counters are needed if we are to support skipping
>> descriptors because of s/g or in order.
>>
>>
>>> Also for split virtqueues  a descriptor has three possible states:
>>> * available
>>> * used
>>> * free
>>>
>>> I wonder if it's the same for packed, and if, how do I recognize
>>> free descriptors (that is descriptors that are neither available
>>> nor used.
>>
>> I'll think about this.
>>
>>> I'm pretty much confused on how this scheme with the available
>>> and used wrap counters (or device and driver wrap counters is
>>> supposed to work). A working implementation in C would really help
>>> me to understand this.
>>
>> DPDK based implementation has been posted.
> 
> vhost and guest drivers have also been posted.
> guest: https://lkml.org/lkml/2018/2/23/242
> vhost: https://lkml.org/lkml/2018/2/13/1102
> 

Thanks a lot. I've already found vhost myself but you saved me
some searching with the other one ;).

> regards,
> Jens
>>
>>> > +        process_buffer(d);
>>> > +        vq->next_used++;
>>> > +        if (vq->next_used >= vq->size) {
>>> > +                vq->next_used = 0;
>>> > +        }
>>> > +}
>>> > +\end{lstlisting}
>>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>
>