Network performance with small packets

I am working on a KVM network performance issue found in our lab running the DayTrader benchmark. The benchmark throughput takes a significant hit when running the application server in a KVM guest verses on bare metal. We have dug into the problem and found that DayTrader's use of small packets exposes KVM's overhead of handling network packets. I have been able to reproduce the performance hit with a simpler setup using the netperf benchmark with the TCP_RR test and the request and response sizes set to 256 bytes. I run the benchmark between two physical systems, each using a 1GB link. In order to get the maximum throughput for the system I have to run 100 instances of netperf. When I run the netserver processes in a guest, I see a maximum throughput that is 51% of what I get if I run the netserver processes directly on the host. The CPU utilization in the guest is only 85% at maximum throughput, whereas it is 100% on bare metal.

The KVM host has 16 CPUs. The KVM guest is configured with 2 VCPUs. When
I run netperf on the host I boot the host with maxcpus=2 on the kernel
command line. The host is running the current KVM upstream kernel along
with the current upstream qemu. Here is the qemu command used to launch
the guest:
/build/qemu-kvm/x86_64-softmmu/qemu-system-x86_64 -name glasgow-RH60 -m 32768 -drive file=/build/guest-data/glasgow-RH60.img,if=virtio,index=0,boot=on
-drive file=/dev/virt/WAS,if=virtio,index=1 -net nic,model=virtio,vlan=3,macaddr=00:1A:64:E5:00:63,netdev=nic0 -netdev tap,id=nic0,vhost=on -smp 2
-vnc :1 -monitor telnet::4499,server,nowait -serial telnet::8899,server,nowait --mem-path /libhugetlbfs -daemonize

We have tried various proposed fixes, each with varying amounts of success. One such fix was to add code to the vhost thread such that when it found the work queue empty it wouldn't just exit the thread but rather would delay for 50 microseconds and then recheck the queue. If there was work on the queue it would loop back and process it, else it would exit the thread. The change got us a 13% improvement in the DayTrader throughput.

Running the same netperf configuration on the same hardware but using a
different hypervisor gets us significantly better throughput numbers. The
guest on that hypervisor runs at 100% CPU utilization. The various fixes we have tried have not gotten us close to the throughput seen on the other hypervisor. I'm looking for ideas/input from the KVM experts on how to make KVM perform better when handling small packets.

Thanks,
Steve

I am seeing a similar problem, and am trying to fix that. My current theory is that this is a variant of a receive livelock:
if the application isn't fast enough to process incoming data, the guest net stack switches
from prequeue to backlog handling.

One thing I noticed is that locking the vhost thread and the vcpu to the same physical CPU almost doubles the bandwidth. Can you confirm that in your setup?

My current guess is that when we lock both to
a single CPU, netperf in guest gets scheduled slowing down the vhost thread in the host.

I also noticed that this specific workload
performs better with vhost off: presumably
we are loading the guest less.

Quoted 10 lines · Michael S. Tsirkin

I am seeing a similar problem, and am trying to fix that. My current theory is that this is a variant of a receive livelock:
if the application isn't fast enough to process incoming data, the guest net stack switches
from prequeue to backlog handling.

One thing I noticed is that locking the vhost thread and the vcpu to the same physical CPU almost doubles the bandwidth. Can you confirm that in your setup?

Quoted 7 lines

My current guess is that when we lock both to
a single CPU, netperf in guest gets scheduled slowing down the vhost thread in the host.

I also noticed that this specific workload
performs better with vhost off: presumably
we are loading the guest less.

I found similar issue for small message size TCP_STREAM test when guest as TX. I found when I slow down TX, the BW performance will be doubled for 1K to 4K message size.

Shirley

Interesting. In particular running vhost and the transmitting guest on the same host would have the effect of slowing down TX. Does it double the BW for you too?

Quoted 4 lines · Michael S. Tsirkin

Interesting. In particular running vhost and the transmitting guest on the same host would have the effect of slowing down TX. Does it double the BW for you too?

Running vhost and TX guest on the same host seems not good enough to slow down TX. In order to gain the double even triple BW for guest TX to local host I still need to play around, so 1K message size, BW is able to increase from 2.XGb/s to 6.XGb/s.

Thanks
Shirley

Well slowing down the guest does not sound hard - for example we can request guest notifications, or send extra interrupts :) A slightly more sophisticated thing to try is to
poll the vq a bit more aggressively.
For example if we handled some requests and now tx vq is empty, reschedule and yeild. Worth a try?

Quoted 6 lines · Michael S. Tsirkin

Well slowing down the guest does not sound hard - for example we can request guest notifications, or send extra interrupts :) A slightly more sophisticated thing to try is to
poll the vq a bit more aggressively.
For example if we handled some requests and now tx vq is empty, reschedule and yeild. Worth a try?

I used dropping packets in high level to slow down TX. I am still
thinking what's the right the approach here.

Requesting guest notification and extra interrupts is what we want to avoid to reduce VM exits for saving CPUs. I don't think it's good.

By polling the vq a bit more aggressively, you meant vhost, right?

Shirley

Interesting. Could this is be a variant of the now famuous bufferbloat then?

I guess we could drop some packets if we see we are not keeping up. For
example if we see that the ring is > X% full, we could quickly complete
Y% without transmitting packets on. Or maybe we should drop some bytes not packets.

Quoted 2 lines

Requesting guest notification and extra interrupts is what we want

to

Quoted 1 line

avoid to reduce VM exits for saving CPUs. I don't think it's good.

Yes but how do you explain regression?
One simple theory is that guest net stack became faster and so the host can't keep up.

Quoted 4 lines

By polling the vq a bit more aggressively, you meant vhost, right?

Shirley

Yes.

Quoted 2 lines

One simple theory is that guest net stack became faster and so the host can't keep up.

Yes, that's what I think here. Some qdisc code has been changed recently.

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines

One simple theory is that guest net stack became faster and so the host can't keep up.

Yes, that's what I think here. Some qdisc code has been changed recently.

I ran a test with txqueuelen set to 128, instead of the default of 1000,

the guest in an attempt to slow down the guest transmits. The change had no effect on the throughput nor on the CPU usage.

On the other hand, I ran some tests with different CPU pinnings and with/without hyperthreading enabled. Here is a summary of the results.

Pinning configuration 1: pin the VCPUs and pin the vhost thread to one

the VCPU CPUs
Pinning configuration 2: pin the VCPUs and pin the vhost thread to a separate CPU on the same socket
Pinning configuration 3: pin the VCPUs and pin the vhost thread to a separate CPU a different socket

HT   Pinning   Throughput  CPU
Yes  config 1  - 40%       - 40%
Yes  config 2  - 37%       - 35%
Yes  config 3  - 37%       - 36%
No   none         0%       -  5%
No   config 1  - 41%       - 43%
No   config 2  + 32%       -  4%
No   config 3  + 34%       +  9%

Pinning the vhost thread to the same CPU as a guest VCPU hurts

Turning off hyperthreading and pinning the VPUS and vhost thread to separate CPUs significantly improves performance, getting it into the competitive range with other hypervisors.

Steve D.

OK, so thinking about it more, maybe the issue is this:
tx becomes full. We process one request and interrupt the guest, then it adds one request and the queue is full again.

Maybe the following will help it stabilize? By default with it we will only interrupt when we see an empty ring.
Which is liklely too much: pls try other values
in the middle: e.g. make bufs half the ring,
or bytes some small value like half ring * 200, or packets some small value etc.

Set any one parameter to 0 to get current
behaviour (interrupt immediately when enabled).

Warning: completely untested.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

drivers/vhost/net.c +10 -0

index aac05bc..6769cdc 100644
@@ -32,6 +32,13 @@
  * Using this limit prevents one virtqueue from starving others. */
 #define VHOST_NET_WEIGHT 0x80000
 
+int tx_bytes_coalesce = 1000000000;
+module_param(tx_bytes_coalesce, int, 0644);
+int tx_bufs_coalesce = 1000000000;
+module_param(tx_bufs_coalesce, int, 0644);
+int tx_packets_coalesce = 1000000000;
+module_param(tx_packets_coalesce, int, 0644);
+
 enum {
 	VHOST_NET_VQ_RX = 0,
 	VHOST_NET_VQ_TX = 1,

@@ -127,6 +134,9 @@ static void handle_tx(struct vhost_net *net)
 	int err, wmem;
 	size_t hdr_size;
 	struct socket *sock;
+	int bytes_coalesced = 0;
+	int bufs_coalesced = 0;
+	int packets_coalesced = 0;
 
 	/* TODO: check that we are running from vhost_worker? */
 	sock = rcu_dereference_check(vq->private_data, 1);

@@ -196,14 +206,26 @@ static void handle_tx(struct vhost_net *net)
 		if (err != len)
 			pr_debug("Truncated TX packet: "

drivers/vhost/net.c +6 -1

+		else
+			vhost_add_used(vq, head, 0);
 		if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
 			vhost_poll_queue(&vq->poll);
 			break;
 		}
 	}
 
+	if (likely(packets_coalesced &&
+		   bytes_coalesced &&
+		   bufs_coalesced))
+		vhost_signal(&net->dev, vq);
 	mutex_unlock(&vq->mutex);
 }
 
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I think the counters that exceed the limits need to be reset to 0 here.
Otherwise we keep signaling for every buffer once we hit this condition.

Quoted 51 lines · Michael S. Tsirkin

OK, so thinking about it more, maybe the issue is this:
tx becomes full. We process one request and interrupt the guest, then it adds one request and the queue is full again.

Maybe the following will help it stabilize? By default with it we will only interrupt when we see an empty ring.
Which is liklely too much: pls try other values
in the middle: e.g. make bufs half the ring,
or bytes some small value like half ring * 200, or packets some small value etc.

Set any one parameter to 0 to get current
behaviour (interrupt immediately when enabled).

Warning: completely untested.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

drivers/vhost/net.c +10 -0

index aac05bc..6769cdc 100644
@@ -32,6 +32,13 @@
  * Using this limit prevents one virtqueue from starving others. */
 #define VHOST_NET_WEIGHT 0x80000
 
