Re: [virtio] 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

So the spec just says this: if you see avail flag change,
descriptor is available. If you see used flag change, it
is used.

As value of the flag is also known (equals the wrap bit)
that is one way to detect change.


> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe from this mail list, you must leave the OASIS TC that 
> generates this mail.  Follow this link to all your TCs in OASIS at:
> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php