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

[Jens Freimann <jfreimann@redhat.com>]

On Tue, Feb 27, 2018 at 06:03:01PM +0100, Halil Pasic wrote:
> On 02/27/2018 03:11 PM, Michael S. Tsirkin wrote:
> > > [..]
> > > > > > +
> > > > > > +\devicenormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table}
> > > > > > +A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT
> > > > > > +read a device-writable buffer.
> > > > > > +A device MUST NOT use a descriptor unless it observes
> > > > > > +VIRTQ_DESC_F_AVAIL bit in its \field{flags} being changed.
> > > > > I don't really understand this. How does the device observe
> > > > > the VIRTQ_DESC_F_AVAIL bit being changed?
> > > > By reading the descriptor.
> > > :) My point is: to observe a change one usually either needs at
> > > least one reading before and at least one reading after the change,
> > > or one needs to know that a certain reading means change. The latter
> > > is possible if we know that at the beginning of the time frame under
> > > consideration (t_0) only a certain set of values,let's say B like before,
> > > is possible, and after the change only a certain other set of values
> > > let's say A like after, is possible, and A and B are disjunctive (
> > > $A \cap B = \emtyset$).
> > Well each descriptor is read each time ring wraps around,
> > and the bit value changes each time ring wraps around.
> > For example device knows it's zero initialized so
> > if it reads bit value as 1 it knows the bit value has changed.
>
> Yeah I kind of understand but I would like having a more straightforward
> formulation here (than changes).
>
> BTW does this mean that the vhost implementation (that is:
>
> +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;
> +}
>
> ) is needlessly looking at the 'used' bit? (I think that is the case.)

Why do you think so? I assume with "used bit" you refer to the device
ring wrap counter. It needs to be looked at because if device looks
only at the descriptor flags it could be that they are different (so
available != used), but actually this descriptor was just skipped
previously. This could happen when descriptors are used in-order
(VIRTIO_F_IN_ORDER) and only the first descriptor of a batch is marked
as used. 

Should we mention that in the section about the counters?

regards,
Jens