+int tx_bytes_coalesce = 1000000000;
+module_param(tx_bytes_coalesce, int, 0644);
+int tx_bufs_coalesce = 1000000000;
+module_param(tx_bufs_coalesce, int, 0644);
+int tx_packets_coalesce = 1000000000;
+module_param(tx_packets_coalesce, int, 0644);
+
 enum {
 	VHOST_NET_VQ_RX = 0,
 	VHOST_NET_VQ_TX = 1,

@@ -127,6 +134,9 @@ static void handle_tx(struct vhost_net *net)
 	int err, wmem;
 	size_t hdr_size;
 	struct socket *sock;
+	int bytes_coalesced = 0;
+	int bufs_coalesced = 0;
+	int packets_coalesced = 0;
 
 	/* TODO: check that we are running from vhost_worker? */
 	sock = rcu_dereference_check(vq->private_data, 1);

@@ -196,14 +206,26 @@ static void handle_tx(struct vhost_net *net)
 		if (err != len)
 			pr_debug("Truncated TX packet: "

drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 216

Quoted 10 lines

drivers/vhost/net.c +7 -1

 				 " len %d != %zd\n", err, len);
-		vhost_add_used_and_signal(&net->dev, vq, head, 0);
 		total_len += len;
+		packets_coalesced += 1;
+		bytes_coalesced += len;
+		bufs_coalesced += out;
+		if (unlikely(packets_coalesced > tx_packets_coalesce ||
+			     bytes_coalesced > tx_bytes_coalesce ||
+			     bufs_coalesced > tx_bufs_coalesce))
+			vhost_add_used_and_signal(&net->dev, vq, head, 0);

I think the counters that exceed the limits need to be reset to 0 here. Otherwise we keep signaling for every buffer once we hit this condition.

Thanks
Sridhar

Quoted 19 lines

drivers/vhost/net.c +6 -1

+		else
+			vhost_add_used(vq, head, 0);
 		if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
 			vhost_poll_queue(&vq->poll);
 			break;
 		}
 	}
 
+	if (likely(packets_coalesced &&
+		   bytes_coalesced &&
+		   bufs_coalesced))
+		vhost_signal(&net->dev, vq);
 	mutex_unlock(&vq->mutex);
 }
 
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Interesting. Could this is be a variant of the now famuous bufferbloat then?

Quoted 1 line

Interesting. Could this is be a variant of the now famuous

bufferbloat then?

Sigh, bufferbloat is the new global warming... :-/

Quoted 4 lines · David Miller

Quoted 1 line

Interesting. Could this is be a variant of the now famuous

bufferbloat then?

Sigh, bufferbloat is the new global warming... :-/

Yep, some places become colder, some other places become warmer; Same as BW results, sometimes faster, sometimes slower. :)

Shirley

OK, so thinking about it more, maybe the issue is this:
tx becomes full. We process one request and interrupt the guest, then it adds one request and the queue is full again.

Maybe the following will help it stabilize?
By itself it does nothing, but if you set
all the parameters to a huge value we will
only interrupt when we see an empty ring.
Which might be too much: pls try other values
in the middle: e.g. make bufs half the ring,
or bytes some small value, or packets some
small value etc.

Warning: completely untested.

drivers/vhost/net.c +10 -0

index aac05bc..6769cdc 100644
@@ -32,6 +32,13 @@
  * Using this limit prevents one virtqueue from starving others. */
 #define VHOST_NET_WEIGHT 0x80000
 
+int tx_bytes_coalesce = 0;
+module_param(tx_bytes_coalesce, int, 0644);
+int tx_bufs_coalesce = 0;
+module_param(tx_bufs_coalesce, int, 0644);
+int tx_packets_coalesce = 0;
+module_param(tx_packets_coalesce, int, 0644);
+
 enum {
    VHOST_NET_VQ_RX = 0,
    VHOST_NET_VQ_TX = 1,

@@ -127,6 +134,9 @@ static void handle_tx(struct vhost_net *net)
    int err, wmem;
    size_t hdr_size;
    struct socket *sock;
+   int bytes_coalesced = 0;
+   int bufs_coalesced = 0;
+   int packets_coalesced = 0;
 
    /* TODO: check that we are running from vhost_worker? */
    sock = rcu_dereference_check(vq->private_data, 1);

drivers/vhost/net.c +6 -0

+      if (unlikely(packets_coalesced > tx_packets_coalesce ||
+              bytes_coalesced > tx_bytes_coalesce ||
+              bufs_coalesced > tx_bufs_coalesce))
+         vhost_add_used_and_signal(&net->dev, vq, head, 0);
+      else
+         vhost_add_used(vq, head, 0);
       if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
          vhost_poll_queue(&vq->poll);
          break;

"Michael S. Tsirkin" <[email protected]> wrote on 01/28/2011 06:16:16 AM:

Quoted 44 lines

OK, so thinking about it more, maybe the issue is this:
tx becomes full. We process one request and interrupt the guest, then it adds one request and the queue is full again.

Maybe the following will help it stabilize?
By itself it does nothing, but if you set
all the parameters to a huge value we will
only interrupt when we see an empty ring.
Which might be too much: pls try other values
in the middle: e.g. make bufs half the ring,
or bytes some small value, or packets some
small value etc.

Warning: completely untested.

drivers/vhost/net.c +10 -0

index aac05bc..6769cdc 100644
@@ -32,6 +32,13 @@
  * Using this limit prevents one virtqueue from starving others. */
 #define VHOST_NET_WEIGHT 0x80000
 
+int tx_bytes_coalesce = 0;
+module_param(tx_bytes_coalesce, int, 0644);
+int tx_bufs_coalesce = 0;
+module_param(tx_bufs_coalesce, int, 0644);
+int tx_packets_coalesce = 0;
+module_param(tx_packets_coalesce, int, 0644);
+
 enum {
    VHOST_NET_VQ_RX = 0,
    VHOST_NET_VQ_TX = 1,

@@ -127,6 +134,9 @@ static void handle_tx(struct vhost_net *net)
    int err, wmem;
    size_t hdr_size;
    struct socket *sock;
+   int bytes_coalesced = 0;
+   int bufs_coalesced = 0;
+   int packets_coalesced = 0;
 
    /* TODO: check that we are running from vhost_worker? */
    sock = rcu_dereference_check(vq->private_data, 1);

drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 212

Quoted 8 lines

drivers/vhost/net.c +3 -1

@@ -196,14 +206,26 @@ static void handle_tx(struct vhost_net *net)
       if (err != len)
          pr_debug("Truncated TX packet: "
              " len %d != %zd\n", err, len);
-      vhost_add_used_and_signal(&net->dev, vq, head, 0);
       total_len += len;
+      packets_coalesced += 1;
+      bytes_coalesced += len;
+      bufs_coalesced += in;

Should this instead be:

      bufs_coalesced += out;

Perusing the code I see that earlier there is a check to see if "in" is not zero, and, if so, error out of the loop. After the check, "in" is not touched until it is added to bufs_coalesced, effectively not changing bufs_coalesced, meaning bufs_coalesced will never trigger the conditions below.

Or am I missing something?

Quoted 18 lines

drivers/vhost/net.c +10 -0

+      if (unlikely(packets_coalesced > tx_packets_coalesce ||
+              bytes_coalesced > tx_bytes_coalesce ||
+              bufs_coalesced > tx_bufs_coalesce))
+         vhost_add_used_and_signal(&net->dev, vq, head, 0);
+      else
+         vhost_add_used(vq, head, 0);
       if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
          vhost_poll_queue(&vq->poll);
          break;
       }
    }
 
+   if (likely(packets_coalesced > tx_packets_coalesce ||
+         bytes_coalesced > tx_bytes_coalesce ||
+         bufs_coalesced > tx_bufs_coalesce))
+      vhost_signal(&net->dev, vq);
    mutex_unlock(&vq->mutex);
 }

Quoted 1 line

Steve D.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 56 lines · Steve Dobbelstein

"Michael S. Tsirkin" <[email protected]> wrote on 01/28/2011 06:16:16 AM:

Quoted 44 lines

OK, so thinking about it more, maybe the issue is this:
tx becomes full. We process one request and interrupt the guest, then it adds one request and the queue is full again.

Maybe the following will help it stabilize?
By itself it does nothing, but if you set
all the parameters to a huge value we will
only interrupt when we see an empty ring.
Which might be too much: pls try other values
in the middle: e.g. make bufs half the ring,
or bytes some small value, or packets some
small value etc.

Warning: completely untested.

drivers/vhost/net.c +10 -0

index aac05bc..6769cdc 100644
@@ -32,6 +32,13 @@
  * Using this limit prevents one virtqueue from starving others. */
 #define VHOST_NET_WEIGHT 0x80000
 
+int tx_bytes_coalesce = 0;
+module_param(tx_bytes_coalesce, int, 0644);
+int tx_bufs_coalesce = 0;
+module_param(tx_bufs_coalesce, int, 0644);
+int tx_packets_coalesce = 0;
+module_param(tx_packets_coalesce, int, 0644);
+
 enum {
    VHOST_NET_VQ_RX = 0,
    VHOST_NET_VQ_TX = 1,

@@ -127,6 +134,9 @@ static void handle_tx(struct vhost_net *net)
    int err, wmem;
    size_t hdr_size;
    struct socket *sock;
+   int bytes_coalesced = 0;
+   int bufs_coalesced = 0;
+   int packets_coalesced = 0;
 
    /* TODO: check that we are running from vhost_worker? */
    sock = rcu_dereference_check(vq->private_data, 1);

drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 212

Quoted 8 lines

drivers/vhost/net.c +3 -1

@@ -196,14 +206,26 @@ static void handle_tx(struct vhost_net *net)
       if (err != len)
          pr_debug("Truncated TX packet: "
              " len %d != %zd\n", err, len);
-      vhost_add_used_and_signal(&net->dev, vq, head, 0);
       total_len += len;
+      packets_coalesced += 1;
+      bytes_coalesced += len;
+      bufs_coalesced += in;

Should this instead be:

Quoted 7 lines

      bufs_coalesced += out;

Perusing the code I see that earlier there is a check to see if "in" is not zero, and, if so, error out of the loop. After the check, "in" is not touched until it is added to bufs_coalesced, effectively not changing bufs_coalesced, meaning bufs_coalesced will never trigger the conditions below.

Yes. It definitely should be 'out'. 'in' should be 0 in the tx path.

I tried a simpler version of this patch without any tunables by delaying the signaling until we come out of the for loop.
It definitely reduced the number of vmexits significantly for small message guest to host stream test and the throughput went up a little.

drivers/vhost/net.c +3 -1

index 9b3ca10..5f9fae9 100644
@@ -197,7 +197,7 @@ static void handle_tx(struct vhost_net *net)
 		if (err != len)
 			pr_debug("Truncated TX packet: "
 				 " len %d != %zd\n", err, len);
-		vhost_add_used_and_signal(&net->dev, vq, head, 0);
+		vhost_add_used(vq, head, 0);
 		total_len += len;
 		if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
 			vhost_poll_queue(&vq->poll);

@@ -205,6 +205,8 @@ static void handle_tx(struct vhost_net *net)
 		}
 	}
 
+	if (total_len > 0)
+		vhost_signal(&net->dev, vq);
 	mutex_unlock(&vq->mutex);
 }
 

Quoted 22 lines

Or am I missing something?

Quoted 18 lines

drivers/vhost/net.c +10 -0

