[RFC v3 4/5] virtio_ring: add event idx support in packed ring
This commit introduces the event idx support in packed ring. This feature is temporarily disabled, because the implementation in this patch may not work as expected, and some further discussions on the implementation are needed, e.g. do we have to check the wrap counter when checking whether a kick is needed? Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 53 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 0181e93897be..b1039c2985b9 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); - u16 flags; + u16 new, old, off_wrap, flags; bool needs_kick; u32 snapshot; @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) * suppressions. */ virtio_mb(vq->weak_barriers); + old = vq->next_avail_idx - vq->num_added; + new = vq->next_avail_idx; + vq->num_added = 0; + snapshot = *(u32 *)vq->vring_packed.device; + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; #ifdef DEBUG @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) vq->last_add_time_valid = false; #endif - needs_kick = (flags != VRING_EVENT_F_DISABLE); + if (flags == VRING_EVENT_F_DESC) + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old); + else + needs_kick = (flags != VRING_EVENT_F_DISABLE); END_USE(vq); return needs_kick; } @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, if (vq->last_used_idx >= vq->vring_packed.num) vq->last_used_idx -= vq->vring_packed.num; + /* If we expect an interrupt for the next entry, tell host +* by writing event index and flush out the write before +* the read in the next get_buf call. */ + if (vq->event_flags_shadow == VRING_EVENT_F_DESC) + virtio_store_mb(vq->weak_barriers, + &vq->vring_packed.driver->off_wrap, + cpu_to_virtio16(_vq->vdev, vq->last_used_idx | + (vq->wrap_counter << 15))); + #ifdef DEBUG vq->last_add_time_valid = false; #endif @@ -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)); 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; + + 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; + } + + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev, + used_idx | (wrap_counter << 15)); if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) { virtio_wmb(vq->weak_barriers); - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年04月25日 13:15, Tiwei Bie wrote: This commit introduces the event idx support in packed ring. This feature is temporarily disabled, because the implementation in this patch may not work as expected, and some further discussions on the implementation are needed, e.g. do we have to check the wrap counter when checking whether a kick is needed? Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 53 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 0181e93897be..b1039c2985b9 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); - u16 flags; + u16 new, old, off_wrap, flags; bool needs_kick; u32 snapshot; @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) * suppressions. */ virtio_mb(vq->weak_barriers); + old = vq->next_avail_idx - vq->num_added; + new = vq->next_avail_idx; + vq->num_added = 0; + snapshot = *(u32 *)vq->vring_packed.device; + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; #ifdef DEBUG @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) vq->last_add_time_valid = false; #endif - needs_kick = (flags != VRING_EVENT_F_DISABLE); + if (flags == VRING_EVENT_F_DESC) + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old); I wonder whether or not the math is correct. Both new and event are in the unit of descriptor ring size, but old looks not. Thanks + else + needs_kick = (flags != VRING_EVENT_F_DISABLE); END_USE(vq); return needs_kick; } @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, if (vq->last_used_idx >= vq->vring_packed.num) vq->last_used_idx -= vq->vring_packed.num; + /* If we expect an interrupt for the next entry, tell host +* by writing event index and flush out the write before +* the read in the next get_buf call. */ + if (vq->event_flags_shadow == VRING_EVENT_F_DESC) + virtio_store_mb(vq->weak_barriers, + &vq->vring_packed.driver->off_wrap, + cpu_to_virtio16(_vq->vdev, vq->last_used_idx | + (vq->wrap_counter << 15))); + #ifdef DEBUG vq->last_add_time_valid = false; #endif @@ -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)); 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; + + 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; + } + + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev, + used_idx | (wrap_counter << 15));
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > On 2018年04月25日 13:15, Tiwei Bie wrote: > > This commit introduces the event idx support in packed > > ring. This feature is temporarily disabled, because the > > implementation in this patch may not work as expected, > > and some further discussions on the implementation are > > needed, e.g. do we have to check the wrap counter when > > checking whether a kick is needed? > > > > Signed-off-by: Tiwei Bie > > --- > > drivers/virtio/virtio_ring.c | 53 > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 0181e93897be..b1039c2985b9 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue > > *_vq, > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > - u16 flags; > > + u16 new, old, off_wrap, flags; > > bool needs_kick; > > u32 snapshot; > > @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct > > virtqueue *_vq) > > * suppressions. */ > > virtio_mb(vq->weak_barriers); > > + old = vq->next_avail_idx - vq->num_added; > > + new = vq->next_avail_idx; > > + vq->num_added = 0; > > + > > snapshot = *(u32 *)vq->vring_packed.device; > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; > > #ifdef DEBUG > > @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct > > virtqueue *_vq) > > vq->last_add_time_valid = false; > > #endif > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > + if (flags == VRING_EVENT_F_DESC) > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old); > > I wonder whether or not the math is correct. Both new and event are in the > unit of descriptor ring size, but old looks not. What vring_need_event() cares is the distance between `new` and `old`, i.e. vq->num_added. So I think there is nothing wrong with `old`. But the calculation of the distance between `new` and `event_idx` isn't right when `new` wraps. How do you think about the below code: wrap_counter = off_wrap >> 15; event_idx = off_wrap & ~(1<<15); if (wrap_counter != vq->wrap_counter) event_idx -= vq->vring_packed.num; needs_kick = vring_need_event(event_idx, new, old); Best regards, Tiwei Bie > > Thanks > > > + else > > + needs_kick = (flags != VRING_EVENT_F_DISABLE); > > END_USE(vq); > > return needs_kick; > > } > > @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct > > virtqueue *_vq, > > if (vq->last_used_idx >= vq->vring_packed.num) > > vq->last_used_idx -= vq->vring_packed.num; > > + /* If we expect an interrupt for the next entry, tell host > > +* by writing event index and flush out the write before > > +* the read in the next get_buf call. */ > > + if (vq->event_flags_shadow == VRING_EVENT_F_DESC) > > + virtio_store_mb(vq->weak_barriers, > > + &vq->vring_packed.driver->off_wrap, > > + cpu_to_virtio16(_vq->vdev, vq->last_used_idx | > > + (vq->wrap_counter << 15))); > > + > > #ifdef DEBUG > > vq->last_add_time_valid = false; > > #endif > > @@ -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)); > > 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 optimisticall
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote: > On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > > On 2018年04月25日 13:15, Tiwei Bie wrote: > > > This commit introduces the event idx support in packed > > > ring. This feature is temporarily disabled, because the > > > implementation in this patch may not work as expected, > > > and some further discussions on the implementation are > > > needed, e.g. do we have to check the wrap counter when > > > checking whether a kick is needed? > > > > > > Signed-off-by: Tiwei Bie > > > --- > > > drivers/virtio/virtio_ring.c | 53 > > > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 0181e93897be..b1039c2985b9 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct > > > virtqueue *_vq, > > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > { > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > - u16 flags; > > > + u16 new, old, off_wrap, flags; > > > bool needs_kick; > > > u32 snapshot; > > > @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct > > > virtqueue *_vq) > > >* suppressions. */ > > > virtio_mb(vq->weak_barriers); > > > + old = vq->next_avail_idx - vq->num_added; > > > + new = vq->next_avail_idx; > > > + vq->num_added = 0; > > > + > > > snapshot = *(u32 *)vq->vring_packed.device; > > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); > > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; > > > #ifdef DEBUG > > > @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct > > > virtqueue *_vq) > > > vq->last_add_time_valid = false; > > > #endif > > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > + if (flags == VRING_EVENT_F_DESC) > > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old); > > > > I wonder whether or not the math is correct. Both new and event are in the > > unit of descriptor ring size, but old looks not. > > What vring_need_event() cares is the distance between > `new` and `old`, i.e. vq->num_added. So I think there > is nothing wrong with `old`. But the calculation of the > distance between `new` and `event_idx` isn't right when > `new` wraps. How do you think about the below code: > > wrap_counter = off_wrap >> 15; > event_idx = off_wrap & ~(1<<15); > if (wrap_counter != vq->wrap_counter) > event_idx -= vq->vring_packed.num; > > needs_kick = vring_need_event(event_idx, new, old); I suspect this hack won't work for non power of 2 ring. > Best regards, > Tiwei Bie > > > > > > Thanks > > > > > + else > > > + needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > END_USE(vq); > > > return needs_kick; > > > } > > > @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct > > > virtqueue *_vq, > > > if (vq->last_used_idx >= vq->vring_packed.num) > > > vq->last_used_idx -= vq->vring_packed.num; > > > + /* If we expect an interrupt for the next entry, tell host > > > + * by writing event index and flush out the write before > > > + * the read in the next get_buf call. */ > > > + if (vq->event_flags_shadow == VRING_EVENT_F_DESC) > > > + virtio_store_mb(vq->weak_barriers, > > > + &vq->vring_packed.driver->off_wrap, > > > + cpu_to_virtio16(_vq->vdev, vq->last_used_idx | > > > + (vq->wrap_counter << 15))); > > > + > > > #ifdef DEBUG > > > vq->last_add_time_valid = false; > > > #endif > > > @@ -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)); > > > 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, > > >
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Wed, May 02, 2018 at 04:51:01PM +0300, Michael S. Tsirkin wrote: > On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote: > > On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > > > On 2018年04月25日 13:15, Tiwei Bie wrote: > > > > This commit introduces the event idx support in packed > > > > ring. This feature is temporarily disabled, because the > > > > implementation in this patch may not work as expected, > > > > and some further discussions on the implementation are > > > > needed, e.g. do we have to check the wrap counter when > > > > checking whether a kick is needed? > > > > > > > > Signed-off-by: Tiwei Bie > > > > --- > > > > drivers/virtio/virtio_ring.c | 53 > > > > > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > index 0181e93897be..b1039c2985b9 100644 > > > > --- a/drivers/virtio/virtio_ring.c > > > > +++ b/drivers/virtio/virtio_ring.c > > > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct > > > > virtqueue *_vq, > > > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > { > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > - u16 flags; > > > > + u16 new, old, off_wrap, flags; > > > > bool needs_kick; > > > > u32 snapshot; > > > > @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct > > > > virtqueue *_vq) > > > > * suppressions. */ > > > > virtio_mb(vq->weak_barriers); > > > > + old = vq->next_avail_idx - vq->num_added; > > > > + new = vq->next_avail_idx; > > > > + vq->num_added = 0; > > > > + > > > > snapshot = *(u32 *)vq->vring_packed.device; > > > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); > > > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; > > > > #ifdef DEBUG > > > > @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct > > > > virtqueue *_vq) > > > > vq->last_add_time_valid = false; > > > > #endif > > > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > + if (flags == VRING_EVENT_F_DESC) > > > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, > > > > old); > > > > > > I wonder whether or not the math is correct. Both new and event are in the > > > unit of descriptor ring size, but old looks not. > > > > What vring_need_event() cares is the distance between > > `new` and `old`, i.e. vq->num_added. So I think there > > is nothing wrong with `old`. But the calculation of the > > distance between `new` and `event_idx` isn't right when > > `new` wraps. How do you think about the below code: > > > > wrap_counter = off_wrap >> 15; > > event_idx = off_wrap & ~(1<<15); > > if (wrap_counter != vq->wrap_counter) > > event_idx -= vq->vring_packed.num; > > > > needs_kick = vring_need_event(event_idx, new, old); > > I suspect this hack won't work for non power of 2 ring. Above code doesn't require the ring size to be a power of 2. For (__u16)(new_idx - old), what we want to get is vq->num_added. old = vq->next_avail_idx - vq->num_added; new = vq->next_avail_idx; When vq->next_avail_idx >= vq->num_added, it's obvious that, (__u16)(new_idx - old) is vq->num_added. And when vq->next_avail_idx < vq->num_added, new will be smaller than old (old will be a big unsigned number), but (__u16)(new_idx - old) is still vq->num_added. For (__u16)(new_idx - event_idx - 1), when new wraps and event_idx doesn't wrap, the most straightforward way to calculate it is: (new + vq->vring_packed.num) - event_idx - 1. But we can also calculate it in this way: event_idx -= vq->vring_packed.num; (event_idx will be a big unsigned number) Then (__u16)(new_idx - event_idx - 1) will be the value we want. Best regards, Tiwei Bie > > > > Best regards, > > Tiwei Bie > > > > > > > > > > Thanks > > > > > > > + else > > > > + needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > END_USE(vq); > > > > return needs_kick; > > > > } > > > > @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct > > > > virtqueue *_vq, > > > > if (vq->last_used_idx >= vq->vring_packed.num) > > > > vq->last_used_idx -= vq->vring_packed.num; > > > > + /* If we expect an interrupt for the next entry, tell host > > > > +* by writing event index and flush out the write before > > > > +* the read in the next get_buf call. */ > > > > + if (vq->event_flags_shadow == VRING_EVENT_F_DESC) > > > > + virtio_store_mb(vq->weak_barriers, > > > > + &vq->vring_packed.driver->off_wrap, > > > > + cpu_to_virtio16(_vq->vdev, > > > > vq->last_used_idx | > > > > + (vq-
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Wed, May 02, 2018 at 11:12:55PM +0800, Tiwei Bie wrote: > On Wed, May 02, 2018 at 04:51:01PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote: > > > On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > > > > On 2018年04月25日 13:15, Tiwei Bie wrote: > > > > > This commit introduces the event idx support in packed > > > > > ring. This feature is temporarily disabled, because the > > > > > implementation in this patch may not work as expected, > > > > > and some further discussions on the implementation are > > > > > needed, e.g. do we have to check the wrap counter when > > > > > checking whether a kick is needed? > > > > > > > > > > Signed-off-by: Tiwei Bie > > > > > --- > > > > > drivers/virtio/virtio_ring.c | 53 > > > > > > > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c > > > > > b/drivers/virtio/virtio_ring.c > > > > > index 0181e93897be..b1039c2985b9 100644 > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct > > > > > virtqueue *_vq, > > > > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > { > > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > > - u16 flags; > > > > > + u16 new, old, off_wrap, flags; > > > > > bool needs_kick; > > > > > u32 snapshot; > > > > > @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct > > > > > virtqueue *_vq) > > > > >* suppressions. */ > > > > > virtio_mb(vq->weak_barriers); > > > > > + old = vq->next_avail_idx - vq->num_added; > > > > > + new = vq->next_avail_idx; > > > > > + vq->num_added = 0; > > > > > + > > > > > snapshot = *(u32 *)vq->vring_packed.device; > > > > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); > > > > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; > > > > > #ifdef DEBUG > > > > > @@ -1006,7 +1011,10 @@ static bool > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > vq->last_add_time_valid = false; > > > > > #endif > > > > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > > + if (flags == VRING_EVENT_F_DESC) > > > > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, > > > > > old); > > > > > > > > I wonder whether or not the math is correct. Both new and event are in > > > > the > > > > unit of descriptor ring size, but old looks not. > > > > > > What vring_need_event() cares is the distance between > > > `new` and `old`, i.e. vq->num_added. So I think there > > > is nothing wrong with `old`. But the calculation of the > > > distance between `new` and `event_idx` isn't right when > > > `new` wraps. How do you think about the below code: > > > > > > wrap_counter = off_wrap >> 15; > > > event_idx = off_wrap & ~(1<<15); > > > if (wrap_counter != vq->wrap_counter) > > > event_idx -= vq->vring_packed.num; > > > > > > needs_kick = vring_need_event(event_idx, new, old); > > > > I suspect this hack won't work for non power of 2 ring. > > Above code doesn't require the ring size to be a power of 2. > > For (__u16)(new_idx - old), what we want to get is vq->num_added. > > old = vq->next_avail_idx - vq->num_added; > new = vq->next_avail_idx; > > When vq->next_avail_idx >= vq->num_added, it's obvious that, > (__u16)(new_idx - old) is vq->num_added. > > And when vq->next_avail_idx < vq->num_added, new will be smaller > than old (old will be a big unsigned number), but (__u16)(new_idx > - old) is still vq->num_added. > > For (__u16)(new_idx - event_idx - 1), when new wraps and event_idx > doesn't wrap, the most straightforward way to calculate it is: > (new + vq->vring_packed.num) - event_idx - 1. So how about we use the straightforward way then? > But we can also calculate it in this way: > > event_idx -= vq->vring_packed.num; > (event_idx will be a big unsigned number) > > Then (__u16)(new_idx - event_idx - 1) will be the value we want. > > Best regards, > Tiwei Bie > > > > > > > Best regards, > > > Tiwei Bie > > > > > > > > > > > > > > Thanks > > > > > > > > > + else > > > > > + needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > > END_USE(vq); > > > > > return needs_kick; > > > > > } > > > > > @@ -1116,6 +1124,15 @@ static void > > > > > *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > if (vq->last_used_idx >= vq->vring_packed.num) > > > > > vq->last_used_idx -= vq->vring_packed.num; > > > > > + /* If we expect an interrupt for the next entry, tell host > > > > > + * by writing event index and flush out the write before > > > > > + * the read in the next get_buf call. */ > > > > > + if (vq->event_flags_shadow == VRING_EVENT_
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Wed, May 02, 2018 at 06:42:57PM +0300, Michael S. Tsirkin wrote: > On Wed, May 02, 2018 at 11:12:55PM +0800, Tiwei Bie wrote: > > On Wed, May 02, 2018 at 04:51:01PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote: > > > > On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > > > > > On 2018年04月25日 13:15, Tiwei Bie wrote: > > > > > > This commit introduces the event idx support in packed > > > > > > ring. This feature is temporarily disabled, because the > > > > > > implementation in this patch may not work as expected, > > > > > > and some further discussions on the implementation are > > > > > > needed, e.g. do we have to check the wrap counter when > > > > > > checking whether a kick is needed? > > > > > > > > > > > > Signed-off-by: Tiwei Bie > > > > > > --- > > > > > > drivers/virtio/virtio_ring.c | 53 > > > > > > > > > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c > > > > > > b/drivers/virtio/virtio_ring.c > > > > > > index 0181e93897be..b1039c2985b9 100644 > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct > > > > > > virtqueue *_vq, > > > > > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > { > > > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > > > - u16 flags; > > > > > > + u16 new, old, off_wrap, flags; > > > > > > bool needs_kick; > > > > > > u32 snapshot; > > > > > > @@ -995,7 +995,12 @@ static bool > > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > * suppressions. */ > > > > > > virtio_mb(vq->weak_barriers); > > > > > > + old = vq->next_avail_idx - vq->num_added; > > > > > > + new = vq->next_avail_idx; > > > > > > + vq->num_added = 0; > > > > > > + > > > > > > snapshot = *(u32 *)vq->vring_packed.device; > > > > > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); > > > > > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; > > > > > > #ifdef DEBUG > > > > > > @@ -1006,7 +1011,10 @@ static bool > > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > vq->last_add_time_valid = false; > > > > > > #endif > > > > > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > > > + if (flags == VRING_EVENT_F_DESC) > > > > > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, > > > > > > old); > > > > > > > > > > I wonder whether or not the math is correct. Both new and event are > > > > > in the > > > > > unit of descriptor ring size, but old looks not. > > > > > > > > What vring_need_event() cares is the distance between > > > > `new` and `old`, i.e. vq->num_added. So I think there > > > > is nothing wrong with `old`. But the calculation of the > > > > distance between `new` and `event_idx` isn't right when > > > > `new` wraps. How do you think about the below code: > > > > > > > > wrap_counter = off_wrap >> 15; > > > > event_idx = off_wrap & ~(1<<15); > > > > if (wrap_counter != vq->wrap_counter) > > > > event_idx -= vq->vring_packed.num; > > > > > > > > needs_kick = vring_need_event(event_idx, new, old); > > > > > > I suspect this hack won't work for non power of 2 ring. > > > > Above code doesn't require the ring size to be a power of 2. > > > > For (__u16)(new_idx - old), what we want to get is vq->num_added. > > > > old = vq->next_avail_idx - vq->num_added; > > new = vq->next_avail_idx; > > > > When vq->next_avail_idx >= vq->num_added, it's obvious that, > > (__u16)(new_idx - old) is vq->num_added. > > > > And when vq->next_avail_idx < vq->num_added, new will be smaller > > than old (old will be a big unsigned number), but (__u16)(new_idx > > - old) is still vq->num_added. > > > > For (__u16)(new_idx - event_idx - 1), when new wraps and event_idx > > doesn't wrap, the most straightforward way to calculate it is: > > (new + vq->vring_packed.num) - event_idx - 1. > > So how about we use the straightforward way then? You mean we do new += vq->vring_packed.num instead of event_idx -= vq->vring_packed.num before calling vring_need_event()? The problem is that, the second param (new_idx) of vring_need_event() will be used for: (__u16)(new_idx - event_idx - 1) (__u16)(new_idx - old) So if we change new, we will need to change old too. And that would be an ugly hack.. Best regards, Tiwei Bie > > > But we can also calculate it in this way: > > > > event_idx -= vq->vring_packed.num; > > (event_idx will be a big unsigned number) > > > > Then (__u16)(new_idx - event_idx - 1) will be the value we want. > > > > Best regards, > > Tiwei Bie > > > > > > > > > > > > Best regards, > > > > Tiwei Bie > > > > > > > > > > > > > > > > > > Thanks > > >
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Thu, May 03, 2018 at 09:11:16AM +0800, Tiwei Bie wrote: > On Wed, May 02, 2018 at 06:42:57PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 02, 2018 at 11:12:55PM +0800, Tiwei Bie wrote: > > > On Wed, May 02, 2018 at 04:51:01PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote: > > > > > On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > > > > > > On 2018年04月25日 13:15, Tiwei Bie wrote: > > > > > > > This commit introduces the event idx support in packed > > > > > > > ring. This feature is temporarily disabled, because the > > > > > > > implementation in this patch may not work as expected, > > > > > > > and some further discussions on the implementation are > > > > > > > needed, e.g. do we have to check the wrap counter when > > > > > > > checking whether a kick is needed? > > > > > > > > > > > > > > Signed-off-by: Tiwei Bie > > > > > > > --- > > > > > > > drivers/virtio/virtio_ring.c | 53 > > > > > > > > > > > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c > > > > > > > b/drivers/virtio/virtio_ring.c > > > > > > > index 0181e93897be..b1039c2985b9 100644 > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct > > > > > > > virtqueue *_vq, > > > > > > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > > { > > > > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > > > > - u16 flags; > > > > > > > + u16 new, old, off_wrap, flags; > > > > > > > bool needs_kick; > > > > > > > u32 snapshot; > > > > > > > @@ -995,7 +995,12 @@ static bool > > > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > >* suppressions. */ > > > > > > > virtio_mb(vq->weak_barriers); > > > > > > > + old = vq->next_avail_idx - vq->num_added; > > > > > > > + new = vq->next_avail_idx; > > > > > > > + vq->num_added = 0; > > > > > > > + > > > > > > > snapshot = *(u32 *)vq->vring_packed.device; > > > > > > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); > > > > > > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & > > > > > > > 0x3; > > > > > > > #ifdef DEBUG > > > > > > > @@ -1006,7 +1011,10 @@ static bool > > > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > > vq->last_add_time_valid = false; > > > > > > > #endif > > > > > > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > > > > + if (flags == VRING_EVENT_F_DESC) > > > > > > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, > > > > > > > old); > > > > > > > > > > > > I wonder whether or not the math is correct. Both new and event are > > > > > > in the > > > > > > unit of descriptor ring size, but old looks not. > > > > > > > > > > What vring_need_event() cares is the distance between > > > > > `new` and `old`, i.e. vq->num_added. So I think there > > > > > is nothing wrong with `old`. But the calculation of the > > > > > distance between `new` and `event_idx` isn't right when > > > > > `new` wraps. How do you think about the below code: > > > > > > > > > > wrap_counter = off_wrap >> 15; > > > > > event_idx = off_wrap & ~(1<<15); > > > > > if (wrap_counter != vq->wrap_counter) > > > > > event_idx -= vq->vring_packed.num; > > > > > > > > > > needs_kick = vring_need_event(event_idx, new, old); > > > > > > > > I suspect this hack won't work for non power of 2 ring. > > > > > > Above code doesn't require the ring size to be a power of 2. > > > > > > For (__u16)(new_idx - old), what we want to get is vq->num_added. > > > > > > old = vq->next_avail_idx - vq->num_added; > > > new = vq->next_avail_idx; > > > > > > When vq->next_avail_idx >= vq->num_added, it's obvious that, > > > (__u16)(new_idx - old) is vq->num_added. > > > > > > And when vq->next_avail_idx < vq->num_added, new will be smaller > > > than old (old will be a big unsigned number), but (__u16)(new_idx > > > - old) is still vq->num_added. > > > > > > For (__u16)(new_idx - event_idx - 1), when new wraps and event_idx > > > doesn't wrap, the most straightforward way to calculate it is: > > > (new + vq->vring_packed.num) - event_idx - 1. > > > > So how about we use the straightforward way then? > > You mean we do new += vq->vring_packed.num instead > of event_idx -= vq->vring_packed.num before calling > vring_need_event()? > > The problem is that, the second param (new_idx) of > vring_need_event() will be used for: > > (__u16)(new_idx - event_idx - 1) > (__u16)(new_idx - old) > > So if we change new, we will need to change old too. I think that since we have a branch there anyway, we are better off just special-casing if (wrap_counter != vq->wrap_cou
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Thu, May 03, 2018 at 04:44:39AM +0300, Michael S. Tsirkin wrote: > On Thu, May 03, 2018 at 09:11:16AM +0800, Tiwei Bie wrote: > > On Wed, May 02, 2018 at 06:42:57PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 02, 2018 at 11:12:55PM +0800, Tiwei Bie wrote: > > > > On Wed, May 02, 2018 at 04:51:01PM +0300, Michael S. Tsirkin wrote: > > > > > On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote: > > > > > > On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote: > > > > > > > On 2018年04月25日 13:15, Tiwei Bie wrote: > > > > > > > > This commit introduces the event idx support in packed > > > > > > > > ring. This feature is temporarily disabled, because the > > > > > > > > implementation in this patch may not work as expected, > > > > > > > > and some further discussions on the implementation are > > > > > > > > needed, e.g. do we have to check the wrap counter when > > > > > > > > checking whether a kick is needed? > > > > > > > > > > > > > > > > Signed-off-by: Tiwei Bie > > > > > > > > --- > > > > > > > > drivers/virtio/virtio_ring.c | 53 > > > > > > > > > > > > > > > > 1 file changed, 49 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c > > > > > > > > b/drivers/virtio/virtio_ring.c > > > > > > > > index 0181e93897be..b1039c2985b9 100644 > > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > > @@ -986,7 +986,7 @@ static inline int > > > > > > > > virtqueue_add_packed(struct virtqueue *_vq, > > > > > > > > static bool virtqueue_kick_prepare_packed(struct virtqueue > > > > > > > > *_vq) > > > > > > > > { > > > > > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > > > > > - u16 flags; > > > > > > > > + u16 new, old, off_wrap, flags; > > > > > > > > bool needs_kick; > > > > > > > > u32 snapshot; > > > > > > > > @@ -995,7 +995,12 @@ static bool > > > > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > > > * suppressions. */ > > > > > > > > virtio_mb(vq->weak_barriers); > > > > > > > > + old = vq->next_avail_idx - vq->num_added; > > > > > > > > + new = vq->next_avail_idx; > > > > > > > > + vq->num_added = 0; > > > > > > > > + > > > > > > > > snapshot = *(u32 *)vq->vring_packed.device; > > > > > > > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & > > > > > > > > 0x); > > > > > > > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & > > > > > > > > 0x3; > > > > > > > > #ifdef DEBUG > > > > > > > > @@ -1006,7 +1011,10 @@ static bool > > > > > > > > virtqueue_kick_prepare_packed(struct virtqueue *_vq) > > > > > > > > vq->last_add_time_valid = false; > > > > > > > > #endif > > > > > > > > - needs_kick = (flags != VRING_EVENT_F_DISABLE); > > > > > > > > + if (flags == VRING_EVENT_F_DESC) > > > > > > > > + needs_kick = vring_need_event(off_wrap & > > > > > > > > ~(1<<15), new, old); > > > > > > > > > > > > > > I wonder whether or not the math is correct. Both new and event > > > > > > > are in the > > > > > > > unit of descriptor ring size, but old looks not. > > > > > > > > > > > > What vring_need_event() cares is the distance between > > > > > > `new` and `old`, i.e. vq->num_added. So I think there > > > > > > is nothing wrong with `old`. But the calculation of the > > > > > > distance between `new` and `event_idx` isn't right when > > > > > > `new` wraps. How do you think about the below code: > > > > > > > > > > > > wrap_counter = off_wrap >> 15; > > > > > > event_idx = off_wrap & ~(1<<15); > > > > > > if (wrap_counter != vq->wrap_counter) > > > > > > event_idx -= vq->vring_packed.num; > > > > > > > > > > > > needs_kick = vring_need_event(event_idx, new, old); > > > > > > > > > > I suspect this hack won't work for non power of 2 ring. > > > > > > > > Above code doesn't require the ring size to be a power of 2. > > > > > > > > For (__u16)(new_idx - old), what we want to get is vq->num_added. > > > > > > > > old = vq->next_avail_idx - vq->num_added; > > > > new = vq->next_avail_idx; > > > > > > > > When vq->next_avail_idx >= vq->num_added, it's obvious that, > > > > (__u16)(new_idx - old) is vq->num_added. > > > > > > > > And when vq->next_avail_idx < vq->num_added, new will be smaller > > > > than old (old will be a big unsigned number), but (__u16)(new_idx > > > > - old) is still vq->num_added. > > > > > > > > For (__u16)(new_idx - event_idx - 1), when new wraps and event_idx > > > > doesn't wrap, the most straightforward way to calculate it is: > > > > (new + vq->vring_packed.num) - event_idx - 1. > > > > > > So how about we use the straightforward way then? > > > > You mean we do new += vq->vring_packed.num instead > > of event_idx -= vq->vring_packed.num before calling > > vring_need_event()? > > > > The
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年05月03日 10:09, Tiwei Bie wrote: So how about we use the straightforward way then? You mean we do new += vq->vring_packed.num instead of event_idx -= vq->vring_packed.num before calling vring_need_event()? The problem is that, the second param (new_idx) of vring_need_event() will be used for: (__u16)(new_idx - event_idx - 1) (__u16)(new_idx - old) So if we change new, we will need to change old too. I think that since we have a branch there anyway, we are better off just special-casing if (wrap_counter != vq->wrap_counter). Treat is differenty and avoid casts. And that would be an ugly hack.. Best regards, Tiwei Bie I consider casts and huge numbers with two's complement games even uglier. The dependency on two's complement game is introduced since the split ring. In packed ring, old is calculated via: old = vq->next_avail_idx - vq->num_added; In split ring, old is calculated via: old = vq->avail_idx_shadow - vq->num_added; In both cases, when vq->num_added is bigger, old will be a big number. Best regards, Tiwei Bie How about just do something like vhost: static u16 vhost_idx_diff(struct vhost_virtqueue *vq, u16 old, u16 new) { if (new > old) return new - old; return (new + vq->num - old); } static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 event_off, __u16 new, __u16 old) { return (__u16)(vhost_idx_diff(vq, new, event_off) - 1) < (__u16)vhost_idx_diff(vq, new, old); } ?
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Thu, May 03, 2018 at 03:25:29PM +0800, Jason Wang wrote: > On 2018年05月03日 10:09, Tiwei Bie wrote: > > > > > So how about we use the straightforward way then? > > > > You mean we do new += vq->vring_packed.num instead > > > > of event_idx -= vq->vring_packed.num before calling > > > > vring_need_event()? > > > > > > > > The problem is that, the second param (new_idx) of > > > > vring_need_event() will be used for: > > > > > > > > (__u16)(new_idx - event_idx - 1) > > > > (__u16)(new_idx - old) > > > > > > > > So if we change new, we will need to change old too. > > > I think that since we have a branch there anyway, > > > we are better off just special-casing if (wrap_counter != > > > vq->wrap_counter). > > > Treat is differenty and avoid casts. > > > > > > > And that would be an ugly hack.. > > > > > > > > Best regards, > > > > Tiwei Bie > > > I consider casts and huge numbers with two's complement > > > games even uglier. > > The dependency on two's complement game is introduced > > since the split ring. > > > > In packed ring, old is calculated via: > > > > old = vq->next_avail_idx - vq->num_added; > > > > In split ring, old is calculated via: > > > > old = vq->avail_idx_shadow - vq->num_added; > > > > In both cases, when vq->num_added is bigger, old will > > be a big number. > > > > Best regards, > > Tiwei Bie > > > > How about just do something like vhost: > > static u16 vhost_idx_diff(struct vhost_virtqueue *vq, u16 old, u16 new) > { > if (new > old) > return new - old; > return (new + vq->num - old); > } > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > __u16 event_off, __u16 new, > __u16 old) > { > return (__u16)(vhost_idx_diff(vq, new, event_off) - 1) < > (__u16)vhost_idx_diff(vq, new, old); > } > > ? It seems that there is a typo in above code. The second param of vhost_idx_diff() is `old`, but when calling this function in vhost_vring_packed_need_event(), `new` is passed as the second param. If we assume the second param of vhost_idx_diff() is new and the third one is old, i.e.: static u16 vhost_idx_diff(struct vhost_virtqueue *vq, u16 new, u16 old) { if (new > old) return new - old; return (new + vq->num - old); } I think it's still not right. Because in virtqueue_enable_cb_delayed(), we may set an event_off which is bigger than new and both of them have wrapped. And in this case, although new is smaller than event_off (i.e. the third param -- old), new shouldn't add vq->num, and actually we are expecting a very big idx diff. Best regards, Tiwei Bie
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年05月03日 21:54, Tiwei Bie wrote: On Thu, May 03, 2018 at 03:25:29PM +0800, Jason Wang wrote: On 2018年05月03日 10:09, Tiwei Bie wrote: So how about we use the straightforward way then? You mean we do new += vq->vring_packed.num instead of event_idx -= vq->vring_packed.num before calling vring_need_event()? The problem is that, the second param (new_idx) of vring_need_event() will be used for: (__u16)(new_idx - event_idx - 1) (__u16)(new_idx - old) So if we change new, we will need to change old too. I think that since we have a branch there anyway, we are better off just special-casing if (wrap_counter != vq->wrap_counter). Treat is differenty and avoid casts. And that would be an ugly hack.. Best regards, Tiwei Bie I consider casts and huge numbers with two's complement games even uglier. The dependency on two's complement game is introduced since the split ring. In packed ring, old is calculated via: old = vq->next_avail_idx - vq->num_added; In split ring, old is calculated via: old = vq->avail_idx_shadow - vq->num_added; In both cases, when vq->num_added is bigger, old will be a big number. Best regards, Tiwei Bie How about just do something like vhost: static u16 vhost_idx_diff(struct vhost_virtqueue *vq, u16 old, u16 new) { if (new > old) return new - old; return (new + vq->num - old); } static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 event_off, __u16 new, __u16 old) { return (__u16)(vhost_idx_diff(vq, new, event_off) - 1) < (__u16)vhost_idx_diff(vq, new, old); } ? It seems that there is a typo in above code. The second param of vhost_idx_diff() is `old`, but when calling this function in vhost_vring_packed_need_event(), `new` is passed as the second param. Right. If we assume the second param of vhost_idx_diff() is new and the third one is old, i.e.: static u16 vhost_idx_diff(struct vhost_virtqueue *vq, u16 new, u16 old) { if (new > old) return new - old; return (new + vq->num - old); } I think it's still not right. Because in virtqueue_enable_cb_delayed(), we may set an event_off which is bigger than new and both of them have wrapped. And in this case, although new is smaller than event_off (i.e. the third param -- old), new shouldn't add vq->num, and actually we are expecting a very big idx diff. Yes, so to calculate distance correctly between event and new, we just need to compare the warp counter and return false if it doesn't match without the need to try to add vq.num here. Thanks Best regards, Tiwei Bie
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年05月08日 11:05, Jason Wang wrote: Because in virtqueue_enable_cb_delayed(), we may set an event_off which is bigger than new and both of them have wrapped. And in this case, although new is smaller than event_off (i.e. the third param -- old), new shouldn't add vq->num, and actually we are expecting a very big idx diff. Yes, so to calculate distance correctly between event and new, we just need to compare the warp counter and return false if it doesn't match without the need to try to add vq.num here. Thanks Sorry, looks like the following should work, we need add vq.num if used_wrap_counter does not match: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = off_wrap >> 15; int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (wrap != vq->used_wrap_counter) d1 = new + vq->num - off - 1; else d1 = new - off - 1; if (new > old) d2 = new - old; else d2 = new + vq->num - old; return d1 < d2; } Thanks
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: > On 2018年05月08日 11:05, Jason Wang wrote: > > > > > > Because in virtqueue_enable_cb_delayed(), we may set an > > > event_off which is bigger than new and both of them have > > > wrapped. And in this case, although new is smaller than > > > event_off (i.e. the third param -- old), new shouldn't > > > add vq->num, and actually we are expecting a very big > > > idx diff. > > > > Yes, so to calculate distance correctly between event and new, we just > > need to compare the warp counter and return false if it doesn't match > > without the need to try to add vq.num here. > > > > Thanks > > Sorry, looks like the following should work, we need add vq.num if > used_wrap_counter does not match: > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > __u16 off_wrap, __u16 new, > __u16 old) > { > bool wrap = off_wrap >> 15; > int off = off_wrap & ~(1 << 15); > __u16 d1, d2; > > if (wrap != vq->used_wrap_counter) > d1 = new + vq->num - off - 1; Just to draw your attention (maybe you have already noticed this). In this case (i.e. wrap != vq->used_wrap_counter), it's also possible that (off < new) is true. Because, when virtqueue_enable_cb_delayed_packed() is used, `off` is calculated in driver in a way like this: off = vq->last_used_idx + bufs; if (off >= vq->vring_packed.num) { off -= vq->vring_packed.num; wrap_counter ^= 1; } And when `new` (in vhost) is close to vq->num. The vq->last_used_idx + bufs (in driver) can be bigger than vq->vring_packed.num, and: 1. `off` will wrap; 2. wrap counters won't match; 3. off < new; And d1 (i.e. new + vq->num - off - 1) will be a value bigger than vq->num. I'm okay with this, although it's a bit weird. Best regards, Tiwei Bie > else > d1 = new - off - 1; > > if (new > old) > d2 = new - old; > else > d2 = new + vq->num - old; > > return d1 < d2; > } > > Thanks >
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年05月08日 14:44, Tiwei Bie wrote: On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: On 2018年05月08日 11:05, Jason Wang wrote: Because in virtqueue_enable_cb_delayed(), we may set an event_off which is bigger than new and both of them have wrapped. And in this case, although new is smaller than event_off (i.e. the third param -- old), new shouldn't add vq->num, and actually we are expecting a very big idx diff. Yes, so to calculate distance correctly between event and new, we just need to compare the warp counter and return false if it doesn't match without the need to try to add vq.num here. Thanks Sorry, looks like the following should work, we need add vq.num if used_wrap_counter does not match: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = off_wrap >> 15; int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (wrap != vq->used_wrap_counter) d1 = new + vq->num - off - 1; Just to draw your attention (maybe you have already noticed this). I miss this, thanks! In this case (i.e. wrap != vq->used_wrap_counter), it's also possible that (off < new) is true. Because, when virtqueue_enable_cb_delayed_packed() is used, `off` is calculated in driver in a way like this: off = vq->last_used_idx + bufs; if (off >= vq->vring_packed.num) { off -= vq->vring_packed.num; wrap_counter ^= 1; } And when `new` (in vhost) is close to vq->num. The vq->last_used_idx + bufs (in driver) can be bigger than vq->vring_packed.num, and: 1. `off` will wrap; 2. wrap counters won't match; 3. off < new; And d1 (i.e. new + vq->num - off - 1) will be a value bigger than vq->num. I'm okay with this, although it's a bit weird. So I'm considering something more compact by reusing vring_need_event() by pretending a larger queue size and adding vq->num back when necessary: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = vq->used_wrap_counter; int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (new < old) { new += vq->num; wrap ^= 1; } if (wrap != off_wrap >> 15) off += vq->num; return vring_need_event(off, new, old); } Best regards, Tiwei Bie else d1 = new - off - 1; if (new > old) d2 = new - old; else d2 = new + vq->num - old; return d1 < d2; } Thanks
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Tue, May 08, 2018 at 03:16:53PM +0800, Jason Wang wrote: > On 2018年05月08日 14:44, Tiwei Bie wrote: > > On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: > > > On 2018年05月08日 11:05, Jason Wang wrote: > > > > > Because in virtqueue_enable_cb_delayed(), we may set an > > > > > event_off which is bigger than new and both of them have > > > > > wrapped. And in this case, although new is smaller than > > > > > event_off (i.e. the third param -- old), new shouldn't > > > > > add vq->num, and actually we are expecting a very big > > > > > idx diff. > > > > Yes, so to calculate distance correctly between event and new, we just > > > > need to compare the warp counter and return false if it doesn't match > > > > without the need to try to add vq.num here. > > > > > > > > Thanks > > > Sorry, looks like the following should work, we need add vq.num if > > > used_wrap_counter does not match: > > > > > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > > > __u16 off_wrap, __u16 new, > > > __u16 old) > > > { > > > bool wrap = off_wrap >> 15; > > > int off = off_wrap & ~(1 << 15); > > > __u16 d1, d2; > > > > > > if (wrap != vq->used_wrap_counter) > > > d1 = new + vq->num - off - 1; > > Just to draw your attention (maybe you have already > > noticed this). > > I miss this, thanks! > > > > > In this case (i.e. wrap != vq->used_wrap_counter), > > it's also possible that (off < new) is true. Because, > > > > when virtqueue_enable_cb_delayed_packed() is used, > > `off` is calculated in driver in a way like this: > > > > off = vq->last_used_idx + bufs; > > if (off >= vq->vring_packed.num) { > > off -= vq->vring_packed.num; > > wrap_counter ^= 1; > > } > > > > And when `new` (in vhost) is close to vq->num. The > > vq->last_used_idx + bufs (in driver) can be bigger > > than vq->vring_packed.num, and: > > > > 1. `off` will wrap; > > 2. wrap counters won't match; > > 3. off < new; > > > > And d1 (i.e. new + vq->num - off - 1) will be a value > > bigger than vq->num. I'm okay with this, although it's > > a bit weird. > > > So I'm considering something more compact by reusing vring_need_event() by > pretending a larger queue size and adding vq->num back when necessary: > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > __u16 off_wrap, __u16 new, > __u16 old) > { > bool wrap = vq->used_wrap_counter; If the wrap counter is obtained from the vq, I think `new` should also be obtained from the vq. Or the wrap counter should be carried in `new`. > int off = off_wrap & ~(1 << 15); > __u16 d1, d2; > > if (new < old) { > new += vq->num; > wrap ^= 1; > } > > if (wrap != off_wrap >> 15) > off += vq->num; When `new` and `old` wraps, and `off` doesn't wrap, wrap != (off_wrap >> 15) will be true. In this case, `off` is bigger than `new`, and what we should do is `off -= vq->num` instead of `off += vq->num`. Best regards, Tiwei Bie > > return vring_need_event(off, new, old); > } > > > > > > Best regards, > > Tiwei Bie > > > > > else > > > d1 = new - off - 1; > > > > > > if (new > old) > > > d2 = new - old; > > > else > > > d2 = new + vq->num - old; > > > > > > return d1 < d2; > > > } > > > > > > Thanks > > > >
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年05月08日 17:16, Tiwei Bie wrote: On Tue, May 08, 2018 at 03:16:53PM +0800, Jason Wang wrote: On 2018年05月08日 14:44, Tiwei Bie wrote: On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: On 2018年05月08日 11:05, Jason Wang wrote: Because in virtqueue_enable_cb_delayed(), we may set an event_off which is bigger than new and both of them have wrapped. And in this case, although new is smaller than event_off (i.e. the third param -- old), new shouldn't add vq->num, and actually we are expecting a very big idx diff. Yes, so to calculate distance correctly between event and new, we just need to compare the warp counter and return false if it doesn't match without the need to try to add vq.num here. Thanks Sorry, looks like the following should work, we need add vq.num if used_wrap_counter does not match: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = off_wrap >> 15; int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (wrap != vq->used_wrap_counter) d1 = new + vq->num - off - 1; Just to draw your attention (maybe you have already noticed this). I miss this, thanks! In this case (i.e. wrap != vq->used_wrap_counter), it's also possible that (off < new) is true. Because, when virtqueue_enable_cb_delayed_packed() is used, `off` is calculated in driver in a way like this: off = vq->last_used_idx + bufs; if (off >= vq->vring_packed.num) { off -= vq->vring_packed.num; wrap_counter ^= 1; } And when `new` (in vhost) is close to vq->num. The vq->last_used_idx + bufs (in driver) can be bigger than vq->vring_packed.num, and: 1. `off` will wrap; 2. wrap counters won't match; 3. off < new; And d1 (i.e. new + vq->num - off - 1) will be a value bigger than vq->num. I'm okay with this, although it's a bit weird. So I'm considering something more compact by reusing vring_need_event() by pretending a larger queue size and adding vq->num back when necessary: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = vq->used_wrap_counter; If the wrap counter is obtained from the vq, I think `new` should also be obtained from the vq. Or the wrap counter should be carried in `new`. int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (new < old) { new += vq->num; wrap ^= 1; } if (wrap != off_wrap >> 15) off += vq->num; When `new` and `old` wraps, and `off` doesn't wrap, wrap != (off_wrap >> 15) will be true. In this case, `off` is bigger than `new`, and what we should do is `off -= vq->num` instead of `off += vq->num`. If I understand this correctly, if we track old correctly, it won't happen if guest driver behave correctly. That means it should only happen for a buggy driver (e.g trying to move off_wrap back). Thanks Best regards, Tiwei Bie return vring_need_event(off, new, old); } Best regards, Tiwei Bie else d1 = new - off - 1; if (new > old) d2 = new - old; else d2 = new + vq->num - old; return d1 < d2; } Thanks
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On Tue, May 08, 2018 at 05:34:40PM +0800, Jason Wang wrote: > On 2018年05月08日 17:16, Tiwei Bie wrote: > > On Tue, May 08, 2018 at 03:16:53PM +0800, Jason Wang wrote: > > > On 2018年05月08日 14:44, Tiwei Bie wrote: > > > > On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: > > > > > On 2018年05月08日 11:05, Jason Wang wrote: > > > > > > > Because in virtqueue_enable_cb_delayed(), we may set an > > > > > > > event_off which is bigger than new and both of them have > > > > > > > wrapped. And in this case, although new is smaller than > > > > > > > event_off (i.e. the third param -- old), new shouldn't > > > > > > > add vq->num, and actually we are expecting a very big > > > > > > > idx diff. > > > > > > Yes, so to calculate distance correctly between event and new, we > > > > > > just > > > > > > need to compare the warp counter and return false if it doesn't > > > > > > match > > > > > > without the need to try to add vq.num here. > > > > > > > > > > > > Thanks > > > > > Sorry, looks like the following should work, we need add vq.num if > > > > > used_wrap_counter does not match: > > > > > > > > > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > > > > > __u16 off_wrap, __u16 new, > > > > > __u16 old) > > > > > { > > > > > bool wrap = off_wrap >> 15; > > > > > int off = off_wrap & ~(1 << 15); > > > > > __u16 d1, d2; > > > > > > > > > > if (wrap != vq->used_wrap_counter) > > > > > d1 = new + vq->num - off - 1; > > > > Just to draw your attention (maybe you have already > > > > noticed this). > > > I miss this, thanks! > > > > > > > In this case (i.e. wrap != vq->used_wrap_counter), > > > > it's also possible that (off < new) is true. Because, > > > > > > > > when virtqueue_enable_cb_delayed_packed() is used, > > > > `off` is calculated in driver in a way like this: > > > > > > > > off = vq->last_used_idx + bufs; > > > > if (off >= vq->vring_packed.num) { > > > > off -= vq->vring_packed.num; > > > > wrap_counter ^= 1; > > > > } > > > > > > > > And when `new` (in vhost) is close to vq->num. The > > > > vq->last_used_idx + bufs (in driver) can be bigger > > > > than vq->vring_packed.num, and: > > > > > > > > 1. `off` will wrap; > > > > 2. wrap counters won't match; > > > > 3. off < new; > > > > > > > > And d1 (i.e. new + vq->num - off - 1) will be a value > > > > bigger than vq->num. I'm okay with this, although it's > > > > a bit weird. > > > > > > So I'm considering something more compact by reusing vring_need_event() by > > > pretending a larger queue size and adding vq->num back when necessary: > > > > > > static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, > > > __u16 off_wrap, __u16 new, > > > __u16 old) > > > { > > > bool wrap = vq->used_wrap_counter; > > If the wrap counter is obtained from the vq, > > I think `new` should also be obtained from > > the vq. Or the wrap counter should be carried > > in `new`. > > > > > int off = off_wrap & ~(1 << 15); > > > __u16 d1, d2; > > > > > > if (new < old) { > > > new += vq->num; > > > wrap ^= 1; > > > } > > > > > > if (wrap != off_wrap >> 15) > > > off += vq->num; > > When `new` and `old` wraps, and `off` doesn't wrap, > > wrap != (off_wrap >> 15) will be true. In this case, > > `off` is bigger than `new`, and what we should do > > is `off -= vq->num` instead of `off += vq->num`. > > If I understand this correctly, if we track old correctly, it won't happen > if guest driver behave correctly. That means it should only happen for a > buggy driver (e.g trying to move off_wrap back). If vhost is faster than virtio driver, I guess above case may happen. The `old` and `new` will be updated each time we want to notify the driver. If the driver is slower, `old` and `new` in vhost may wrap before the `off` which is set by driver wraps. Best regards, Tiwei Bie > > Thanks > > > > > Best regards, > > Tiwei Bie > > > > > return vring_need_event(off, new, old); > > > } > > > > > > > > > > Best regards, > > > > Tiwei Bie > > > > > > > > > else > > > > > d1 = new - off - 1; > > > > > > > > > > if (new > old) > > > > > d2 = new - old; > > > > > else > > > > > d2 = new + vq->num - old; > > > > > > > > > > return d1 < d2; > > > > > } > > > > > > > > > > Thanks > > > > > >
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年05月08日 17:44, Tiwei Bie wrote: On Tue, May 08, 2018 at 05:34:40PM +0800, Jason Wang wrote: On 2018年05月08日 17:16, Tiwei Bie wrote: On Tue, May 08, 2018 at 03:16:53PM +0800, Jason Wang wrote: On 2018年05月08日 14:44, Tiwei Bie wrote: On Tue, May 08, 2018 at 01:40:40PM +0800, Jason Wang wrote: On 2018年05月08日 11:05, Jason Wang wrote: Because in virtqueue_enable_cb_delayed(), we may set an event_off which is bigger than new and both of them have wrapped. And in this case, although new is smaller than event_off (i.e. the third param -- old), new shouldn't add vq->num, and actually we are expecting a very big idx diff. Yes, so to calculate distance correctly between event and new, we just need to compare the warp counter and return false if it doesn't match without the need to try to add vq.num here. Thanks Sorry, looks like the following should work, we need add vq.num if used_wrap_counter does not match: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = off_wrap >> 15; int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (wrap != vq->used_wrap_counter) d1 = new + vq->num - off - 1; Just to draw your attention (maybe you have already noticed this). I miss this, thanks! In this case (i.e. wrap != vq->used_wrap_counter), it's also possible that (off < new) is true. Because, when virtqueue_enable_cb_delayed_packed() is used, `off` is calculated in driver in a way like this: off = vq->last_used_idx + bufs; if (off >= vq->vring_packed.num) { off -= vq->vring_packed.num; wrap_counter ^= 1; } And when `new` (in vhost) is close to vq->num. The vq->last_used_idx + bufs (in driver) can be bigger than vq->vring_packed.num, and: 1. `off` will wrap; 2. wrap counters won't match; 3. off < new; And d1 (i.e. new + vq->num - off - 1) will be a value bigger than vq->num. I'm okay with this, although it's a bit weird. So I'm considering something more compact by reusing vring_need_event() by pretending a larger queue size and adding vq->num back when necessary: static bool vhost_vring_packed_need_event(struct vhost_virtqueue *vq, __u16 off_wrap, __u16 new, __u16 old) { bool wrap = vq->used_wrap_counter; If the wrap counter is obtained from the vq, I think `new` should also be obtained from the vq. Or the wrap counter should be carried in `new`. int off = off_wrap & ~(1 << 15); __u16 d1, d2; if (new < old) { new += vq->num; wrap ^= 1; } if (wrap != off_wrap >> 15) off += vq->num; When `new` and `old` wraps, and `off` doesn't wrap, wrap != (off_wrap >> 15) will be true. In this case, `off` is bigger than `new`, and what we should do is `off -= vq->num` instead of `off += vq->num`. If I understand this correctly, if we track old correctly, it won't happen if guest driver behave correctly. That means it should only happen for a buggy driver (e.g trying to move off_wrap back). If vhost is faster than virtio driver, I guess above case may happen. The `old` and `new` will be updated each time we want to notify the driver. If the driver is slower, `old` and `new` in vhost may wrap before the `off` which is set by driver wraps. Best regards, Tiwei Bie Oh, right. But the code still work (in this case new - event_idx - 1 will underflow). (And I admit it still looks ugly). Thanks
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
On 2018年04月25日 13:15, Tiwei Bie wrote: This commit introduces the event idx support in packed ring. This feature is temporarily disabled, because the implementation in this patch may not work as expected, and some further discussions on the implementation are needed, e.g. do we have to check the wrap counter when checking whether a kick is needed? Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 53 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 0181e93897be..b1039c2985b9 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) { struct vring_virtqueue *vq = to_vvq(_vq); - u16 flags; + u16 new, old, off_wrap, flags; bool needs_kick; u32 snapshot; @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) * suppressions. */ virtio_mb(vq->weak_barriers); + old = vq->next_avail_idx - vq->num_added; + new = vq->next_avail_idx; + vq->num_added = 0; + snapshot = *(u32 *)vq->vring_packed.device; + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0x); flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3; #ifdef DEBUG @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq) vq->last_add_time_valid = false; #endif - needs_kick = (flags != VRING_EVENT_F_DISABLE); + if (flags == VRING_EVENT_F_DESC) + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old); + else + needs_kick = (flags != VRING_EVENT_F_DISABLE); END_USE(vq); return needs_kick; } @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, if (vq->last_used_idx >= vq->vring_packed.num) vq->last_used_idx -= vq->vring_packed.num; + /* If we expect an interrupt for the next entry, tell host +* by writing event index and flush out the write before +* the read in the next get_buf call. */ + if (vq->event_flags_shadow == VRING_EVENT_F_DESC) + virtio_store_mb(vq->weak_barriers, + &vq->vring_packed.driver->off_wrap, + cpu_to_virtio16(_vq->vdev, vq->last_used_idx | + (vq->wrap_counter << 15))); + #ifdef DEBUG vq->last_add_time_valid = false; #endif @@ -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. And I think there's even no need to bother with event idx here, how about just set VRING_EVENT_F_ENABLE? 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? + + 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
Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
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