Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Patrik Hermansson]

Hi Michael

Thanks for looking into the patch and for your comments.

2016-03-15 5:50 GMT+01:00 Michael S. Tsirkin :
> On Fri, Nov 27, 2015 at 04:02:39PM +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.
>
> Any more data here? 256 packets seems plenty. Is there a chance
> your application does not support indirect descriptors?
> That's a better approach I think.

My application within the VM is using DPDK(2.0) virtio PMD and even if
it seems to have been attempts
getting support for indirect descriptors it does not seems it have
reached mainline.
To be honest I hadn't heard about indirect descriptors in virtio until
you mentioned it so
I still have some homework to do, but it seems  to be a much more
attractive solution
if such support exists in my case.

With 256 descriptors I see some problems with sporadic packet drops
from time to time
which I believe have to do with qemu getting rescheduled, increasing
the queue to 1024
seems to remedy the problem. My beliefs is that it is due the process
getting some more
headroom until packets are getting dropped.

>
>> 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.
>
> To me, this seems excessive.  For example, 1024 will break if we have to
> add a virtio net header entry (on old kernels).
>

1024 were chosen due to assertion that the queue were not allowed to greater
than VIRTQUEUE_SIZE_MAX. I can lower the upper limit and do some more
measurements, but maybe it is the wrong path to go regardless?

Thanks,
Patrik

Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Michael S. Tsirkin]

On Tue, Mar 15, 2016 at 09:00:39AM +0100, Greg Kurz wrote:
> On Tue, 15 Mar 2016 06:50:38 +0200
> "Michael S. Tsirkin"  wrote:
> 
> > On Fri, Nov 27, 2015 at 04:02:39PM +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.  
> > 
> > Any more data here? 256 packets seems plenty. Is there a chance
> > your application does not support indirect descriptors?
> > That's a better approach I think.
> > 
> > > 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.  
> > 
> > To me, this seems excessive.  For example, 1024 will break if we have to
> > add a virtio net header entry (on old kernels).
> > 
> 
> Hi Michael,
> 
> Can you elaborate on what/how will break ?

At least on linux, we add a header and then pass the scatter
list to writev as is.  Linux doesn't support io vectors > 1024 so if we
allow 1024 entries for the guest, adding even a single entry will
make the write fail.


> Thanks.
> 
> --
> Greg
> 
> > > ---
> > >  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,
> > > +  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,
> > > +  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(>net_conf.rx_virtqueue_sz);
> > > +virtio_net_validate_queue_limit(>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,
> > >>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, 
> > > >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
> > >  

Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Patrik Hermansson]

Hi Greg,
Thanks for your comments and for taking the time.

2016-03-14 12:27 GMT+01:00 Greg Kurz :
>
> SoB tag is missing.

I will add this in the future.

> > +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.

Makes sense, I will fix this.

> > +  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.

See previous comment.

Thanks,
Patrik

Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Greg Kurz]

On Tue, 15 Mar 2016 06:50:38 +0200
"Michael S. Tsirkin"  wrote:

> On Fri, Nov 27, 2015 at 04:02:39PM +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.  
> 
> Any more data here? 256 packets seems plenty. Is there a chance
> your application does not support indirect descriptors?
> That's a better approach I think.
> 
> > 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.  
> 
> To me, this seems excessive.  For example, 1024 will break if we have to
> add a virtio net header entry (on old kernels).
> 

Hi Michael,

Can you elaborate on what/how will break ?

Thanks.

--
Greg

> > ---
> >  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,
> > +  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,
> > +  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(>net_conf.rx_virtqueue_sz);
> > +virtio_net_validate_queue_limit(>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,
> >>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, 
> > >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
> > +#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
> >  
> >  typedef struct virtio_net_conf
> >  {
> >  uint32_t txtimer;
> >  int32_t txburst;
> >  char *tx;
> > +uint16_t rx_virtqueue_sz;
> > +

Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Greg Kurz]

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(>net_conf.rx_virtqueue_sz);
> > +virtio_net_validate_queue_limit(>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,
> >>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, 
> > >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
> >  {
> >  

Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Michael S. Tsirkin]

On Fri, Nov 27, 2015 at 04:02:39PM +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.

Any more data here? 256 packets seems plenty. Is there a chance
your application does not support indirect descriptors?
That's a better approach I think.

> 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.

To me, this seems excessive.  For example, 1024 will break if we have to
add a virtio net header entry (on old kernels).

> ---
>  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,
> +  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,
> +  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(>net_conf.rx_virtqueue_sz);
> +virtio_net_validate_queue_limit(>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,
>>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, >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
> +#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
>  
>  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 */
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Greg Kurz]

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(>net_conf.rx_virtqueue_sz);
> +virtio_net_validate_queue_limit(>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,
>>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, >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)

> +#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
>  
>  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 */

[Qemu-devel] [PATCH] virtio-net: Make virtio queue sizes configurable

[Patrik Hermansson]

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.
---
 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,
+  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,
+  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(>net_conf.rx_virtqueue_sz);
+virtio_net_validate_queue_limit(>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,
   >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, >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
+#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
 
 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 */
-- 
1.9.1