+      if (unlikely(packets_coalesced > tx_packets_coalesce ||
+              bytes_coalesced > tx_bytes_coalesce ||
+              bufs_coalesced > tx_bufs_coalesce))
+         vhost_add_used_and_signal(&net->dev, vq, head, 0);
+      else
+         vhost_add_used(vq, head, 0);
       if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
          vhost_poll_queue(&vq->poll);
          break;
       }
    }
 
+   if (likely(packets_coalesced > tx_packets_coalesce ||
+         bytes_coalesced > tx_bytes_coalesce ||
+         bufs_coalesced > tx_bufs_coalesce))
+      vhost_signal(&net->dev, vq);
    mutex_unlock(&vq->mutex);
 }

It is possible that we can miss signaling the guest even after processing a few pkts, if we don't hit any of these conditions.

Quoted 8 lines

Quoted 1 line

Steve D.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yes. It definitely should be 'out'. 'in' should be 0 in the tx path.

I tried a simpler version of this patch without any tunables by delaying the signaling until we come out of the for loop. It definitely reduced the number of vmexits significantly for small message
guest to host stream test and the throughput went up a little.

drivers/vhost/net.c +1 -1

index 9b3ca10..5f9fae9 100644
@@ -197,7 +197,7 @@ static void handle_tx(struct vhost_net *net)
                if (err != len)
                        pr_debug("Truncated TX packet: "
                                 " len %d != %zd\n", err, len);
-               vhost_add_used_and_signal(&net->dev, vq, head, 0);
+               vhost_add_used(vq, head, 0);
                total_len += len;
                if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
                        vhost_poll_queue(&vq->poll);

Quoted 22 lines · Sridhar Samudrala

Yes. It definitely should be 'out'. 'in' should be 0 in the tx path.

I tried a simpler version of this patch without any tunables by delaying the signaling until we come out of the for loop. It definitely reduced the number of vmexits significantly for small message
guest to host stream test and the throughput went up a little.

drivers/vhost/net.c +1 -1

index 9b3ca10..5f9fae9 100644
@@ -197,7 +197,7 @@ static void handle_tx(struct vhost_net *net)
                if (err != len)
                        pr_debug("Truncated TX packet: "
                                 " len %d != %zd\n", err, len);
-               vhost_add_used_and_signal(&net->dev, vq, head, 0);
+               vhost_add_used(vq, head, 0);
                total_len += len;
                if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
                        vhost_poll_queue(&vq->poll);

drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 211

Quoted 7 lines

drivers/vhost/net.c +2 -0

@@ -205,6 +205,8 @@ static void handle_tx(struct vhost_net *net)
                }
        }
 
+       if (total_len > 0)
+               vhost_signal(&net->dev, vq);
        mutex_unlock(&vq->mutex);
 }

Reducing the signaling will reduce the CPU utilization by reducing VM exits.

The small message BW is a problem we have seen faster guest/slow vhost, even I increased VHOST_NET_WEIGHT times, it didn't help that much for BW. For large message size, vhost is able to process all packets on time. I played around with guest/host codes, I only see huge BW improvement by dropping packets on guest side so far.

Thanks
Shirley

My theory is that the issue is not signalling.
Rather, our queue fills up, then host handles
one packet and sends an interrupt, and we
immediately wake the queue. So the vq
once it gets full, stays full.

From the printk debugging output, it might not be exactly the case. The

If you try my patch with bufs threshold set to e.g.
half the vq, what we will do is send interrupt after we have processed half the vq. So host has half the vq to go, and guest has half the vq to fill.

See?

Quoted 7 lines · Michael S. Tsirkin

My theory is that the issue is not signalling.
Rather, our queue fills up, then host handles
one packet and sends an interrupt, and we
immediately wake the queue. So the vq
once it gets full, stays full.

From the printk debugging output, it might not be exactly the case. The

ring gets full, run a bit, then gets full, then run a bit, then full...

Quoted 6 lines

If you try my patch with bufs threshold set to e.g.
half the vq, what we will do is send interrupt after we have processed half the vq. So host has half the vq to go, and guest has half the vq to fill.

See?

I am cleaning up my set up to run your patch ...

Shirley

Quoted 7 lines · Shirley Ma

Quoted 5 lines · Michael S. Tsirkin

My theory is that the issue is not signalling. Rather, our queue fills up, then host handles one packet and sends an interrupt, and we immediately wake the queue. So the vq once it gets full, stays full.

Quoted 1 line

Quoted 1 line

From the printk debugging output, it might not be exactly the case.

The

Quoted 1 line

ring gets full, run a bit, then gets full, then run a bit, then

full...

Yes, but does it get even half empty in between?

Quoted 13 lines · Michael S. Tsirkin

Quoted 8 lines · Shirley Ma

Quoted 7 lines · Michael S. Tsirkin

My theory is that the issue is not signalling. Rather, our queue fills up, then host handles one packet and sends an interrupt, and we immediately wake the queue. So the vq once it gets full, stays full.

From the printk debugging output, it might not be exactly the case.

The

Quoted 1 line

ring gets full, run a bit, then gets full, then run a bit, then

full...

Quoted 1 line

Yes, but does it get even half empty in between?

Sometimes, most of them not half of empty in between. But printk slow down the traffics, so it's not accurate. I think your patch will improve the performance if it signals guest when half of the ring size is empty.

But you manage signal by using TX bytes,

I would like to change it to
half of the ring size instead for signaling. Is that OK?

Shirley

There are flags for bytes, buffers and packets.
Try playing with any one of them :)
Just be sure to use v2.

Quoted 6 lines

I would like to change it to
half of the ring size instead for signaling. Is that OK?

Shirley

Sure that is why I made it a parameter so you can experiment.

Quoted 5 lines · Michael S. Tsirkin

There are flags for bytes, buffers and packets.
Try playing with any one of them :)
Just be sure to use v2.

Quoted 8 lines

Quoted 6 lines

I would like to change it to
half of the ring size instead for signaling. Is that OK?

Shirley

Sure that is why I made it a parameter so you can experiment.

The initial test results shows that the CPUs utilization has been reduced some, and BW has increased some with the default parameters, like 1K message size BW goes from 2.5Gb/s about 2.8Gb/s, CPU utilization down from 4x% to 38%, (Similar results from the patch I submitted a while ago to reduce signaling on vhost) but far away from dropping packet results.

I am going to change the code to use 1/2 ring size to wake the netif queue.

Shirley

ust tweak the parameters with sysfs, you do not have to edit the code:
echo 64 > /sys/module/vhost_net/parameters/tx_bufs_coalesce

Or in a similar way for tx_packets_coalesce (since we use indirect, packets will typically use 1 buffer each).

The way I am changing is only when netif queue has stopped, then we start to count num_free descriptors to send the signal to wake netif queue.

Quoted 4 lines · Shirley Ma

The way I am changing is only when netif queue has stopped, then we start to count num_free descriptors to send the signal to wake netif queue.

I forgot to mention, the code change I am making is in guest kernel, in xmit call back only wake up the queue when it's stopped && num_free >= 1/2 *vq->num, I add a new API in virtio_ring.

However vhost signaling reduction is needed as well. The patch I submitted a while ago showed both CPUs and BW improvement.

Thanks
Shirley

Quoted 3 lines · Shirley Ma

Quoted 2 lines · Shirley Ma

The way I am changing is only when netif queue has stopped, then

we

Quoted 1 line

Quoted 1 line

start to count num_free descriptors to send the signal to wake

netif

Quoted 3 lines

Quoted 1 line

queue.

I forgot to mention, the code change I am making is in guest kernel,

in

Quoted 3 lines

xmit call back only wake up the queue when it's stopped && num_free =
1/2 *vq->num, I add a new API in virtio_ring.

Interesting. Yes, I agree an API extension would be helpful. However, wouldn't just the signaling reduction be enough, without guest changes?

Quoted 5 lines

However vhost signaling reduction is needed as well. The patch I submitted a while ago showed both CPUs and BW improvement.

Thanks
Shirley

Which patch was that?

w/i guest change, I played around the parameters,for example: I could get 3.7Gb/s with 42% CPU BW increasing from 2.5Gb/s for 1K message size,
w/i dropping packet, I was able to get up to 6.2Gb/s with similar CPU usage.

Quoted 5 lines · Shirley Ma

w/i guest change, I played around the parameters,for example: I could get 3.7Gb/s with 42% CPU BW increasing from 2.5Gb/s for 1K message size,
w/i dropping packet, I was able to get up to 6.2Gb/s with similar CPU usage.

I meant w/o guest change, only vhost changes. Sorry about that.

Shirley

Quoted 2 lines · Shirley Ma

Quoted 1 line · Shirley Ma

w/i guest change, I played around the parameters,for example: I

could

Quoted 3 lines

Quoted 3 lines

get 3.7Gb/s with 42% CPU BW increasing from 2.5Gb/s for 1K message size,
w/i dropping packet, I was able to get up to 6.2Gb/s with similar

CPU

Quoted 5 lines

Quoted 1 line

usage.

I meant w/o guest change, only vhost changes. Sorry about that.

Shirley

Ah, excellent. What were the parameters?

Quoted 14 lines · Michael S. Tsirkin

Quoted 2 lines · Shirley Ma

Quoted 1 line · Shirley Ma

w/i guest change, I played around the parameters,for example: I

could

Quoted 3 lines

Quoted 3 lines

get 3.7Gb/s with 42% CPU BW increasing from 2.5Gb/s for 1K message size,
w/i dropping packet, I was able to get up to 6.2Gb/s with similar

CPU

Quoted 5 lines

Quoted 1 line

usage.

I meant w/o guest change, only vhost changes. Sorry about that.

Shirley

Quoted 1 line

Ah, excellent. What were the parameters?

I used half of the ring size 129 for packet counters, but the performance is still not as good as dropping packets on guest, 3.7 Gb/s vs. 6.2Gb/s.

Shirley

And this is with sndbuf=0 in host, yes?
And do you see a lot of tx interrupts?
How packets per interrupt?

Quoted 14 lines · Michael S. Tsirkin

Quoted 2 lines · Shirley Ma

Quoted 1 line · Shirley Ma

w/i guest change, I played around the parameters,for example: I

could

Quoted 3 lines

Quoted 3 lines

get 3.7Gb/s with 42% CPU BW increasing from 2.5Gb/s for 1K message size,
w/i dropping packet, I was able to get up to 6.2Gb/s with similar

CPU

Quoted 5 lines

Quoted 1 line

usage.

I meant w/o guest change, only vhost changes. Sorry about that.

Shirley

Quoted 1 line

Ah, excellent. What were the parameters?

I used half of the ring size 129 for packet counters,
but the
performance is still not as good as dropping packets on guest, 3.7 Gb/s vs. 6.2Gb/s.

Shirley

How many packets and bytes per interrupt are sent? Also, what about other values for the counters and other counters?

What does your patch do? Just drop packets instead of stopping the interface?

