From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43513) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXNZm-0004iI-Bm for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:05:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXNZh-0002hE-0s for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:05:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXNZg-0002gi-Ok for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:05:12 -0400 References: <1470266160-17896-1-git-send-email-mst@redhat.com> <20160804093515.05627a95.cornelia.huck@de.ibm.com> <20160804225038-mutt-send-email-mst@kernel.org> <20160805110211.385ae55b.cornelia.huck@de.ibm.com> From: Jason Wang Message-ID: <51601385-bb67-7ab6-61fe-4346df3db717@redhat.com> Date: Wed, 10 Aug 2016 15:05:04 +0800 MIME-Version: 1.0 In-Reply-To: <20160805110211.385ae55b.cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On 2016=E5=B9=B408=E6=9C=8805=E6=97=A5 17:02, Cornelia Huck wrote: > On Thu, 4 Aug 2016 22:52:29 +0300 > "Michael S. Tsirkin" wrote: > >> On Thu, Aug 04, 2016 at 09:35:15AM +0200, Cornelia Huck wrote: >>> On Thu, 4 Aug 2016 02:16:14 +0300 >>> "Michael S. Tsirkin" wrote: >>> >>>> This allows increasing the rx queue size up to 1024: unlike with tx, >>>> guests don't put in huge S/G lists into RX so the risk of running in= to >>>> the max 1024 limitation due to some off-by-one seems small. >>>> >>>> It's helpful for users like OVS-DPDK which don't do any buffering on= the >>>> host - 1K roughly matches 500 entries in tun + 256 in the current rx >>>> queue, which seems to work reasonably well. We could probably make d= o >>>> with ~750 entries but virtio spec limits us to powers of two. >>>> It might be a good idea to specify an s/g size limit in a future >>>> version. >>>> >>>> It also might be possible to make the queue size smaller down the ro= ad, 64 >>>> seems like the minimal value which will still work (as guests seem t= o >>>> assume a queue full of 1.5K buffers is enough to process the largest >>>> incoming packet, which is ~64K). No one actually asked for this, an= d >>>> with virtio 1 guests can reduce ring size without need for host >>>> configuration, so don't bother with this for now. >>> Do we need some kind of sanity check that the guest did not resize >>> below a reasonable limit? >> Unfortunately the spec does not have an interface for that. >> Guests expect they can get away with any size. > Might be a good idea to add this in the future, so that the guest is > able to discover the minimum and the host can refuse to work if the > configured queue is too small. > > (I can easily reject the setup ccw on virtio-ccw, but is there an > elegant way to refuse setting up the queues with virtio-pci?) > >>>> Signed-off-by: Michael S. Tsirkin >>>> --- >>>> include/hw/virtio/virtio-net.h | 1 + >>>> hw/net/virtio-net.c | 22 +++++++++++++++++++++- >>>> 2 files changed, 22 insertions(+), 1 deletion(-) >>>> >>> >>>> @@ -1716,10 +1717,28 @@ static void virtio_net_device_realize(Device= State *dev, Error **errp) >>>> VirtIONet *n =3D VIRTIO_NET(dev); >>>> NetClientState *nc; >>>> int i; >>>> + int min_rx_queue_size; >>>> >>>> virtio_net_set_config_size(n, n->host_features); >>>> virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size)= ; >>>> >>>> + /* >>>> + * We set a lower limit on RX queue size to what it always was. >>>> + * Guests that want a smaller ring can always resize it without >>>> + * help from us (using virtio 1 and up). >>>> + */ >>>> + min_rx_queue_size =3D 256; >>> I'd find it more readable to introduce a #define with the old queue >>> size as the minimum size... >>> >>>> + if (n->net_conf.rx_queue_size < min_rx_queue_size || >>>> + n->net_conf.rx_queue_size > VIRTQUEUE_MAX_SIZE || >>>> + (n->net_conf.rx_queue_size & (n->net_conf.rx_queue_size - 1= ))) { >>>> + error_setg(errp, "Invalid rx_queue_size (=3D %" PRIu16 "), = " >>>> + "must be a power of 2 between %d and %d.", >>>> + n->net_conf.rx_queue_size, min_rx_queue_size, >>>> + VIRTQUEUE_MAX_SIZE); >>>> + virtio_cleanup(vdev); >>>> + return; >>>> + } >>>> + >>>> n->max_queues =3D MAX(n->nic_conf.peers.queues, 1); >>>> if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) { >>>> error_setg(errp, "Invalid number of queues (=3D %" PRIu32 = "), " >>>> @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] =3D { >>>> TX_TIMER_INTERVAL), >>>> DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX= _BURST), >>>> DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx), >>>> + DEFINE_PROP_UINT16("rx_queue_size", VirtIONet, net_conf.rx_queu= e_size, 256), >>> ...and defaulting to that #define (or one derived from the #define >>> above) here. >> These happen to be the same, but they are in fact >> unrelated: one is the default, the other is the >> min value. > Hm... > > /* previously fixed value */ > #define VIRTIO_NET_RX_DEFAULT_SIZE 256 > /* for now, only allow larger queues; with virtio-1, guest can downsize= */ > #define VIRTIO_NET_RX_MIN_SIZE VIRTIO_NET_RX_DEFAULT_SIZE > > This would allow getting rid of the new local variable and gets us a > speaking define in the property definition. > > This makes sense. What's your opinion Michael? Thanks