On Wed, May 16, 2018 at 01:01:04PM +0800, Jason Wang wrote: > On 2018年04月25日 13:15, Tiwei Bie wrote: [...] > > @@ -1143,10 +1160,17 @@ static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq) > > /* We optimistically turn back on interrupts, then check if there was > > * more to do. */ > > + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to > > + * either clear the flags bit or point the event index at the next > > + * entry. Always update the event index to keep code simple. */ > > + > > + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev, > > + vq->last_used_idx | (vq->wrap_counter << 15)); > > > Using vq->wrap_counter seems not correct, what we need is the warp counter > for the last_used_idx not next_avail_idx. Yes, you're right. I have fixed it in my local repo, but haven't sent out a new version yet. I'll try to send out a new RFC today. > > And I think there's even no need to bother with event idx here, how about > just set VRING_EVENT_F_ENABLE? We had a similar discussion before. Michael prefers to use VRING_EVENT_F_DESC when possible to avoid extra interrupts if host is fast: https://lkml.org/lkml/2018/4/16/1085 """ I suspect this will lead to extra interrupts if host is fast. So I think for now we should always use VRING_EVENT_F_DESC if EVENT_IDX is negotiated. """ > > > if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) { > > virtio_wmb(vq->weak_barriers); > > - vq->event_flags_shadow = VRING_EVENT_F_ENABLE; > > + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC : > > + VRING_EVENT_F_ENABLE; > > vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev, > > vq->event_flags_shadow); > > } > > @@ -1172,15 +1196,34 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx) > > static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > + u16 bufs, used_idx, wrap_counter; > > START_USE(vq); > > /* We optimistically turn back on interrupts, then check if there was > > * more to do. */ > > + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to > > + * either clear the flags bit or point the event index at the next > > + * entry. Always update the event index to keep code simple. */ > > + > > + /* TODO: tune this threshold */ > > + bufs = (u16)(vq->next_avail_idx - vq->last_used_idx) * 3 / 4; > > bufs could be more than vq->num here, is this intended? Yes, you're right. Like the above one -- I have fixed it in my local repo, but haven't sent out a new version yet. Thanks for spotting this! > > > + > > + used_idx = vq->last_used_idx + bufs; > > + wrap_counter = vq->wrap_counter; > > + > > + if (used_idx >= vq->vring_packed.num) { > > + used_idx -= vq->vring_packed.num; > > + wrap_counter ^= 1; > > When used_idx is greater or equal vq->num, there's no need to flip > warp_counter bit since it should match next_avail_idx. > > And we need also care about the case when next_avail wraps but used_idx not. > so we probaly need: > > else if (vq->next_avail_idx < used_idx) { > wrap_counter ^= 1; > } > > I think maybe it's time to add some sample codes in the spec to avoid > duplicating the efforts(bugs). +1 Best regards, Tiwei Bie _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