To have an understanding when should we drop packets in the guest, we need to know *why* does it help. Otherwise, how do we know it will work for others? Note that qdisc will drop packets when it overruns - so what is different? Also, are we over-running some other queue somewhere?

Quoted 15 lines · Michael S. Tsirkin

Quoted 3 lines · Shirley Ma

Quoted 2 lines · Shirley Ma

The way I am changing is only when netif queue has stopped, then

we

Quoted 1 line

Quoted 1 line

start to count num_free descriptors to send the signal to wake

netif

Quoted 3 lines

Quoted 1 line

queue.

I forgot to mention, the code change I am making is in guest kernel,

in

Quoted 3 lines

xmit call back only wake up the queue when it's stopped && num_free =
1/2 *vq->num, I add a new API in virtio_ring.

Quoted 3 lines

Interesting. Yes, I agree an API extension would be helpful. However, wouldn't just the signaling reduction be enough, without guest changes?

w/i guest change, I played around the parameters,for example: I could get 3.7Gb/s with 42% CPU BW increasing from 2.5Gb/s for 1K message size, w/i dropping packet, I was able to get up to 6.2Gb/s with similar CPU usage.

Quoted 7 lines

Quoted 5 lines

However vhost signaling reduction is needed as well. The patch I submitted a while ago showed both CPUs and BW improvement.

Thanks
Shirley

Which patch was that?

The patch was called "vhost: TX used buffer guest signal accumulation".

You suggested to split add_used_bufs and signal.

I am still thinking
what's the best approach to cooperate guest (virtio_kick) and vhost(handle_tx), vhost(signaling) and guest (xmit callback) to reduce the overheads, so I haven't submit the new patch yet.

Thanks
Shirley

Quoted 4 lines

The way I am changing is only when netif queue has stopped, then we start to count num_free descriptors to send the signal to wake netif queue.

I forgot to mention, the code change I am making is in guest kernel, in xmit call back only wake up the queue when it's stopped && num_free >= 1/2 *vq->num, I add a new API in virtio_ring.

However vhost signaling reduction is needed as well. The patch I submitted a while ago showed both CPUs and BW improvement.

Quoted 3 lines · Shirley Ma

Quoted 2 lines

The way I am changing is only when netif queue has stopped, then

we

Quoted 1 line

Quoted 1 line

start to count num_free descriptors to send the signal to wake

netif

Quoted 3 lines

Quoted 1 line

queue.

I forgot to mention, the code change I am making is in guest kernel,

in

Quoted 3 lines

xmit call back only wake up the queue when it's stopped && num_free =
1/2 *vq->num, I add a new API in virtio_ring.

FYI :)

I have tried this before. There are a couple of issues:

