Re: [Qemu-devel] [PATCH] virtio-net: allow increasing rx queue size

[Michael S. Tsirkin]

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" <address@hidden> 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 <address@hidden>
> > ---
> >  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 :)