Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
"Michael S. Tsirkin" writes: > On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote: >> Jason Wang writes: >> > We currently does: >> > >> > bufs = (avail->idx - last_used_idx) * 3 / 4; >> > >> > This is ok now since we only try to enable the delayed callbacks when >> > the queue is about to be full. This may not work well when there is >> > only one pending buffer in the virtqueue (this may be the case after >> > tx interrupt was enabled). Since virtqueue_enable_cb() will return >> > false which may cause unnecessary triggering of napis. This patch >> > correct this by only calculate the four thirds when bufs is not one. >> >> I mildly prefer to avoid the branch, by changing the calculation like >> so: >> >> /* Set bufs >= 1, even if there's only one pending buffer */ >> bufs = (bufs + 1) * 3 / 4; > > Or bus * 3/4 + 1 > >> But it's not clear to me how much this happens. I'm happy with the >> patch though, as currently virtqueue_enable_cb_delayed() is the same >> as virtqueue_enable_cb() if there's only been one buffer added. >> >> Cheers, >> Rusty. > > But isn't this by design? > The documentation says: > > * This re-enables callbacks but hints to the other side to delay > * interrupts until most of the available buffers have been processed; > > Clearly, this implies that when there's one buffer it must behave > exactly the same. Yes, my mistake. We could hit the "never gets notified" case with this change, and that's a much bigger problem. So I don't think we can accept this patch... Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
On Tue, Feb 10, 2015 at 6:18 PM, Michael S. Tsirkin wrote: On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote: Jason Wang writes: > We currently does: > > bufs = (avail->idx - last_used_idx) * 3 / 4; > > This is ok now since we only try to enable the delayed callbacks when > the queue is about to be full. This may not work well when there is > only one pending buffer in the virtqueue (this may be the case after > tx interrupt was enabled). Since virtqueue_enable_cb() will return > false which may cause unnecessary triggering of napis. This patch > correct this by only calculate the four thirds when bufs is not one. I mildly prefer to avoid the branch, by changing the calculation like so: /* Set bufs >= 1, even if there's only one pending buffer */ bufs = (bufs + 1) * 3 / 4; Or bus * 3/4 + 1 But it's not clear to me how much this happens. I'm happy with the patch though, as currently virtqueue_enable_cb_delayed() is the same as virtqueue_enable_cb() if there's only been one buffer added. Cheers, Rusty. But isn't this by design? The documentation says: * This re-enables callbacks but hints to the other side to delay * interrupts until most of the available buffers have been processed; Clearly, this implies that when there's one buffer it must behave exactly the same. So I'm not very happy - this changes the meaning of the API without updating the documentation. Think hard about this. And looks like your are right. And the patch may in fact cause more trouble e.g the only pending buffer were consumed before the final check between used idx and last_used_idx. Will drop this. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote:
> Jason Wang writes:
> > We currently does:
> >
> > bufs = (avail->idx - last_used_idx) * 3 / 4;
> >
> > This is ok now since we only try to enable the delayed callbacks when
> > the queue is about to be full. This may not work well when there is
> > only one pending buffer in the virtqueue (this may be the case after
> > tx interrupt was enabled). Since virtqueue_enable_cb() will return
> > false which may cause unnecessary triggering of napis. This patch
> > correct this by only calculate the four thirds when bufs is not one.
>
> I mildly prefer to avoid the branch, by changing the calculation like
> so:
>
> /* Set bufs >= 1, even if there's only one pending buffer */
> bufs = (bufs + 1) * 3 / 4;
Or bus * 3/4 + 1
> But it's not clear to me how much this happens. I'm happy with the
> patch though, as currently virtqueue_enable_cb_delayed() is the same
> as virtqueue_enable_cb() if there's only been one buffer added.
>
> Cheers,
> Rusty.
But isn't this by design?
The documentation says:
* This re-enables callbacks but hints to the other side to delay
* interrupts until most of the available buffers have been processed;
Clearly, this implies that when there's one buffer it must behave
exactly the same.
So I'm not very happy - this changes the meaning of the API without
updating the documentation.
> > Signed-off-by: Jason Wang
> > ---
> > drivers/virtio/virtio_ring.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 00ec6b3..545fed5 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
> > * entry. Always do both to keep code simple. */
> > vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev,
> > ~VRING_AVAIL_F_NO_INTERRUPT);
> > /* TODO: tune this threshold */
> > - bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
> > vq->last_used_idx) * 3 / 4;
> > + bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
> > +vq->last_used_idx);
> > + if (bufs != 1)
> > + bufs = bufs * 3 / 4;
> > vring_used_event(>vring) = cpu_to_virtio16(_vq->vdev,
> > vq->last_used_idx + bufs);
> > virtio_mb(vq->weak_barriers);
> > if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) -
> > vq->last_used_idx) > bufs)) {
> > --
> > 1.8.3.1
> >
> > ___
> > Virtualization mailing list
> > virtualizat...@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote:
Jason Wang jasow...@redhat.com writes:
We currently does:
bufs = (avail-idx - last_used_idx) * 3 / 4;
This is ok now since we only try to enable the delayed callbacks when
the queue is about to be full. This may not work well when there is
only one pending buffer in the virtqueue (this may be the case after
tx interrupt was enabled). Since virtqueue_enable_cb() will return
false which may cause unnecessary triggering of napis. This patch
correct this by only calculate the four thirds when bufs is not one.
I mildly prefer to avoid the branch, by changing the calculation like
so:
/* Set bufs = 1, even if there's only one pending buffer */
bufs = (bufs + 1) * 3 / 4;
Or bus * 3/4 + 1
But it's not clear to me how much this happens. I'm happy with the
patch though, as currently virtqueue_enable_cb_delayed() is the same
as virtqueue_enable_cb() if there's only been one buffer added.
Cheers,
Rusty.
But isn't this by design?
The documentation says:
* This re-enables callbacks but hints to the other side to delay
* interrupts until most of the available buffers have been processed;
Clearly, this implies that when there's one buffer it must behave
exactly the same.
So I'm not very happy - this changes the meaning of the API without
updating the documentation.
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/virtio/virtio_ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 00ec6b3..545fed5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
* entry. Always do both to keep code simple. */
vq-vring.avail-flags = cpu_to_virtio16(_vq-vdev,
~VRING_AVAIL_F_NO_INTERRUPT);
/* TODO: tune this threshold */
- bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
vq-last_used_idx) * 3 / 4;
+ bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
+vq-last_used_idx);
+ if (bufs != 1)
+ bufs = bufs * 3 / 4;
vring_used_event(vq-vring) = cpu_to_virtio16(_vq-vdev,
vq-last_used_idx + bufs);
virtio_mb(vq-weak_barriers);
if (unlikely((u16)(virtio16_to_cpu(_vq-vdev, vq-vring.used-idx) -
vq-last_used_idx) bufs)) {
--
1.8.3.1
___
Virtualization mailing list
virtualizat...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
On Tue, Feb 10, 2015 at 6:18 PM, Michael S. Tsirkin m...@redhat.com wrote: On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote: Jason Wang jasow...@redhat.com writes: We currently does: bufs = (avail-idx - last_used_idx) * 3 / 4; This is ok now since we only try to enable the delayed callbacks when the queue is about to be full. This may not work well when there is only one pending buffer in the virtqueue (this may be the case after tx interrupt was enabled). Since virtqueue_enable_cb() will return false which may cause unnecessary triggering of napis. This patch correct this by only calculate the four thirds when bufs is not one. I mildly prefer to avoid the branch, by changing the calculation like so: /* Set bufs = 1, even if there's only one pending buffer */ bufs = (bufs + 1) * 3 / 4; Or bus * 3/4 + 1 But it's not clear to me how much this happens. I'm happy with the patch though, as currently virtqueue_enable_cb_delayed() is the same as virtqueue_enable_cb() if there's only been one buffer added. Cheers, Rusty. But isn't this by design? The documentation says: * This re-enables callbacks but hints to the other side to delay * interrupts until most of the available buffers have been processed; Clearly, this implies that when there's one buffer it must behave exactly the same. So I'm not very happy - this changes the meaning of the API without updating the documentation. Think hard about this. And looks like your are right. And the patch may in fact cause more trouble e.g the only pending buffer were consumed before the final check between used idx and last_used_idx. Will drop this. Thanks -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
Michael S. Tsirkin m...@redhat.com writes: On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote: Jason Wang jasow...@redhat.com writes: We currently does: bufs = (avail-idx - last_used_idx) * 3 / 4; This is ok now since we only try to enable the delayed callbacks when the queue is about to be full. This may not work well when there is only one pending buffer in the virtqueue (this may be the case after tx interrupt was enabled). Since virtqueue_enable_cb() will return false which may cause unnecessary triggering of napis. This patch correct this by only calculate the four thirds when bufs is not one. I mildly prefer to avoid the branch, by changing the calculation like so: /* Set bufs = 1, even if there's only one pending buffer */ bufs = (bufs + 1) * 3 / 4; Or bus * 3/4 + 1 But it's not clear to me how much this happens. I'm happy with the patch though, as currently virtqueue_enable_cb_delayed() is the same as virtqueue_enable_cb() if there's only been one buffer added. Cheers, Rusty. But isn't this by design? The documentation says: * This re-enables callbacks but hints to the other side to delay * interrupts until most of the available buffers have been processed; Clearly, this implies that when there's one buffer it must behave exactly the same. Yes, my mistake. We could hit the never gets notified case with this change, and that's a much bigger problem. So I don't think we can accept this patch... Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
On Tue, Feb 10, 2015 at 9:03 AM, Rusty Russell
wrote:
Jason Wang writes:
We currently does:
bufs = (avail->idx - last_used_idx) * 3 / 4;
This is ok now since we only try to enable the delayed callbacks
when
the queue is about to be full. This may not work well when there is
only one pending buffer in the virtqueue (this may be the case after
tx interrupt was enabled). Since virtqueue_enable_cb() will return
false which may cause unnecessary triggering of napis. This patch
correct this by only calculate the four thirds when bufs is not one.
I mildly prefer to avoid the branch, by changing the calculation like
so:
/* Set bufs >= 1, even if there's only one pending buffer */
bufs = (bufs + 1) * 3 / 4;
Ok.
But it's not clear to me how much this happens.
Depends on the traffic type. In some case e.g one session TCP_RR test
(which has at most 1 pending packet), it may happen very often.
I'm happy with the
patch though, as currently virtqueue_enable_cb_delayed() is the same
as virtqueue_enable_cb() if there's only been one buffer added.
Yes. Thanks.
Cheers,
Rusty.
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c
b/drivers/virtio/virtio_ring.c
index 00ec6b3..545fed5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct
virtqueue *_vq)
* entry. Always do both to keep code simple. */
vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev,
~VRING_AVAIL_F_NO_INTERRUPT);
/* TODO: tune this threshold */
- bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
vq->last_used_idx) * 3 / 4;
+ bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
+ vq->last_used_idx);
+ if (bufs != 1)
+ bufs = bufs * 3 / 4;
vring_used_event(>vring) = cpu_to_virtio16(_vq->vdev,
vq->last_used_idx + bufs);
virtio_mb(vq->weak_barriers);
if (unlikely((u16)(virtio16_to_cpu(_vq->vdev,
vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
--
1.8.3.1
___
Virtualization mailing list
virtualizat...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
Jason Wang writes:
> We currently does:
>
> bufs = (avail->idx - last_used_idx) * 3 / 4;
>
> This is ok now since we only try to enable the delayed callbacks when
> the queue is about to be full. This may not work well when there is
> only one pending buffer in the virtqueue (this may be the case after
> tx interrupt was enabled). Since virtqueue_enable_cb() will return
> false which may cause unnecessary triggering of napis. This patch
> correct this by only calculate the four thirds when bufs is not one.
I mildly prefer to avoid the branch, by changing the calculation like
so:
/* Set bufs >= 1, even if there's only one pending buffer */
bufs = (bufs + 1) * 3 / 4;
But it's not clear to me how much this happens. I'm happy with the
patch though, as currently virtqueue_enable_cb_delayed() is the same
as virtqueue_enable_cb() if there's only been one buffer added.
Cheers,
Rusty.
> Signed-off-by: Jason Wang
> ---
> drivers/virtio/virtio_ring.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 00ec6b3..545fed5 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>* entry. Always do both to keep code simple. */
> vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev,
> ~VRING_AVAIL_F_NO_INTERRUPT);
> /* TODO: tune this threshold */
> - bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
> vq->last_used_idx) * 3 / 4;
> + bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
> + vq->last_used_idx);
> + if (bufs != 1)
> + bufs = bufs * 3 / 4;
> vring_used_event(>vring) = cpu_to_virtio16(_vq->vdev,
> vq->last_used_idx + bufs);
> virtio_mb(vq->weak_barriers);
> if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) -
> vq->last_used_idx) > bufs)) {
> --
> 1.8.3.1
>
> ___
> Virtualization mailing list
> virtualizat...@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
We currently does:
bufs = (avail->idx - last_used_idx) * 3 / 4;
This is ok now since we only try to enable the delayed callbacks when
the queue is about to be full. This may not work well when there is
only one pending buffer in the virtqueue (this may be the case after
tx interrupt was enabled). Since virtqueue_enable_cb() will return
false which may cause unnecessary triggering of napis. This patch
correct this by only calculate the four thirds when bufs is not one.
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 00ec6b3..545fed5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
* entry. Always do both to keep code simple. */
vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev,
~VRING_AVAIL_F_NO_INTERRUPT);
/* TODO: tune this threshold */
- bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
vq->last_used_idx) * 3 / 4;
+ bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) -
+vq->last_used_idx);
+ if (bufs != 1)
+ bufs = bufs * 3 / 4;
vring_used_event(>vring) = cpu_to_virtio16(_vq->vdev,
vq->last_used_idx + bufs);
virtio_mb(vq->weak_barriers);
if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) -
vq->last_used_idx) > bufs)) {
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
Jason Wang jasow...@redhat.com writes:
We currently does:
bufs = (avail-idx - last_used_idx) * 3 / 4;
This is ok now since we only try to enable the delayed callbacks when
the queue is about to be full. This may not work well when there is
only one pending buffer in the virtqueue (this may be the case after
tx interrupt was enabled). Since virtqueue_enable_cb() will return
false which may cause unnecessary triggering of napis. This patch
correct this by only calculate the four thirds when bufs is not one.
I mildly prefer to avoid the branch, by changing the calculation like
so:
/* Set bufs = 1, even if there's only one pending buffer */
bufs = (bufs + 1) * 3 / 4;
But it's not clear to me how much this happens. I'm happy with the
patch though, as currently virtqueue_enable_cb_delayed() is the same
as virtqueue_enable_cb() if there's only been one buffer added.
Cheers,
Rusty.
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/virtio/virtio_ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 00ec6b3..545fed5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
* entry. Always do both to keep code simple. */
vq-vring.avail-flags = cpu_to_virtio16(_vq-vdev,
~VRING_AVAIL_F_NO_INTERRUPT);
/* TODO: tune this threshold */
- bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
vq-last_used_idx) * 3 / 4;
+ bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
+ vq-last_used_idx);
+ if (bufs != 1)
+ bufs = bufs * 3 / 4;
vring_used_event(vq-vring) = cpu_to_virtio16(_vq-vdev,
vq-last_used_idx + bufs);
virtio_mb(vq-weak_barriers);
if (unlikely((u16)(virtio16_to_cpu(_vq-vdev, vq-vring.used-idx) -
vq-last_used_idx) bufs)) {
--
1.8.3.1
___
Virtualization mailing list
virtualizat...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
On Tue, Feb 10, 2015 at 9:03 AM, Rusty Russell ru...@rustcorp.com.au
wrote:
Jason Wang jasow...@redhat.com writes:
We currently does:
bufs = (avail-idx - last_used_idx) * 3 / 4;
This is ok now since we only try to enable the delayed callbacks
when
the queue is about to be full. This may not work well when there is
only one pending buffer in the virtqueue (this may be the case after
tx interrupt was enabled). Since virtqueue_enable_cb() will return
false which may cause unnecessary triggering of napis. This patch
correct this by only calculate the four thirds when bufs is not one.
I mildly prefer to avoid the branch, by changing the calculation like
so:
/* Set bufs = 1, even if there's only one pending buffer */
bufs = (bufs + 1) * 3 / 4;
Ok.
But it's not clear to me how much this happens.
Depends on the traffic type. In some case e.g one session TCP_RR test
(which has at most 1 pending packet), it may happen very often.
I'm happy with the
patch though, as currently virtqueue_enable_cb_delayed() is the same
as virtqueue_enable_cb() if there's only been one buffer added.
Yes. Thanks.
Cheers,
Rusty.
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/virtio/virtio_ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c
b/drivers/virtio/virtio_ring.c
index 00ec6b3..545fed5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct
virtqueue *_vq)
* entry. Always do both to keep code simple. */
vq-vring.avail-flags = cpu_to_virtio16(_vq-vdev,
~VRING_AVAIL_F_NO_INTERRUPT);
/* TODO: tune this threshold */
- bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
vq-last_used_idx) * 3 / 4;
+ bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
+ vq-last_used_idx);
+ if (bufs != 1)
+ bufs = bufs * 3 / 4;
vring_used_event(vq-vring) = cpu_to_virtio16(_vq-vdev,
vq-last_used_idx + bufs);
virtio_mb(vq-weak_barriers);
if (unlikely((u16)(virtio16_to_cpu(_vq-vdev,
vq-vring.used-idx) - vq-last_used_idx) bufs)) {
--
1.8.3.1
___
Virtualization mailing list
virtualizat...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
We currently does:
bufs = (avail-idx - last_used_idx) * 3 / 4;
This is ok now since we only try to enable the delayed callbacks when
the queue is about to be full. This may not work well when there is
only one pending buffer in the virtqueue (this may be the case after
tx interrupt was enabled). Since virtqueue_enable_cb() will return
false which may cause unnecessary triggering of napis. This patch
correct this by only calculate the four thirds when bufs is not one.
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/virtio/virtio_ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 00ec6b3..545fed5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -636,7 +636,10 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
* entry. Always do both to keep code simple. */
vq-vring.avail-flags = cpu_to_virtio16(_vq-vdev,
~VRING_AVAIL_F_NO_INTERRUPT);
/* TODO: tune this threshold */
- bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
vq-last_used_idx) * 3 / 4;
+ bufs = (u16)(virtio16_to_cpu(_vq-vdev, vq-vring.avail-idx) -
+vq-last_used_idx);
+ if (bufs != 1)
+ bufs = bufs * 3 / 4;
vring_used_event(vq-vring) = cpu_to_virtio16(_vq-vdev,
vq-last_used_idx + bufs);
virtio_mb(vq-weak_barriers);
if (unlikely((u16)(virtio16_to_cpu(_vq-vdev, vq-vring.used-idx) -
vq-last_used_idx) bufs)) {
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