1. the free count will not reduce until you run free_old_xmit_skbs, which will not run anymore since the tx queue is stopped.
2. You cannot call free_old_xmit_skbs directly as it races with a queue that was just awakened (current cb was due to the delay in disabling cb's).

You have to call free_old_xmit_skbs() under netif_queue_stopped() check to avoid the race.

Quoted 8 lines · Shirley Ma

I have tried this before. There are a couple of issues:

1. the free count will not reduce until you run free_old_xmit_skbs, which will not run anymore since the tx queue is stopped.
2. You cannot call free_old_xmit_skbs directly as it races with a queue that was just awakened (current cb was due to the delay in disabling cb's).

Quoted 2 lines

You have to call free_old_xmit_skbs() under netif_queue_stopped() check to avoid the race.

Yes, that' what I did, when the netif queue stop, don't enable the queue, just free_old_xmit_skbs(), if not enough freed, then enabling callback until half of the ring size are freed, then wake the netif queue. But somehow I didn't reach the performance compared to drop packets, need to think about it more. :)

Quoted 9 lines · Shirley Ma

Quoted 4 lines

The way I am changing is only when netif queue has stopped, then we start to count num_free descriptors to send the signal to wake netif queue.

I forgot to mention, the code change I am making is in guest kernel, in xmit call back only wake up the queue when it's stopped && num_free >= 1/2 *vq->num, I add a new API in virtio_ring.

FYI :)

I have tried this before. There are a couple of issues:

1. the free count will not reduce until you run free_old_xmit_skbs, which will not run anymore since the tx queue is stopped.
2. You cannot call free_old_xmit_skbs directly as it races with a queue that was just awakened (current cb was due to the delay in disabling cb's).

You have to call free_old_xmit_skbs() under netif_queue_stopped() check to avoid the race.

I got a small improvement in my testing upto some number of threads (32 or 48?), but beyond that I was getting a regression.

Thanks,

- KK

Quoted 2 lines

However vhost signaling reduction is needed as well. The patch I submitted a while ago showed both CPUs and BW improvement.

Yes, I think doing this in the host is much simpler, just send an interrupt after there's a decent amount of space in the queue.

Having said that the simple heuristic that I coded might be a bit too simple.

From the debugging out what I have seen so far (a single small message

Quoted 8 lines · Michael S. Tsirkin

Yes, I think doing this in the host is much simpler, just send an interrupt after there's a decent amount of space in the queue.

Having said that the simple heuristic that I coded might be a bit too simple.

From the debugging out what I have seen so far (a single small message

TCP_STEAM test), I think the right approach is to patch both guest and vhost.

The problem I have found is a regression for single small message TCP_STEAM test. Old kernel works well for TCP_STREAM, only new kernel has problem.

For Steven's problem, it's multiple stream TCP_RR issues, the old guest doesn't perform well, so does new guest kernel. We tested reducing vhost signaling patch before, it didn't help the performance at all.

Thanks
Shirley

Quoted 9 lines · Shirley Ma

Quoted 8 lines · Michael S. Tsirkin

Yes, I think doing this in the host is much simpler, just send an interrupt after there's a decent amount of space in the queue.

Having said that the simple heuristic that I coded might be a bit too simple.

From the debugging out what I have seen so far (a single small

message

Quoted 1 line

TCP_STEAM test), I think the right approach is to patch both guest

and

Quoted 1 line

vhost.

One problem is slowing down the guest helps here. So there's a chance that just by adding complexity in guest driver we get a small improvement :(

We can't rely on a patched guest anyway, so
I think it is best to test guest and host changes separately.

And I do agree something needs to be done in guest too, for example when vqs share an interrupt, we might invoke a callback when we see vq is not empty even though it's not requested. Probably should check interrupts enabled here?

Quoted 2 lines

The problem I have found is a regression for single small message TCP_STEAM test. Old kernel works well for TCP_STREAM, only

new

Quoted 1 line

kernel has problem.

Likely new kernel is faster :)

Quoted 1 line

For Steven's problem, it's multiple stream TCP_RR issues, the old

guest

Quoted 1 line

doesn't perform well, so does new guest kernel. We tested reducing

vhost

Quoted 4 lines

signaling patch before, it didn't help the performance at all.

Thanks
Shirley

Yes, it seems unrelated to tx interrupts.

Quoted 19 lines · Michael S. Tsirkin

Quoted 9 lines · Shirley Ma

Quoted 8 lines · Michael S. Tsirkin

Yes, I think doing this in the host is much simpler, just send an interrupt after there's a decent amount of space in the queue.

Having said that the simple heuristic that I coded might be a bit too simple.

From the debugging out what I have seen so far (a single small

message

Quoted 1 line

TCP_STEAM test), I think the right approach is to patch both guest

and

Quoted 1 line

vhost.

One problem is slowing down the guest helps here. So there's a chance that just by adding complexity in guest driver we get a small improvement :(

Quoted 8 lines

We can't rely on a patched guest anyway, so
I think it is best to test guest and host changes separately.

And I do agree something needs to be done in guest too, for example when vqs share an interrupt, we might invoke a callback when we see vq is not empty even though it's not requested. Probably should check interrupts enabled here?

Yes, I modified xmit callback something like below:

static void skb_xmit_done(struct virtqueue *svq)

{
        struct virtnet_info *vi = svq->vdev->priv;

        /* Suppress further interrupts. */
        virtqueue_disable_cb(svq);

        /* We were probably waiting for more output buffers. */
        if (netif_queue_stopped(vi->dev)) {
                free_old_xmit_skbs(vi);
                if (virtqueue_free_size(svq)  <= svq->vring.num / 2) {
                        virtqueue_enable_cb(svq);
			return;
		}
        }
	netif_wake_queue(vi->dev);
}

Quoted 11 lines

Quoted 2 lines

The problem I have found is a regression for single small message TCP_STEAM test. Old kernel works well for TCP_STREAM, only

new

Quoted 1 line

kernel has problem.

Likely new kernel is faster :)

Quoted 1 line

For Steven's problem, it's multiple stream TCP_RR issues, the old

guest

Quoted 1 line

doesn't perform well, so does new guest kernel. We tested reducing

vhost

Quoted 6 lines

Quoted 4 lines

signaling patch before, it didn't help the performance at all.

Thanks
Shirley

Yes, it seems unrelated to tx interrupts.

The issue is more likely related to latency.

Do you have anything in
mind on how to reduce vhost latency?

Thanks
Shirley

OK, but this should have no effect with a vhost patch which should ensure that we don't get an interrupt until the queue is at least half empty. Right?

Quoted 4 lines

Quoted 2 lines

Yes, it seems unrelated to tx interrupts.

The issue is more likely related to latency.

Could be. Why do you think so?

Quoted 5 lines

Do you have anything in
mind on how to reduce vhost latency?

Thanks
Shirley

Hmm, bypassing the bridge might help a bit. Are you using tap+bridge or macvtap?

Quoted 4 lines · Michael S. Tsirkin

OK, but this should have no effect with a vhost patch which should ensure that we don't get an interrupt until the queue is at least half empty. Right?

There should be some coordination between guest and vhost.

We shouldn't
count the TX packets when netif queue is enabled since next guest TX xmit will free any used buffers in vhost. We need to be careful here in case we miss the interrupts when netif queue has stopped.

However we can't change old guest so we can test the patches separately for guest only, vhost only, and the combination.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Yes, it seems unrelated to tx interrupts.

The issue is more likely related to latency.

Could be. Why do you think so?

Since I played with latency hack, I can see performance difference for different latency.

Quoted 8 lines

Quoted 5 lines

Do you have anything in
mind on how to reduce vhost latency?

Thanks
Shirley

Hmm, bypassing the bridge might help a bit. Are you using tap+bridge or macvtap?

I am using tap+bridge for TCP_RR test, I think Steven tested macvtap before. He might have some data from his workload performance measurement.

Shirley

Quoted 6 lines · Shirley Ma

Quoted 4 lines · Michael S. Tsirkin

OK, but this should have no effect with a vhost patch which should ensure that we don't get an interrupt until the queue is at least half empty. Right?

Quoted 1 line

There should be some coordination between guest and vhost.

What kind of coordination? With a patched vhost, and a full ring. you should get an interrupt per 100 packets.
Is this what you see? And if yes, isn't the guest patch doing nothing then?

Quoted 3 lines

We shouldn't
count the TX packets when netif queue is enabled since next guest TX xmit will free any used buffers in vhost. We need to be careful here

in

Quoted 3 lines

case we miss the interrupts when netif queue has stopped.

However we can't change old guest so we can test the patches

separately

Quoted 7 lines

for guest only, vhost only, and the combination.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

Yes, it seems unrelated to tx interrupts.

The issue is more likely related to latency.

Quoted 3 lines

Quoted 1 line

Could be. Why do you think so?

Since I played with latency hack, I can see performance difference

for

Quoted 1 line

different latency.

Which hack was that?

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Shirley Ma

Quoted 4 lines · Michael S. Tsirkin

OK, but this should have no effect with a vhost patch which should ensure that we don't get an interrupt until the queue is at least half empty. Right?

There should be some coordination between guest and vhost.

Quoted 4 lines

What kind of coordination? With a patched vhost, and a full ring. you should get an interrupt per 100 packets.
Is this what you see? And if yes, isn't the guest patch doing nothing then?

vhost_signal won't be able send any TX interrupts to guest when guest TX interrupt is disabled. Guest TX interrupt is only enabled when running out of descriptors.

Quoted 17 lines

Quoted 3 lines

We shouldn't
count the TX packets when netif queue is enabled since next guest TX xmit will free any used buffers in vhost. We need to be careful here

in

Quoted 3 lines

case we miss the interrupts when netif queue has stopped.

However we can't change old guest so we can test the patches

separately

Quoted 9 lines

for guest only, vhost only, and the combination.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Yes, it seems unrelated to tx interrupts.

The issue is more likely related to latency.

Could be. Why do you think so?

Quoted 5 lines

Quoted 1 line

Since I played with latency hack, I can see performance difference

for

Quoted 1 line

different latency.

Which hack was that?

I tried to accumulate multiple guest to host notifications for TX xmits, it did help multiple streams TCP_RR results;

I also forced vhost
handle_tx to handle more packets; both hack seemed help.

Thanks
Shirley

Well, this is also the only case where the queue is stopped, no?

Quoted 1 line

I tried to accumulate multiple guest to host notifications for TX

xmits,

Quoted 1 line

it did help multiple streams TCP_RR results;

I don't see a point to delay used idx update, do you?

So delaying just signal seems better, right?

Haven't noticed that part, how does your patch make it

Quoted 1 line · Michael S. Tsirkin

Well, this is also the only case where the queue is stopped, no?

Yes. I got some debugging data, I saw that sometimes there were so many packets were waiting for free in guest between vhost_signal & guest xmit callback.

Looks like the time spent too long from vhost_signal to guest xmit callback?

Quoted 4 lines

Quoted 1 line

I tried to accumulate multiple guest to host notifications for TX

xmits,

Quoted 1 line

it did help multiple streams TCP_RR results;

I don't see a point to delay used idx update, do you?

It might cause per vhost handle_tx processed more packets.

Quoted 1 line

So delaying just signal seems better, right?

I think I need to define the test matrix to collect data for TX xmit from guest to host here for different tests.

Data to be collected:
---------------------
1. kvm_stat for VM, I/O exits
2. cpu utilization for both guest and host
3. cat /proc/interrupts on guest
4. packets rate from vhost handle_tx per loop
5. guest netif queue stop rate
6. how many packets are waiting for free between vhost signaling and guest callback
7. performance results

Test
----
1. TCP_STREAM single stream test for 1K to 4K message size
2. TCP_RR (64 instance test): 128 - 1K request/response size

Different hacks
---------------
1. Base line data ( with the patch to fix capacity check first, free_old_xmit_skbs returns number of skbs)

2. Drop packet data (will put some debugging in generic networking code)

3. Delay guest netif queue wake up until certain descriptors (1/2 ring size, 1/4 ring size...) are available once the queue has stopped.

4. Accumulate more packets per vhost signal in handle_tx?

5. 3 & 4 combinations

6. Accumulate more packets per guest kick() (TCP_RR) by adding a timer?

7. Accumulate more packets per vhost handle_tx() by adding some delay?

Quoted 1 line

Haven't noticed that part, how does your patch make it

handle more packets?

Added a delay in handle_tx().

What else?

It would take sometimes to do this.

Shirley

Quoted 3 lines · Michael S. Tsirkin

Quoted 1 line

Well, this is also the only case where the queue is stopped, no?

Yes. I got some debugging data, I saw that sometimes there were so

many

Quoted 1 line

packets were waiting for free in guest between vhost_signal & guest

xmit

Quoted 1 line

callback.

What does this mean?

Quoted 6 lines

Looks like the time spent too long from vhost_signal to guest xmit callback?

Quoted 1 line

Quoted 1 line

I tried to accumulate multiple guest to host notifications for

TX

Quoted 5 lines

Quoted 3 lines

xmits,

Quoted 1 line

it did help multiple streams TCP_RR results;

I don't see a point to delay used idx update, do you?

It might cause per vhost handle_tx processed more packets.

I don't understand. It's a couple of writes - what is the issue?

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Michael S. Tsirkin

Quoted 1 line

Well, this is also the only case where the queue is stopped, no?

Yes. I got some debugging data, I saw that sometimes there were so

many

Quoted 1 line

packets were waiting for free in guest between vhost_signal & guest

xmit

Quoted 1 line

callback.

What does this mean?

Let's look at the sequence here:

guest start_xmit()

	xmit_skb()
	if ring is full,
		enable_cb()

guest skb_xmit_done()
disable_cb,
printk free_old_xmit_skbs <-- it was between more than 1/2 to
full ring size

	printk vq->num_free 

vhost handle_tx()
if (guest interrupt is enabled)
signal guest to free xmit buffers

So between guest queue full/stopped queue/enable call back to guest receives the callback from host to free_old_xmit_skbs, there were about 1/2 to full ring size descriptors available. I thought there were only a few. (I disabled your vhost patch for this test.)

Quoted 7 lines

Quoted 7 lines

Looks like the time spent too long from vhost_signal to guest xmit callback?

Quoted 1 line

Quoted 1 line

I tried to accumulate multiple guest to host notifications for

Quoted 8 lines

TX

Quoted 5 lines

Quoted 3 lines

xmits,

Quoted 1 line

it did help multiple streams TCP_RR results;

I don't see a point to delay used idx update, do you?

It might cause per vhost handle_tx processed more packets.

I don't understand. It's a couple of writes - what is the issue?

Oh, handle_tx could process more packets per loop for multiple streams TCP_RR case. I need to print out the data rate per loop to confirm this.

Shirley

Quoted 10 lines

Let's look at the sequence here:

guest start_xmit()

      xmit_skb()
      if ring is full,
              enable_cb()

guest skb_xmit_done()
disable_cb,
printk free_old_xmit_skbs <-- it was between more than 1/2

to

Quoted 9 lines

full ring size

      printk vq->num_free 

vhost handle_tx()
if (guest interrupt is enabled)
signal guest to free xmit buffers

So between guest queue full/stopped queue/enable call back to guest receives the callback from host to free_old_xmit_skbs, there were

about

Quoted 1 line

1/2 to full ring size descriptors available. I thought there were

only a

Quoted 1 line

few. (I disabled your vhost patch for this test.)

The expected number is vq->num - max skb frags - 2.

Quoted 26 lines · Michael S. Tsirkin

Quoted 10 lines

Let's look at the sequence here:

guest start_xmit()

      xmit_skb()
      if ring is full,
              enable_cb()

guest skb_xmit_done()
disable_cb,
printk free_old_xmit_skbs <-- it was between more than 1/2

to

Quoted 9 lines

full ring size

      printk vq->num_free 

vhost handle_tx()
if (guest interrupt is enabled)
signal guest to free xmit buffers

So between guest queue full/stopped queue/enable call back to guest receives the callback from host to free_old_xmit_skbs, there were

about

Quoted 1 line

1/2 to full ring size descriptors available. I thought there were

only a

Quoted 1 line

few. (I disabled your vhost patch for this test.)

Quoted 1 line

The expected number is vq->num - max skb frags - 2.

It was various (up to the ring size 256). This is using indirection buffers, it returned how many freed descriptors, not number of buffers.

Why do you think it is vq->num - max skb frags - 2 here?

Shirley

Quoted 19 lines

I think I need to define the test matrix to collect data for TX xmit from guest to host here for different tests.

Data to be collected:
---------------------
1. kvm_stat for VM, I/O exits
2. cpu utilization for both guest and host
3. cat /proc/interrupts on guest
4. packets rate from vhost handle_tx per loop
5. guest netif queue stop rate
6. how many packets are waiting for free between vhost signaling and guest callback
7. performance results

Test
----
1. TCP_STREAM single stream test for 1K to 4K message size
2. TCP_RR (64 instance test): 128 - 1K request/response size

Quoted 6 lines

Different hacks
---------------
1. Base line data ( with the patch to fix capacity check first, free_old_xmit_skbs returns number of skbs)

2. Drop packet data (will put some debugging in generic networking

code)

Quoted 1 line

3. Delay guest netif queue wake up until certain descriptors (1/2

ring

Quoted 7 lines

size, 1/4 ring size...) are available once the queue has stopped.

4. Accumulate more packets per vhost signal in handle_tx?

5. 3 & 4 combinations

6. Accumulate more packets per guest kick() (TCP_RR) by adding a

timer?

Quoted 2 lines

7. Accumulate more packets per vhost handle_tx() by adding some

delay?

Quoted 11 lines

Quoted 2 lines

Haven't noticed that part, how does your patch make it

handle more packets?

Added a delay in handle_tx().

What else?

It would take sometimes to do this.

Shirley

Need to think about this.

Quoted 19 lines · Michael S. Tsirkin

Quoted 19 lines

I think I need to define the test matrix to collect data for TX xmit from guest to host here for different tests.

Data to be collected:
---------------------
1. kvm_stat for VM, I/O exits
2. cpu utilization for both guest and host
3. cat /proc/interrupts on guest
4. packets rate from vhost handle_tx per loop
5. guest netif queue stop rate
6. how many packets are waiting for free between vhost signaling and guest callback
7. performance results

Test
----
1. TCP_STREAM single stream test for 1K to 4K message size
2. TCP_RR (64 instance test): 128 - 1K request/response size

Quoted 7 lines

Quoted 6 lines

Different hacks
---------------
1. Base line data ( with the patch to fix capacity check first, free_old_xmit_skbs returns number of skbs)

2. Drop packet data (will put some debugging in generic networking

code)

Since I found that the netif queue stop/wake up is so expensive, I created a dropping packets patch on guest side so I don't need to debug generic networking code.

guest start_xmit()

	capacity = free_old_xmit_skb() + virtqueue_get_num_freed()
	if (capacity == 0)
		drop this packet;
		return;

In the patch, both guest TX interrupts and callback have been omitted. Host vhost_signal in handle_tx can totally be removed as well. (A new virtio_ring API is needed for exporting total of num_free descriptors here -- virtioqueue_get_num_freed)

Initial TCP_STREAM performance results I got for guest to local host 4.2Gb/s for 1K message size, (vs. 2.5Gb/s)
6.2Gb/s for 2K message size, and (vs. 3.8Gb/s)
9.8Gb/s for 4K message size. (vs.5.xGb/s)

Since large message size (64K) doesn't hit (capacity == 0) case, so the performance only has a little better. (from 13.xGb/s to 14.x Gb/s)

kvm_stat output shows significant exits reduction for both VM and I/O, no guest TX interrupts.

With dropping packets, TCP retrans has been increased here, so I can see performance numbers are various.

This might be not a good solution, but it gave us some ideas on expensive netif queue stop/wake up between guest and host notification.

I couldn't find a better solution on how to reduce netif queue stop/wake up rate for small message size. But I think once we can address this, the guest TX performance will burst for small message size.

I also compared this with return TX_BUSY approach when (capacity == 0), it is not as good as dropping packets.

Quoted 29 lines

Quoted 1 line

3. Delay guest netif queue wake up until certain descriptors (1/2

ring

Quoted 7 lines

size, 1/4 ring size...) are available once the queue has stopped.

4. Accumulate more packets per vhost signal in handle_tx?

5. 3 & 4 combinations

6. Accumulate more packets per guest kick() (TCP_RR) by adding a

timer?

Quoted 2 lines

7. Accumulate more packets per vhost handle_tx() by adding some

delay?

Quoted 11 lines

Quoted 2 lines

Haven't noticed that part, how does your patch make it

handle more packets?

Added a delay in handle_tx().

What else?

It would take sometimes to do this.

Shirley

Need to think about this.

Quoted 1 line

Initial TCP_STREAM performance results I got for guest to local

host

Quoted 3 lines

4.2Gb/s for 1K message size, (vs. 2.5Gb/s)
6.2Gb/s for 2K message size, and (vs. 3.8Gb/s)
9.8Gb/s for 4K message size. (vs.5.xGb/s)

What is the average packet size, # bytes per ack, and the # of interrupts
per packet? It could be that just slowing down trahsmission makes GSO work better.

Quoted 10 lines · Michael S. Tsirkin

Quoted 1 line

Initial TCP_STREAM performance results I got for guest to local

host

Quoted 3 lines

4.2Gb/s for 1K message size, (vs. 2.5Gb/s)
6.2Gb/s for 2K message size, and (vs. 3.8Gb/s)
9.8Gb/s for 4K message size. (vs.5.xGb/s)

What is the average packet size, # bytes per ack, and the # of interrupts
per packet? It could be that just slowing down trahsmission makes GSO work better.

There is no TX interrupts with dropping packet.

GSO/TSO is the key for small message performance, w/o GSO/TSO, the performance is limited to about 2Gb/s no matter how big the message size it is. I think any work we try here will increase large packet size rate. BTW for dropping packet, TCP increased fast retrans, not slow start.

I will collect tcpdump, netstart before and after data to compare packet size/rate w/o w/i the patch.

Thanks
Shirley

Just a thought: does it help to make tx queue len of the virtio device smaller?

"Michael S. Tsirkin" <[email protected]> wrote on 01/28/2011 06:16:16 AM:

Quoted 44 lines

OK, so thinking about it more, maybe the issue is this:
tx becomes full. We process one request and interrupt the guest, then it adds one request and the queue is full again.

Maybe the following will help it stabilize?
By itself it does nothing, but if you set
all the parameters to a huge value we will
only interrupt when we see an empty ring.
Which might be too much: pls try other values
in the middle: e.g. make bufs half the ring,
or bytes some small value, or packets some
small value etc.

Warning: completely untested.

drivers/vhost/net.c +10 -0

index aac05bc..6769cdc 100644
@@ -32,6 +32,13 @@
  * Using this limit prevents one virtqueue from starving others. */
 #define VHOST_NET_WEIGHT 0x80000
 
+int tx_bytes_coalesce = 0;
+module_param(tx_bytes_coalesce, int, 0644);
+int tx_bufs_coalesce = 0;
+module_param(tx_bufs_coalesce, int, 0644);
+int tx_packets_coalesce = 0;
+module_param(tx_packets_coalesce, int, 0644);
+
 enum {
    VHOST_NET_VQ_RX = 0,
    VHOST_NET_VQ_TX = 1,

@@ -127,6 +134,9 @@ static void handle_tx(struct vhost_net *net)
    int err, wmem;
    size_t hdr_size;
    struct socket *sock;
+   int bytes_coalesced = 0;
+   int bufs_coalesced = 0;
+   int packets_coalesced = 0;
 
    /* TODO: check that we are running from vhost_worker? */
    sock = rcu_dereference_check(vq->private_data, 1);

drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 212

Quoted 8 lines

drivers/vhost/net.c +3 -1

@@ -196,14 +206,26 @@ static void handle_tx(struct vhost_net *net)
       if (err != len)
          pr_debug("Truncated TX packet: "
              " len %d != %zd\n", err, len);
-      vhost_add_used_and_signal(&net->dev, vq, head, 0);
       total_len += len;
+      packets_coalesced += 1;
+      bytes_coalesced += len;
+      bufs_coalesced += in;

Should this instead be:

      bufs_coalesced += out;

Perusing the code I see that earlier there is a check to see if "in" is not zero, and, if so, error out of the loop. After the check, "in" is not touched until it is added to bufs_coalesced, effectively not changing bufs_coalesced, meaning bufs_coalesced will never trigger the conditions below.

Or am I missing something?

Quoted 9 lines

drivers/vhost/net.c +6 -0

+      if (unlikely(packets_coalesced > tx_packets_coalesce ||
+              bytes_coalesced > tx_bytes_coalesce ||
+              bufs_coalesced > tx_bufs_coalesce))
+         vhost_add_used_and_signal(&net->dev, vq, head, 0);
+      else
+         vhost_add_used(vq, head, 0);
       if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
          vhost_poll_queue(&vq->poll);
          break;

drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 231

Quoted 10 lines

drivers/vhost/net.c +4 -0

       }
    }
 
