From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afjsi-0001cF-6T for qemu-devel@nongnu.org; Tue, 15 Mar 2016 03:59:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afjse-0006vL-41 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 03:59:08 -0400 Received: from e06smtp09.uk.ibm.com ([195.75.94.105]:38410) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afjsd-0006un-Qp for qemu-devel@nongnu.org; Tue, 15 Mar 2016 03:59:04 -0400 Received: from localhost by e06smtp09.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Mar 2016 07:59:00 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 3070917D8059 for ; Tue, 15 Mar 2016 07:59:29 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2F7wwtJ63701244 for ; Tue, 15 Mar 2016 07:58:58 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2F7wvPQ004754 for ; Tue, 15 Mar 2016 03:58:58 -0400 Date: Tue, 15 Mar 2016 08:58:54 +0100 From: Greg Kurz Message-ID: <20160315085854.4b1a6ffe@bahia.huguette.org> In-Reply-To: <20160314122755.5aa27f05@bahia.huguette.org> References: <1448636559-2140-1-git-send-email-phermansson@gmail.com> <20160314122755.5aa27f05@bahia.huguette.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Patrik Hermansson Cc: qemu-devel@nongnu.org, mst@redhat.com On Mon, 14 Mar 2016 12:27:55 +0100 Greg Kurz wrote: > On Fri, 27 Nov 2015 16:02:39 +0100 > Patrik Hermansson wrote: > > > This patch adds the option to specify virtio queue sizes. Currently the > > queue sizes is hard coded to 256, which might not be suitable for all types > > of applications. This patch makes it possible to specify the queue size between > > 256 to 1024. > > > > The minimum value is chosen based on the current sizes of the virtio > > queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX. > > --- > > Michael, any thoughts on this patch ? > > Patrik, I have a few remarks. See below. > > SoB tag is missing. > > > hw/net/virtio-net.c | 31 ++++++++++++++++++++++++++++--- > > include/hw/virtio/virtio-net.h | 8 +++++++- > > 2 files changed, 35 insertions(+), 4 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index a877614..c4fcb39 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque) > > } > > } > > > > +static void virtio_net_validate_queue_limit(uint16_t *queue_size) > > +{ > > + if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) { > > + error_report("queue-size: %d, exceeds maximum allowed queue-size(%d)," > > + "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX, > > It is encouraged to keep format strings passed to error_report() on 1 line. It > is easier to grep. > > > + VIRTIO_QUEUE_SIZE_MAX); > > + *queue_size = VIRTIO_QUEUE_SIZE_MAX; > > + } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) { > > + error_report("queue-size: %d, below minimum allowed queue-size(%d)," > > + "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN, > > Same remark. > > > + VIRTIO_QUEUE_SIZE_MIN); > > + *queue_size = VIRTIO_QUEUE_SIZE_MIN; > > + } > > +} > > + > > static void virtio_net_add_queue(VirtIONet *n, int index) > > { > > VirtIODevice *vdev = VIRTIO_DEVICE(n); > > > > - n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx); > > + virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz); > > + virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz); > > + n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz, > > + virtio_net_handle_rx); > > if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) { > > n->vqs[index].tx_vq = > > - virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer); > > + virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz, > > + virtio_net_handle_tx_timer); > > n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, > > virtio_net_tx_timer, > > &n->vqs[index]); > > } else { > > n->vqs[index].tx_vq = > > - virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh); > > + virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz, > > + virtio_net_handle_tx_bh); > > n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]); > > } > > > > @@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = { > > DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer, > > TX_TIMER_INTERVAL), > > DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST), > > + DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz, > > + RX_VIRTQUEUE_SIZE), > > + DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz, > > + TX_VIRTQUEUE_SIZE), > > + > > DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx), > > DEFINE_PROP_END_OF_LIST(), > > }; > > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > > index f3cc25f..dbcabbf 100644 > > --- a/include/hw/virtio/virtio-net.h > > +++ b/include/hw/virtio/virtio-net.h > > @@ -28,13 +28,19 @@ > > * ensures fairness in the io path. 256 conveniently matches the > > * length of the TX queue and shows a good balance of performance > > * and latency. */ > > -#define TX_BURST 256 > > TX_BURST is still used by: > > DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST) > /me found his missing eye and read one line further... > > +#define VIRTIO_QUEUE_SIZE_MIN 256 > > +#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE > > +#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN > > +#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN > > +#define TX_BURST TX_VIRTQUEUE_SIZE > > ... please ignore the comment above :) > > typedef struct virtio_net_conf > > { > > uint32_t txtimer; > > int32_t txburst; > > char *tx; > > + uint16_t rx_virtqueue_sz; > > + uint16_t tx_virtqueue_sz; > > } virtio_net_conf; > > > > /* Maximum packet size we can receive from tap device: header + 64k */ > > >