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

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

On Wed, Feb 28, 2018 at 02:59:51PM +0100, Halil Pasic wrote:
> 
> 
> On 02/28/2018 02:42 PM, Jens Freimann wrote:
> > On Tue, Feb 27, 2018 at 12:29:11PM +0100, Halil Pasic wrote:
> >>
> >>
> >> 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).
> > 
> > Not sure I get this.
> > I was merely saying that when descriptor flags are initialized to 0
> > and the wrap counters to 1, then it is not the case that the driver
> > would see all descriptors as used because it takes the wrap counter
> > into account.
> > 
> 
> Please point me to the paragraph where it's written  how is the wrap
> counter to be taken into account when trying to determine if the
> desc_ring[last_used] (the descriptor we are polling) is used or not.
> 
> Nothing like that being specified (or at least I could not find it)
> was actually what I got hooked on.
> 
> Regards,
> Halil
> 

Maintaining an internal "last used wrap counter" is one way to
detect ring entry change. Another would be to maintain
a per-entry "last used flag".

I should probably do this in pseudo-code, and maybe add an
implementation note.

-- 
MST