+   if (likely(packets_coalesced > tx_packets_coalesce ||
+         bytes_coalesced > tx_bytes_coalesce ||
+         bufs_coalesced > tx_bufs_coalesce))
+      vhost_signal(&net->dev, vq);
    mutex_unlock(&vq->mutex);
 }
 

Steve D.

Quoted 4 lines · David Miller

Quoted 1 line

Interesting. Could this is be a variant of the now famuous

bufferbloat then?

Sigh, bufferbloat is the new global warming... :-/

Yep, some places become colder, some other places become warmer; Same as BW results, sometimes faster, sometimes slower. :)

Shirley

Quoted 17 lines · Shirley Ma

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Shirley Ma

Quoted 4 lines · David Miller

Quoted 1 line

Interesting. Could this is be a variant of the now famuous

bufferbloat then?

Sigh, bufferbloat is the new global warming... :-/

Quoted 8 lines

Quoted 1 line

Yep, some places become colder, some other places become warmer;

Same as

Quoted 3 lines

BW results, sometimes faster, sometimes slower. :)

Shirley

Sent a tuning patch (v2) that might help.
Could you try it and play with the module parameters please?

Quoted 8 lines

Hello Michael,

Sure I will play with this patch to see how it could help.

I am looking at guest side as well, I found a couple issues on guest side:

1. free_old_xmit_skbs() should return the number of skbs instead of

the

Quoted 14 lines

total of sgs since we are using ring size to stop/start netif queue. static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)

{
        struct sk_buff *skb;
        unsigned int len, tot_sgs = 0;

        while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
                pr_debug("Sent skb %p\n", skb);
                vi->dev->stats.tx_bytes += skb->len;
                vi->dev->stats.tx_packets++;
                tot_sgs += skb_vnet_hdr(skb)->num_sg;
                dev_kfree_skb_any(skb);
        }
        return tot_sgs; <---- should return numbers of skbs to track

Quoted 6 lines

ring usage here, I think;
}

Did the old guest use number of buffers to track ring usage before?

2. In start_xmit, I think we should move capacity +=

free_old_xmit_skbs

Quoted 4 lines

before netif_stop_queue(); so we avoid unnecessary netif queue stop/start. This condition is heavily hit for small message size.

Also we capacity checking condition should change to something like

half

Quoted 6 lines

of the vring.num size, instead of comparing 2+MAX_SKB_FRAGS?

       if (capacity < 2+MAX_SKB_FRAGS) {
                netif_stop_queue(dev);
                if (unlikely(!virtqueue_enable_cb(vi->svq))) {
                        /* More just got used, free them then

recheck.

Quoted 10 lines

*/

                        capacity += free_old_xmit_skbs(vi);
                        if (capacity >= 2+MAX_SKB_FRAGS) {
                                netif_start_queue(dev);
                                virtqueue_disable_cb(vi->svq);
                        }
                }
        }

3. Looks like the xmit callback is only used to wake the queue when

the

Quoted 14 lines

queue has stopped, right? Should we put a condition check here? static void skb_xmit_done(struct virtqueue *svq)

{
        struct virtnet_info *vi = svq->vdev->priv;

        /* Suppress further interrupts. */
        virtqueue_disable_cb(svq);

        /* We were probably waiting for more output buffers. */
--->   if (netif_queue_stopped(vi->dev))
        netif_wake_queue(vi->dev);
}

Quoted 1 line

Shirley

Well the return value is used to calculate capacity and that counts the # of s/g. No?

From cache utilization POV it might be better to read from the skb and not peek at virtio header though...
Pls Cc the lists on any discussions in the future.

Signature · 1 line

MST

Quoted 85 lines · Michael S. Tsirkin

Quoted 17 lines · Shirley Ma

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Shirley Ma

Quoted 4 lines · David Miller

Quoted 1 line

Interesting. Could this is be a variant of the now famuous

bufferbloat then?

Sigh, bufferbloat is the new global warming... :-/

Quoted 8 lines

Quoted 1 line

Yep, some places become colder, some other places become warmer;

Same as

Quoted 3 lines

BW results, sometimes faster, sometimes slower. :)

Shirley

Sent a tuning patch (v2) that might help.
Could you try it and play with the module parameters please?

Quoted 8 lines

Hello Michael,

Sure I will play with this patch to see how it could help.

I am looking at guest side as well, I found a couple issues on guest side:

1. free_old_xmit_skbs() should return the number of skbs instead of

the

Quoted 14 lines

total of sgs since we are using ring size to stop/start netif queue. static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)

