Re: [PATCH] virtio_net tx performance fix

On Mon, 2008-01-28 at 09:32 -0600, Anthony Liguori wrote:

Hi Dor,

How are you measuring performance?  The numbers I've gotten with netperf 
before and after your patch are:

tx - 647.27mbit
rx - 89.22

tx - 27.82
rx - 79.93

I've been testing with iperf (patched with Ingo's fix).
I also tested tcp/udp (udp tx is only 550Mbps with the patch, w/o it's
only 220Mbps)

So this patch is pretty much killing performance for netperf.

Did you have hrtimer configured on the host?

Dor Laor wrote:

quoted

There was a problem with the location of the notify call in
add_buff function:
When VRING_USED_F_NO_NOTIFY is set, the host does not kick the
guest when packets were transmitted, as a result the guest runs
out of tx buffers sometimes.

But even if F_NO_NOTIFY is set, if the tx buffer is full, we notify the 
guest, so this prevents that from happening.

quoted

 This is fine but the problem lies
when add_buf fails, it called notify and the host sends all the
pending tx pkts. When enable_cb was called, more_used(vq) returned
false so eventually the skb was dropped.
  

I'm having a tough time following this part.  If add_buf fails, we 
notify unconditionally (which is, I think what we want).  I'm not sure 
how that relates to a packet getting dropped though.

Here is start_xmit function:

again:
	/* Free up any pending old buffers before queueing new ones. */
	free_old_xmit_skbs(vi);
	err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb);

<<< here add_buf might fail since the ring is full.
<<< once it fails, the original code called notify which triggered the
<<< host to send all the pending tx so all descriptors are used.

	if (err) {
		vi->stats.sendq_full++;

		pr_debug("%s: virtio not prepared to send\n",dev->name);
		netif_stop_queue(dev);

		/* Activate callback for using skbs: if this fails it
		 * means some were used in the meantime. */
		vi->stats.sendq_enabled++;
		if (unlikely(!vi->svq->vq_ops->enable_cb(vi->svq))) {

<<< Since before the patch enable_cb returned true, thus the goto below
<<< was not called.
<<< The patch moves the notify to enable_cb above.

			printk("Unlikely: restart svq failed\n");
			vi->stats.sendq_enable_failed++;
			netif_start_queue(dev);
			goto again;
		}
		__skb_unlink(skb, &vi->send);

		return NETDEV_TX_BUSY;
	}

Regards,

Anthony Liguori