[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