{
        struct sk_buff *skb;
        unsigned int len, tot_sgs = 0;

        while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
                pr_debug("Sent skb %p\n", skb);
                vi->dev->stats.tx_bytes += skb->len;
                vi->dev->stats.tx_packets++;
                tot_sgs += skb_vnet_hdr(skb)->num_sg;
                dev_kfree_skb_any(skb);
        }
        return tot_sgs; <---- should return numbers of skbs to track

Quoted 6 lines

ring usage here, I think;
}

Did the old guest use number of buffers to track ring usage before?

2. In start_xmit, I think we should move capacity +=

free_old_xmit_skbs

Quoted 4 lines

before netif_stop_queue(); so we avoid unnecessary netif queue stop/start. This condition is heavily hit for small message size.

Also we capacity checking condition should change to something like

half

Quoted 6 lines

of the vring.num size, instead of comparing 2+MAX_SKB_FRAGS?

       if (capacity < 2+MAX_SKB_FRAGS) {
                netif_stop_queue(dev);
                if (unlikely(!virtqueue_enable_cb(vi->svq))) {
                        /* More just got used, free them then

recheck.

Quoted 10 lines

*/

                        capacity += free_old_xmit_skbs(vi);
                        if (capacity >= 2+MAX_SKB_FRAGS) {
                                netif_start_queue(dev);
                                virtqueue_disable_cb(vi->svq);
                        }
                }
        }

3. Looks like the xmit callback is only used to wake the queue when

the

Quoted 14 lines

queue has stopped, right? Should we put a condition check here? static void skb_xmit_done(struct virtqueue *svq)

{
        struct virtnet_info *vi = svq->vdev->priv;

        /* Suppress further interrupts. */
        virtqueue_disable_cb(svq);

        /* We were probably waiting for more output buffers. */
--->   if (netif_queue_stopped(vi->dev))
        netif_wake_queue(vi->dev);
}

Quoted 4 lines

Quoted 1 line

Shirley

Well the return value is used to calculate capacity and that counts the # of s/g. No?

Nope, the current guest kernel uses descriptors not number of sgs.

not sure the old guest.

Quoted 6 lines

From cache utilization POV it might be better to read from the skb and not peek at virtio header though...
Pls Cc the lists on any discussions in the future.

Signature · 1 line

MST

Sorry I missed reply all. :(

Shirley

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Shirley

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 8 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

Another failure mode is when skb_xmit_done wakes the queue: it might be too early, there might not be space for the next packet in the vq yet.

A solution might be to keep some kind of pool around for indirect, we wanted to do it for block anyway ...

Quoted 19 lines

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 8 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

Quoted 3 lines

Another failure mode is when skb_xmit_done wakes the queue: it might be too early, there might not be space for the next packet in the vq yet.

I am not sure if this is the problem - shouldn't you see these messages:

	if (likely(capacity == -ENOMEM)) {
		dev_warn(&dev->dev,
			"TX queue failure: out of memory\n");
	} else {
		dev->stats.tx_fifo_errors++;
		dev_warn(&dev->dev,
			"Unexpected TX queue failure: %d\n",
			capacity);
	}

in next xmit? I am not getting this in my testing.

Quoted 2 lines

A solution might be to keep some kind of pool around for indirect, we wanted to do it for block anyway ...

Your vhost patch should fix this automatically. Right?

Thanks,

- KK

Quoted 10 lines · On Wed, Feb 02, 2011 at 10:09:18AM +0530, Krishna Kumar2 wrote:

Quoted 10 lines

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 7 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · On Wed, Feb 02, 2011 at 10:09:18AM +0530, Krishna Kumar2 wrote:

Quoted 10 lines

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 7 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Subject: virtio: CONFIG_VIRTIO_STATS

For performance problems we'd like to know exactly what the ring looks like. This patch adds stats indexed by how-full-ring-is; we could extend it to also record them by how-used-ring-is if we need.

Signed-off-by Rusty Russell <[email protected]>

drivers/virtio/Kconfig +8 -0

@@ -7,6 +7,14 @@ config VIRTIO_RING
 	tristate
 	depends on VIRTIO
 
+config VIRTIO_STATS
+	bool "Virtio debugging stats (EXPERIMENTAL)"
+	depends on VIRTIO_RING
+	select DEBUG_FS
+	---help---
+	  Virtio stats collected by how full the ring is at any time,
+	  presented under debugfs/virtio/<name>-<vq>/<num-used>/
+
 config VIRTIO_PCI
 	tristate "PCI driver for virtio devices (EXPERIMENTAL)"
 	depends on PCI && EXPERIMENTAL

drivers/virtio/virtio_ring.c +116 -2

@@ -21,6 +21,7 @@
 #include <linux/virtio_config.h>
 #include <linux/device.h>
 #include <linux/slab.h>
+#include <linux/debugfs.h>
 
 /* virtio guest is communicating with a virtual "device" that actually runs on
  * a host processor.  Memory barriers are used to control SMP effects. */

@@ -95,6 +96,11 @@ struct vring_virtqueue
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);
 
+#ifdef CONFIG_VIRTIO_STATS
+	struct vring_stat *stats;
+	struct dentry *statdir;
+#endif
+
 #ifdef DEBUG
 	/* They're supposed to lock for us. */
 	unsigned int in_use;

