[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] Re: [virtio-dev] Re: [PATCH v8 08/16] packed virtqueues: more efficient virtqueue layout
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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]