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

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

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).

I added such a counter in the pseudo-code.


> >>
> >>
> >>> 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
> >>
> > 
> 
> 
> ---------------------------------------------------------------------
> 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