@@ -106,6 +112,87 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+#ifdef CONFIG_VIRTIO_STATS
+/* We have an array of these, indexed by how full the ring is. */
+struct vring_stat {
+	/* How many interrupts? */
+	size_t interrupt_nowork, interrupt_work;
+	/* How many non-notify kicks, how many notify kicks, how many add notify? */
+	size_t kick_no_notify, kick_notify, add_notify;
+	/* How many adds? */
+	size_t add_direct, add_indirect, add_fail;
+	/* How many gets? */
+	size_t get;
+	/* How many disable callbacks? */
+	size_t disable_cb;
+	/* How many enables? */
+	size_t enable_cb_retry, enable_cb_success;
+};
+
+static struct dentry *virtio_stats;
+
+static void create_stat_files(struct vring_virtqueue *vq)
+{
+	char name[80];
+	unsigned int i;
+
+	/* Racy in theory, but we don't care. */
+	if (!virtio_stats)
+		virtio_stats = debugfs_create_dir("virtio-stats", NULL);
+
+	sprintf(name, "%s-%s", dev_name(&vq->vq.vdev->dev), vq->vq.name);
+	vq->statdir = debugfs_create_dir(name, virtio_stats);
+
+	for (i = 0; i < vq->vring.num; i++) {
+		struct dentry *dir;
+
+		sprintf(name, "%i", i);
+		dir = debugfs_create_dir(name, vq->statdir);
+		debugfs_create_size_t("interrupt_nowork", 0400, dir,
+				      &vq->stats[i].interrupt_nowork);
+		debugfs_create_size_t("interrupt_work", 0400, dir,
+				      &vq->stats[i].interrupt_work);
+		debugfs_create_size_t("kick_no_notify", 0400, dir,
+				      &vq->stats[i].kick_no_notify);
+		debugfs_create_size_t("kick_notify", 0400, dir,
+				      &vq->stats[i].kick_notify);
+		debugfs_create_size_t("add_notify", 0400, dir,
+				      &vq->stats[i].add_notify);
+		debugfs_create_size_t("add_direct", 0400, dir,
+				      &vq->stats[i].add_direct);
+		debugfs_create_size_t("add_indirect", 0400, dir,
+				      &vq->stats[i].add_indirect);
+		debugfs_create_size_t("add_fail", 0400, dir,
+				      &vq->stats[i].add_fail);
+		debugfs_create_size_t("get", 0400, dir,
+				      &vq->stats[i].get);
+		debugfs_create_size_t("disable_cb", 0400, dir,
+				      &vq->stats[i].disable_cb);
+		debugfs_create_size_t("enable_cb_retry", 0400, dir,
+				      &vq->stats[i].enable_cb_retry);
+		debugfs_create_size_t("enable_cb_success", 0400, dir,
+				      &vq->stats[i].enable_cb_success);
+	}
+}
+
+static void delete_stat_files(struct vring_virtqueue *vq)
+{
+	debugfs_remove_recursive(vq->statdir);
+}
+
+#define add_stat(vq, name)						\
+	do {								\
+		struct vring_virtqueue *_vq = (vq);			\
+		_vq->stats[_vq->num_free - _vq->vring.num].name++;	\
+	} while (0)
+
+#else
+#define add_stat(vq, name)
+static void delete_stat_files(struct vring_virtqueue *vq)
+{
+}
+#endif
+
 /* Set up an indirect table of descriptors and add it to the queue. */
 static int vring_add_indirect(struct vring_virtqueue *vq,
 			      struct scatterlist sg[],

@@ -121,6 +208,8 @@ static int vring_add_indirect(struct vri
 	if (!desc)
 		return -ENOMEM;
 
+	add_stat(vq, add_indirect);
+
 	/* Transfer entries from the sg list into the indirect page */
 	for (i = 0; i < out; i++) {
 		desc[i].flags = VRING_DESC_F_NEXT;

@@ -183,17 +272,22 @@ int virtqueue_add_buf_gfp(struct virtque
 	BUG_ON(out + in == 0);
 
 	if (vq->num_free < out + in) {
+		add_stat(vq, add_fail);
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);
 		/* FIXME: for historical reasons, we force a notify here if
 		 * there are outgoing parts to the buffer.  Presumably the
 		 * host should service the ring ASAP. */
-		if (out)
+		if (out) {
+			add_stat(vq, add_notify);
 			vq->notify(&vq->vq);
+		}
 		END_USE(vq);
 		return -ENOSPC;
 	}
 
+	add_stat(vq, add_direct);
+
 	/* We're about to use some buffers from the free list. */
 	vq->num_free -= out + in;
 

@@ -248,9 +342,12 @@ void virtqueue_kick(struct virtqueue *_v
 	/* Need to update avail index before checking if we should notify */
 	virtio_mb();
 
-	if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
+	if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) {
+		add_stat(vq, kick_notify);
 		/* Prod other side to tell it about changes. */
 		vq->notify(&vq->vq);
+	} else
+		add_stat(vq, kick_no_notify);
 
 	END_USE(vq);
 }

@@ -294,6 +391,8 @@ void *virtqueue_get_buf(struct virtqueue
 
 	START_USE(vq);
 
+	add_stat(vq, get);
+
 	if (unlikely(vq->broken)) {
 		END_USE(vq);
 		return NULL;

@@ -333,6 +432,7 @@ void virtqueue_disable_cb(struct virtque
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
+	add_stat(vq, disable_cb);
 	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
 }
 EXPORT_SYMBOL_GPL(virtqueue_disable_cb);

@@ -348,10 +448,12 @@ bool virtqueue_enable_cb(struct virtqueu
 	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
 	virtio_mb();
 	if (unlikely(more_used(vq))) {
+		add_stat(vq, enable_cb_retry);
 		END_USE(vq);
 		return false;
 	}
 
+	add_stat(vq, enable_cb_success);
 	END_USE(vq);
 	return true;
 }

@@ -387,10 +489,12 @@ irqreturn_t vring_interrupt(int irq, voi
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
 	if (!more_used(vq)) {
+		add_stat(vq, interrupt_nowork);
 		pr_debug("virtqueue interrupt with no work for %p\n", vq);
 		return IRQ_NONE;
 	}
 
+	add_stat(vq, interrupt_work);
 	if (unlikely(vq->broken))
 		return IRQ_HANDLED;
 

@@ -451,6 +555,15 @@ struct virtqueue *vring_new_virtqueue(un
 	}
 	vq->data[i] = NULL;
 
+#ifdef CONFIG_VIRTIO_STATS
+	vq->stats = kzalloc(sizeof(*vq->stats) * num, GFP_KERNEL);
+	if (!vq->stats) {
+		kfree(vq);
+		return NULL;
+	}
+	create_stat_files(vq);
+#endif
+
 	return &vq->vq;
 }
 EXPORT_SYMBOL_GPL(vring_new_virtqueue);

@@ -458,6 +571,7 @@ EXPORT_SYMBOL_GPL(vring_new_virtqueue);
 void vring_del_virtqueue(struct virtqueue *vq)
 {
 	list_del(&vq->list);
+	delete_stat_files(to_vvq(vq));
 	kfree(to_vvq(vq));
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);

Quoted 33 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · On Wed, Feb 02, 2011 at 10:09:18AM +0530, Krishna Kumar2 wrote:

Quoted 10 lines

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 7 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Subject: virtio: CONFIG_VIRTIO_STATS

For performance problems we'd like to know exactly what the ring looks like. This patch adds stats indexed by how-full-ring-is; we could extend it to also record them by how-used-ring-is if we need.

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

Not sure whether the intent is to merge this. If yes - would it make sense to use tracing for this instead? That's what kvm does.

Quoted 34 lines · Michael S. Tsirkin

Quoted 33 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · On Wed, Feb 02, 2011 at 10:09:18AM +0530, Krishna Kumar2 wrote:

Quoted 10 lines

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 7 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Subject: virtio: CONFIG_VIRTIO_STATS

For performance problems we'd like to know exactly what the ring looks like. This patch adds stats indexed by how-full-ring-is; we could extend it to also record them by how-used-ring-is if we need.

Quoted 5 lines

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

Not sure whether the intent is to merge this. If yes - would it make sense to use tracing for this instead? That's what kvm does.

Intent wasn't; I've not used tracepoints before, but maybe we should consider a longer-term monitoring solution?

Patch welcome!

Cheers,
Rusty.

Quoted 44 lines · Rusty Russell

Quoted 34 lines · Michael S. Tsirkin

Quoted 33 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · On Wed, Feb 02, 2011 at 10:09:18AM +0530, Krishna Kumar2 wrote:

Quoted 10 lines

"Michael S. Tsirkin" <[email protected]> 02/02/2011 03:11 AM

Quoted 7 lines · Shirley Ma

Quoted 2 lines · Michael S. Tsirkin

Confused. We compare capacity to skb frags, no?
That's sg I think ...

Current guest kernel use indirect buffers, num_free returns how many available descriptors not skb frags. So it's wrong here.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Shirley

I see. Good point. In other words when we complete the buffer it was indirect, but when we add a new one we
can not allocate indirect so we consume.
And then we start the queue and add will fail.
I guess we need some kind of API to figure out whether the buf we complete was indirect?

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Subject: virtio: CONFIG_VIRTIO_STATS

For performance problems we'd like to know exactly what the ring looks like.  This patch adds stats indexed by how-full-ring-is; we could extend it to also record them by how-used-ring-is if we need.

Quoted 5 lines

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

Not sure whether the intent is to merge this. If yes - would it make sense to use tracing for this instead? That's what kvm does.

Intent wasn't; I've not used tracepoints before, but maybe we should consider a longer-term monitoring solution?

Quoted 4 lines

Patch welcome!

Cheers,
Rusty.

Sure, I'll look into this.

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Quoted 5 lines · Rusty Russell

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Should we also add similar stat on vhost vq as well for monitoring vhost_signal & vhost_notify?

Shirley

--
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

Tom L has started using Rusty's patches and found some interesting results, sent yesterday:
http://marc.info/?l=kvm&m=129953710930124&w=2

Quoted 7 lines · Shirley Ma

Quoted 5 lines · Rusty Russell

I've finally read this thread... I think we need to get more serious with our stats gathering to diagnose these kind of performance issues.

This is a start; it should tell us what is actually happening to the virtio ring(s) without significant performance impact...

Quoted 2 lines

Should we also add similar stat on vhost vq as well for monitoring vhost_signal & vhost_notify?

Tom L has started using Rusty's patches and found some interesting results, sent yesterday:
http://marc.info/?l=kvm&m=129953710930124&w=2

Also, it looks like vhost is sending a lot of notifications for packets it has received before the guest can get scheduled to disable notifications and begin processing the packets resulting in some lock contention in the guest (and high interrupt rates).

Here's an old patch where I played with implementing this:

virtio: put last_used and last_avail index into ring itself.

Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.

Fortunately, we have room to expand:

the ring is always a whole number
of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Signed-off-by Rusty Russell <[email protected]>

---

include/linux/virtio_ring.h +5 -0

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)

include/linux/virtio_ring.h +0 -0

 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Quoted 1 line · Rusty Russell

Here's an old patch where I played with implementing this:

...

Quoted 9 lines

virtio: put last_used and last_avail index into ring itself.

Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.

Quoted 1 line

Fortunately, we have room to expand:

This seems to be true for x86 kvm and lguest but is it true for s390?

Will this last bit work on s390?
If I understand correctly the memory is allocated by host there?

Quoted 14 lines · Michael S. Tsirkin

Quoted 1 line · Rusty Russell

Here's an old patch where I played with implementing this:

...

Quoted 9 lines

virtio: put last_used and last_avail index into ring itself.

Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.

Quoted 4 lines

Quoted 1 line

Fortunately, we have room to expand:

This seems to be true for x86 kvm and lguest but is it true for s390?

Yes, as the ring is page aligned so there's always room.

Quoted 2 lines

Will this last bit work on s390?
If I understand correctly the memory is allocated by host there?

They have to offer the feature, so if the have some way of allocating non-page-aligned amounts of memory, they'll have to add those extra 2 bytes.

So I think it's OK...
Rusty.

Quoted 14 lines · Michael S. Tsirkin

Quoted 1 line · Rusty Russell

Here's an old patch where I played with implementing this:

...

Quoted 9 lines

virtio: put last_used and last_avail index into ring itself.

Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.

Quoted 4 lines

Quoted 1 line

Fortunately, we have room to expand:

This seems to be true for x86 kvm and lguest but is it true for s390?

Yes, as the ring is page aligned so there's always room.

Quoted 2 lines

Will this last bit work on s390?
If I understand correctly the memory is allocated by host there?

They have to offer the feature, so if the have some way of allocating non-page-aligned amounts of memory, they'll have to add those extra 2 bytes.

So I think it's OK...
Rusty.

Quoted 6 lines · Rusty Russell

They have to offer the feature, so if the have some way of allocating non-page-aligned amounts of memory, they'll have to add those extra 2 bytes.

So I think it's OK...
Rusty.

To clarify, my concern is that we always seem to try to map these extra 2 bytes, which thinkably might fail?

I am working on a KVM network performance issue found in our lab running the DayTrader benchmark. The benchmark throughput takes a significant hit when running the application server in a KVM guest verses on bare metal. We have dug into the problem and found that DayTrader's use of small packets exposes KVM's overhead of handling network packets. I have been able to reproduce the performance hit with a simpler setup using the netperf benchmark with the TCP_RR test and the request and response sizes set to 256 bytes. I run the benchmark between two physical systems, each using a 1GB link. In order to get the maximum throughput for the system I have to run 100 instances of netperf. When I run the netserver processes in a guest, I see a maximum throughput that is 51% of what I get if I run the netserver processes directly on the host. The CPU utilization in the guest is only 85% at maximum throughput, whereas it is 100% on bare metal.

The KVM host has 16 CPUs. The KVM guest is configured with 2 VCPUs. When
I run netperf on the host I boot the host with maxcpus=2 on the kernel
command line. The host is running the current KVM upstream kernel along
with the current upstream qemu. Here is the qemu command used to launch
the guest:
/build/qemu-kvm/x86_64-softmmu/qemu-system-x86_64 -name glasgow-RH60 -m 32768 -drive file=/build/guest-data/glasgow-RH60.img,if=virtio,index=0,boot=on
-drive file=/dev/virt/WAS,if=virtio,index=1 -net nic,model=virtio,vlan=3,macaddr=00:1A:64:E5:00:63,netdev=nic0 -netdev tap,id=nic0,vhost=on -smp 2
-vnc :1 -monitor telnet::4499,server,nowait -serial telnet::8899,server,nowait --mem-path /libhugetlbfs -daemonize

We have tried various proposed fixes, each with varying amounts of success. One such fix was to add code to the vhost thread such that when it found the work queue empty it wouldn't just exit the thread but rather would delay for 50 microseconds and then recheck the queue. If there was work on the queue it would loop back and process it, else it would exit the thread. The change got us a 13% improvement in the DayTrader throughput.

Running the same netperf configuration on the same hardware but using a
different hypervisor gets us significantly better throughput numbers. The
guest on that hypervisor runs at 100% CPU utilization. The various fixes we have tried have not gotten us close to the throughput seen on the other hypervisor. I'm looking for ideas/input from the KVM experts on how to make KVM perform better when handling small packets.

Thanks,
Steve

--
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

Quoted 2 lines

I am working on a KVM network performance issue found in our lab

Quoted 1 line

the DayTrader benchmark. The benchmark throughput takes a significant

Quoted 1 line

when running the application server in a KVM guest verses on bare

Quoted 2 lines

We have dug into the problem and found that DayTrader's use of small packets exposes KVM's overhead of handling network packets. I have

Quoted 2 lines

able to reproduce the performance hit with a simpler setup using the netperf benchmark with the TCP_RR test and the request and response

Quoted 1 line

set to 256 bytes. I run the benchmark between two physical systems,

Quoted 1 line

using a 1GB link. In order to get the maximum throughput for the

Quoted 1 line

have to run 100 instances of netperf. When I run the netserver

Quoted 1 line

in a guest, I see a maximum throughput that is 51% of what I get if I

Quoted 1 line

the netserver processes directly on the host. The CPU utilization in

Quoted 1 line

guest is only 85% at maximum throughput, whereas it is 100% on bare

You are stressing the scheduler pretty hard with this test :) Is your real benchmark also using a huge number of threads?

If it's not, you might be seeing a different issue. IOW, the netperf degradation might not be network-related at all, but have to do with speed of context switch in guest. Thoughts?