Re: [PATCH 3/9] kvm: qemu: Remove virtio_net tx ring-full heuristic
Mark McLoughlin wrote: virtio_net tries to guess when it has received a tx notification from the guest whether it indicates that the guest has no more room in the tx ring and it should immediately flush the queued buffers. The heuristic is based on the fact that there are 128 buffer entries in the ring and each packet uses 2 buffers (i.e. the virtio_net_hdr and the packet's linear data). Using GSO or increasing the size of the rings will break that heuristic, so let's remove it and assume that any notification from the guest after we've disabled notifications indicates that we should flush our buffers. Signed-off-by: Mark McLoughlin <[EMAIL PROTECTED]> --- qemu/hw/virtio-net.c |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c index 31867f1..4adfa42 100644 --- a/qemu/hw/virtio-net.c +++ b/qemu/hw/virtio-net.c @@ -175,8 +175,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq) { VirtIONet *n = to_virtio_net(vdev); -if (n->tx_timer_active && - (vq->vring.avail->idx - vq->last_avail_idx) == 64) { +if (n->tx_timer_active) { Actually it was worthless anyway since if we set the timer, the flag below would have been cleared, causing the guest not to notify the host, thus the ==64 never called. So I'm in favour too. vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY; qemu_del_timer(n->tx_timer); n->tx_timer_active = 0; As stated by newer messages, we should handle the first tx notification if the timer wasn't active to shorten latency. Cheers, Dor -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] kvm: qemu: Remove virtio_net tx ring-full heuristic
Mark McLoughlin wrote: virtio_net tries to guess when it has received a tx notification from the guest whether it indicates that the guest has no more room in the tx ring and it should immediately flush the queued buffers. The heuristic is based on the fact that there are 128 buffer entries in the ring and each packet uses 2 buffers (i.e. the virtio_net_hdr and the packet's linear data). Using GSO or increasing the size of the rings will break that heuristic, so let's remove it and assume that any notification from the guest after we've disabled notifications indicates that we should flush our buffers. Signed-off-by: Mark McLoughlin <[EMAIL PROTECTED]> --- qemu/hw/virtio-net.c |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c index 31867f1..4adfa42 100644 --- a/qemu/hw/virtio-net.c +++ b/qemu/hw/virtio-net.c @@ -175,8 +175,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq) { VirtIONet *n = to_virtio_net(vdev); -if (n->tx_timer_active && - (vq->vring.avail->idx - vq->last_avail_idx) == 64) { +if (n->tx_timer_active) { vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY; qemu_del_timer(n->tx_timer); n->tx_timer_active = 0; Actually we can improve latency a bit more by using this timer only for high throughput scenario. For example, if during the previous timer period no/few packets were accumulated, we can set the flag off and not issue new timer. This way we'll get notified immediately without timer latency. When lots of packets will be transmitted, we'll go back to this batch mode again. Cheers, Dor -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] kvm: qemu: Remove virtio_net tx ring-full heuristic
On Friday 25 July 2008 09:22:53 Dor Laor wrote: > Mark McLoughlin wrote: > > vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY; > > qemu_del_timer(n->tx_timer); > > n->tx_timer_active = 0; > > As stated by newer messages, we should handle the first tx notification > if the timer wasn't active to shorten latency. > Cheers, Dor Here's what lguest does at the moment. Basically, we cut the timeout a tiny bit each time, until we get *fewer* packets than last time. Then we bump it up again. Rough, but seems to work (it should be a per-device var of course, not a static). @@ -921,6 +922,7 @@ static void handle_net_output(int fd, st unsigned int head, out, in, num = 0; int len; struct iovec iov[vq->vring.num]; + static int last_timeout_num; if (!timeout) net_xmit_notify++; @@ -941,6 +943,14 @@ static void handle_net_output(int fd, st /* Block further kicks and set up a timer if we saw anything. */ if (!timeout && num) block_vq(vq); + + if (timeout) { + if (num < last_timeout_num) + timeout_usec += 10; + else if (timeout_usec > 1) + timeout_usec--; + last_timeout_num = num; + } } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] kvm: qemu: Remove virtio_net tx ring-full heuristic
On Fri, 2008-07-25 at 02:22 +0300, Dor Laor wrote: > Mark McLoughlin wrote: > > virtio_net tries to guess when it has received a tx > > notification from the guest whether it indicates that the > > guest has no more room in the tx ring and it should > > immediately flush the queued buffers. ... > > -if (n->tx_timer_active && > > - (vq->vring.avail->idx - vq->last_avail_idx) == 64) { > > +if (n->tx_timer_active) { > > > Actually it was worthless anyway since if we set the timer, the flag > below would have been cleared, > causing the guest not to notify the host, thus the ==64 never called. No, that's the point I'm trying to make in the commit message - the guest ignores the NO_NOTIFY flag when the tx queue fills up and sends a notification anyway. The "== 64" test seems to be an attempt to catch this ring-full condition. Well, actually the test was worthless for a different reason, I guess. If tx_timer_active is set and we get a notification, then given the current guest code the number of buffers available should *always* be 64. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] kvm: qemu: Remove virtio_net tx ring-full heuristic
On Fri, 2008-07-25 at 10:30 +1000, Rusty Russell wrote: > On Friday 25 July 2008 09:22:53 Dor Laor wrote: > > Mark McLoughlin wrote: > > > vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY; > > > qemu_del_timer(n->tx_timer); > > > n->tx_timer_active = 0; > > > > As stated by newer messages, we should handle the first tx notification > > if the timer wasn't active to shorten latency. > > Cheers, Dor > > Here's what lguest does at the moment. Basically, we cut the timeout a tiny > bit each time, until we get *fewer* packets than last time. Then we bump it > up again. > > Rough, but seems to work (it should be a per-device var of course, not a > static). > > @@ -921,6 +922,7 @@ static void handle_net_output(int fd, st > unsigned int head, out, in, num = 0; > int len; > struct iovec iov[vq->vring.num]; > + static int last_timeout_num; > > if (!timeout) > net_xmit_notify++; > @@ -941,6 +943,14 @@ static void handle_net_output(int fd, st > /* Block further kicks and set up a timer if we saw anything. */ > if (!timeout && num) > block_vq(vq); > + > + if (timeout) { > + if (num < last_timeout_num) > + timeout_usec += 10; > + else if (timeout_usec > 1) > + timeout_usec--; > + last_timeout_num = num; > + } Yeah, I gave this a try in kvm and in the host->guest case the timeout just grew and grew. In the guest->host case, it did stabilise at around 50us with high throughput. Basically, I think in the host->guest case the number of buffers was very variable so the timeout would get bumped by 10, reduced by a small amount, bumped by 10, reduced by a small amount, ... But, I agree the general principal seems about right; just needs some tweaking. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html