From: Jason Wang <jasowang@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>,
virtualization@lists.osdl.org, netdev@vger.kernel.org,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] vhost-net: Unify the code of mergeable and big buffer handling
Date: Tue, 18 Jan 2011 11:05:33 +0800 [thread overview]
Message-ID: <19765.893.776528.869640@gargle.gargle.HOWL> (raw)
In-Reply-To: <20110117083644.GA23479@redhat.com>
Michael S. Tsirkin writes:
> On Mon, Jan 17, 2011 at 04:11:08PM +0800, Jason Wang wrote:
> > Codes duplication were found between the handling of mergeable and big
> > buffers, so this patch tries to unify them. This could be easily done
> > by adding a quota to the get_rx_bufs() which is used to limit the
> > number of buffers it returns (for mergeable buffer, the quota is
> > simply UIO_MAXIOV, for big buffers, the quota is just 1), and then the
> > previous handle_rx_mergeable() could be resued also for big buffers.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> We actually started this way. However the code turned out
> to have measureable overhead when handle_rx_mergeable
> is called with quota 1.
> So before applying this I'd like to see some data
> to show this is not the case anymore.
>
I've run a round of test (Host->Guest) for these three patches on my desktop:
Without these patches
mergeable buffers:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.42 (10.66.91.42) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 575.87 69.20 26.36 39.375 7.499
87380 16384 256 60.01 1123.57 73.16 34.73 21.335 5.064
87380 16384 512 60.00 1351.12 75.26 35.80 18.251 4.341
87380 16384 1024 60.00 1955.31 74.73 37.67 12.523 3.156
87380 16384 2048 60.01 3411.92 74.82 39.49 7.186 1.896
bug buffers:
Netperf test results
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.109 (10.66.91.109) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 567.10 72.06 26.13 41.638 7.550
87380 16384 256 60.00 1143.69 74.66 32.58 21.392 4.667
87380 16384 512 60.00 1460.92 73.94 33.40 16.585 3.746
87380 16384 1024 60.00 3454.85 77.49 33.89 7.349 1.607
87380 16384 2048 60.00 3781.11 76.51 38.38 6.631 1.663
With these patches:
mergeable buffers:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.236 (10.66.91.236) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 657.53 71.27 26.42 35.517 6.583
87380 16384 256 60.00 1217.73 74.34 34.67 20.004 4.665
87380 16384 512 60.00 1575.25 75.27 37.12 15.658 3.861
87380 16384 1024 60.00 2416.07 74.77 37.20 10.140 2.522
87380 16384 2048 60.00 3702.29 77.31 36.29 6.842 1.606
big buffers:
Netperf test results
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.202 (10.66.91.202) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 647.67 71.86 27.26 36.356 6.895
87380 16384 256 60.00 1265.82 76.19 36.54 19.724 4.729
87380 16384 512 60.00 1796.64 76.06 39.48 13.872 3.601
87380 16384 1024 60.00 4008.37 77.05 36.47 6.299 1.491
87380 16384 2048 60.00 4468.56 75.18 41.79 5.513 1.532
Looks like the unification does not hurt the performance, and with those patches
we can get some improvement. BTW, the regression of mergeable buffer still
exist.
> > ---
> > drivers/vhost/net.c | 128 +++------------------------------------------------
> > 1 files changed, 7 insertions(+), 121 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 95e49de..c32a2e4 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -227,6 +227,7 @@ static int peek_head_len(struct sock *sk)
> > * @iovcount - returned count of io vectors we fill
> > * @log - vhost log
> > * @log_num - log offset
> > + * @quota - headcount quota, 1 for big buffer
> > * returns number of buffer heads allocated, negative on error
> > */
> > static int get_rx_bufs(struct vhost_virtqueue *vq,
> > @@ -234,7 +235,8 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > int datalen,
> > unsigned *iovcount,
> > struct vhost_log *log,
> > - unsigned *log_num)
> > + unsigned *log_num,
> > + unsigned int quota)
> > {
> > unsigned int out, in;
> > int seg = 0;
> > @@ -242,7 +244,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > unsigned d;
> > int r, nlogs = 0;
> >
> > - while (datalen > 0) {
> > + while (datalen > 0 && headcount < quota) {
> > if (unlikely(seg >= UIO_MAXIOV)) {
> > r = -ENOBUFS;
> > goto err;
> > @@ -282,116 +284,7 @@ err:
> >
> > /* Expects to be always run from workqueue - which acts as
> > * read-size critical section for our kind of RCU. */
> > -static void handle_rx_big(struct vhost_net *net)
> > -{
> > - struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > - unsigned out, in, log, s;
> > - int head;
> > - struct vhost_log *vq_log;
> > - struct msghdr msg = {
> > - .msg_name = NULL,
> > - .msg_namelen = 0,
> > - .msg_control = NULL, /* FIXME: get and handle RX aux data. */
> > - .msg_controllen = 0,
> > - .msg_iov = vq->iov,
> > - .msg_flags = MSG_DONTWAIT,
> > - };
> > -
> > - struct virtio_net_hdr hdr = {
> > - .flags = 0,
> > - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> > - };
> > -
> > - size_t len, total_len = 0;
> > - int err;
> > - size_t hdr_size;
> > - struct socket *sock = rcu_dereference(vq->private_data);
> > - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > - return;
> > -
> > - mutex_lock(&vq->mutex);
> > - vhost_disable_notify(vq);
> > - hdr_size = vq->vhost_hlen;
> > -
> > - vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > - vq->log : NULL;
> > -
> > - for (;;) {
> > - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > - ARRAY_SIZE(vq->iov),
> > - &out, &in,
> > - vq_log, &log);
> > - /* On error, stop handling until the next kick. */
> > - if (unlikely(head < 0))
> > - break;
> > - /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > - if (unlikely(vhost_enable_notify(vq))) {
> > - /* They have slipped one in as we were
> > - * doing that: check again. */
> > - vhost_disable_notify(vq);
> > - continue;
> > - }
> > - /* Nothing new? Wait for eventfd to tell us
> > - * they refilled. */
> > - break;
> > - }
> > - /* We don't need to be notified again. */
> > - if (out) {
> > - vq_err(vq, "Unexpected descriptor format for RX: "
> > - "out %d, int %d\n",
> > - out, in);
> > - break;
> > - }
> > - /* Skip header. TODO: support TSO/mergeable rx buffers. */
> > - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> > - msg.msg_iovlen = in;
> > - len = iov_length(vq->iov, in);
> > - /* Sanity check */
> > - if (!len) {
> > - vq_err(vq, "Unexpected header len for RX: "
> > - "%zd expected %zd\n",
> > - iov_length(vq->hdr, s), hdr_size);
> > - break;
> > - }
> > - err = sock->ops->recvmsg(NULL, sock, &msg,
> > - len, MSG_DONTWAIT | MSG_TRUNC);
> > - /* TODO: Check specific error and bomb out unless EAGAIN? */
> > - if (err < 0) {
> > - vhost_discard_vq_desc(vq, 1);
> > - break;
> > - }
> > - /* TODO: Should check and handle checksum. */
> > - if (err > len) {
> > - pr_debug("Discarded truncated rx packet: "
> > - " len %d > %zd\n", err, len);
> > - vhost_discard_vq_desc(vq, 1);
> > - continue;
> > - }
> > - len = err;
> > - err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> > - if (err) {
> > - vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> > - vq->iov->iov_base, err);
> > - break;
> > - }
> > - len += hdr_size;
> > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > - if (unlikely(vq_log))
> > - vhost_log_write(vq, vq_log, log, len);
> > - total_len += len;
> > - if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > - vhost_poll_queue(&vq->poll);
> > - break;
> > - }
> > - }
> > -
> > - mutex_unlock(&vq->mutex);
> > -}
> > -
> > -/* Expects to be always run from workqueue - which acts as
> > - * read-size critical section for our kind of RCU. */
> > -static void handle_rx_mergeable(struct vhost_net *net)
> > +static void handle_rx(struct vhost_net *net)
> > {
> > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > unsigned uninitialized_var(in), log;
> > @@ -431,7 +324,8 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > sock_len += sock_hlen;
> > vhost_len = sock_len + vhost_hlen;
> > headcount = get_rx_bufs(vq, vq->heads, vhost_len,
> > - &in, vq_log, &log);
> > + &in, vq_log, &log,
> > + likely(mergeable) ? UIO_MAXIOV : 1);
> > /* On error, stop handling until the next kick. */
> > if (unlikely(headcount < 0))
> > break;
> > @@ -497,14 +391,6 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > mutex_unlock(&vq->mutex);
> > }
> >
> > -static void handle_rx(struct vhost_net *net)
> > -{
> > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> > - handle_rx_mergeable(net);
> > - else
> > - handle_rx_big(net);
> > -}
> > -
> > static void handle_tx_kick(struct vhost_work *work)
> > {
> > struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
next prev parent reply other threads:[~2011-01-18 3:05 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-01-17 8:10 [PATCH 1/3] vhost-net: check the support of mergeable buffer outside the receive loop Jason Wang
2011-01-17 8:11 ` [PATCH 2/3] vhost-net: Unify the code of mergeable and big buffer handling Jason Wang
2011-01-17 8:36 ` Michael S. Tsirkin
2011-01-18 3:05 ` Jason Wang [this message]
2011-01-18 4:37 ` Michael S. Tsirkin
2011-01-18 7:41 ` Jason Wang
2011-01-17 8:11 ` [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len() Jason Wang
2011-01-17 9:33 ` Eric Dumazet
2011-01-17 9:57 ` Michael S. Tsirkin
2011-03-13 15:06 ` Michael S. Tsirkin
2011-03-13 15:52 ` Eric Dumazet
2011-03-13 16:19 ` Michael S. Tsirkin
2011-03-13 16:32 ` Eric Dumazet
2011-03-13 16:43 ` Michael S. Tsirkin
2011-03-13 17:41 ` Eric Dumazet
2011-03-13 21:11 ` Michael S. Tsirkin
2011-01-17 8:46 ` [PATCH 1/3] vhost-net: check the support of mergeable buffer outside the receive loop Michael S. Tsirkin
2011-01-18 4:26 ` Jason Wang
2011-01-18 4:36 ` Michael S. Tsirkin
2011-01-18 9:15 ` Jason Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=19765.893.776528.869640@gargle.gargle.HOWL \
--to=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=virtualization@lists.osdl.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®