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

[Jens Freimann <jfreimann@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).

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.


regards,
Jens