* Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size
2016-08-03 23:16 [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size Michael S. Tsirkin
@ 2016-08-03 23:17 ` Michael S. Tsirkin
2016-08-04 2:03 ` Jason Wang
2016-08-04 7:35 ` Cornelia Huck
2 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2016-08-03 23:17 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang
On Thu, Aug 04, 2016 at 02:16:14AM +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 into
> 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 do
> 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 road, 64
> seems like the minimal value which will still work (as guests seem to
> 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, and
> with virtio 1 guests can reduce ring size without need for host
> configuration, so don't bother with this for now.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Forgot to say: this was originally suggested by Patrik Hermansson
<phermansson@gmail.com>
> ---
> include/hw/virtio/virtio-net.h | 1 +
> hw/net/virtio-net.c | 22 +++++++++++++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 91ed97c..0ced975 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -35,6 +35,7 @@ typedef struct virtio_net_conf
> uint32_t txtimer;
> int32_t txburst;
> char *tx;
> + uint16_t rx_queue_size;
> } virtio_net_conf;
>
> /* Maximum packet size we can receive from tap device: header + 64k */
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 01f1351..4e595e9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1412,7 +1412,8 @@ 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);
> + n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_queue_size,
> + 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);
> @@ -1716,10 +1717,28 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> VirtIONet *n = 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 = 256;
> + 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 (= %" 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 = MAX(n->nic_conf.peers.queues, 1);
> if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
> error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] = {
> 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_queue_size, 256),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> MST
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size
2016-08-03 23:16 [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size Michael S. Tsirkin
2016-08-03 23:17 ` Michael S. Tsirkin
@ 2016-08-04 2:03 ` Jason Wang
2016-08-04 7:35 ` Cornelia Huck
2 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2016-08-04 2:03 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
On 2016年08月04日 07:16, 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 into
> 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 do
> 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 road, 64
> seems like the minimal value which will still work (as guests seem to
> 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, and
> with virtio 1 guests can reduce ring size without need for host
> configuration, so don't bother with this for now.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/hw/virtio/virtio-net.h | 1 +
> hw/net/virtio-net.c | 22 +++++++++++++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 91ed97c..0ced975 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -35,6 +35,7 @@ typedef struct virtio_net_conf
> uint32_t txtimer;
> int32_t txburst;
> char *tx;
> + uint16_t rx_queue_size;
> } virtio_net_conf;
>
> /* Maximum packet size we can receive from tap device: header + 64k */
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 01f1351..4e595e9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1412,7 +1412,8 @@ 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);
> + n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_queue_size,
> + 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);
> @@ -1716,10 +1717,28 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> VirtIONet *n = 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 = 256;
> + 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 (= %" 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 = MAX(n->nic_conf.peers.queues, 1);
> if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
> error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] = {
> 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_queue_size, 256),
> DEFINE_PROP_END_OF_LIST(),
> };
>
Reviewed-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size
2016-08-03 23:16 [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size Michael S. Tsirkin
2016-08-03 23:17 ` Michael S. Tsirkin
2016-08-04 2:03 ` Jason Wang
@ 2016-08-04 7:35 ` Cornelia Huck
2016-08-04 19:52 ` Michael S. Tsirkin
2 siblings, 1 reply; 7+ messages in thread
From: Cornelia Huck @ 2016-08-04 7:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, Jason Wang
On Thu, 4 Aug 2016 02:16:14 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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 into
> 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 do
> 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 road, 64
> seems like the minimal value which will still work (as guests seem to
> 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, and
> 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?
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 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(DeviceState *dev, Error **errp)
> VirtIONet *n = 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 = 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 (= %" 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 = MAX(n->nic_conf.peers.queues, 1);
> if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
> error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] = {
> 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_queue_size, 256),
...and defaulting to that #define (or one derived from the #define
above) here.
> DEFINE_PROP_END_OF_LIST(),
> };
>
Do we need compat handling for the new property?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size
2016-08-04 7:35 ` Cornelia Huck
@ 2016-08-04 19:52 ` Michael S. Tsirkin
2016-08-05 9:02 ` Cornelia Huck
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2016-08-04 19:52 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-devel, Jason Wang
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" <mst@redhat.com> 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 into
> > 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 do
> > 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 road, 64
> > seems like the minimal value which will still work (as guests seem to
> > 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, and
> > 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.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 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(DeviceState *dev, Error **errp)
> > VirtIONet *n = 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 = 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 (= %" 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 = MAX(n->nic_conf.peers.queues, 1);
> > if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
> > error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> > @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] = {
> > 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_queue_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.
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
>
> Do we need compat handling for the new property?
No since we did not change the default :)
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size
2016-08-04 19:52 ` Michael S. Tsirkin
@ 2016-08-05 9:02 ` Cornelia Huck
2016-08-10 7:05 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Cornelia Huck @ 2016-08-05 9:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, Jason Wang
On Thu, 4 Aug 2016 22:52:29 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 into
> > > 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 do
> > > 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 road, 64
> > > seems like the minimal value which will still work (as guests seem to
> > > 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, and
> > > 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 <mst@redhat.com>
> > > ---
> > > 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(DeviceState *dev, Error **errp)
> > > VirtIONet *n = 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 = 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 (= %" 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 = MAX(n->nic_conf.peers.queues, 1);
> > > if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
> > > error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> > > @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] = {
> > > 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_queue_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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size
2016-08-05 9:02 ` Cornelia Huck
@ 2016-08-10 7:05 ` Jason Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2016-08-10 7:05 UTC (permalink / raw)
To: Cornelia Huck, Michael S. Tsirkin; +Cc: qemu-devel
On 2016年08月05日 17:02, Cornelia Huck wrote:
> On Thu, 4 Aug 2016 22:52:29 +0300
> "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 into
>>>> 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 do
>>>> 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 road, 64
>>>> seems like the minimal value which will still work (as guests seem to
>>>> 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, and
>>>> 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 <mst@redhat.com>
>>>> ---
>>>> 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(DeviceState *dev, Error **errp)
>>>> VirtIONet *n = 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 = 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 (= %" 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 = MAX(n->nic_conf.peers.queues, 1);
>>>> if (n->max_queues * 2 + 1 > VIRTIO_QUEUE_MAX) {
>>>> error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
>>>> @@ -1880,6 +1899,7 @@ static Property virtio_net_properties[] = {
>>>> 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_queue_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
^ permalink raw reply [flat|nested] 7+ messages in thread