[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] Re: [PATCH v8 08/16] packed virtqueues: more efficient virtqueue layout
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 > Maintaining an internal "last used wrap counter" is one way to detect ring entry change. Another would be to maintain a per-entry "last used flag". I should probably do this in pseudo-code, and maybe add an implementation note. -- MST
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]