[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 01/12] Introduce /domain/devices/interface/driver/@queues attribute



On 05/13/2013 01:22 PM, Michal Privoznik wrote:
> This attribute is going to represent number of queues for
> multique vhost network interface. This commit implements XML
> extension part of the feature and add one test as well. For now,
> we can only do xml2xml test as qemu command line generation code
> is not adapted yet.
> ---
>  docs/formatdomain.html.in                          | 11 ++++-
>  docs/schemas/domaincommon.rng                      |  5 +++
>  src/conf/domain_conf.c                             | 15 +++++++
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_domain.c                             | 27 +++++++-----
>  tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml  |  2 +-
>  .../qemuxml2argv-net-virtio-device.xml             |  2 +-
>  .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++
>  tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml |  2 +-
>  tests/qemuxml2xmltest.c                            |  1 +
>  10 files changed, 103 insertions(+), 14 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9ade507..68cd3d4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3186,7 +3186,7 @@ qemu-kvm -net nic,model=? /dev/null
>        <source network='default'/>
>        <target dev='vnet1'/>
>        <model type='virtio'/>
> -      <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off'/&gt;</b>
> +      <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/&gt;</b>
>      &lt;/interface&gt;
>    &lt;/devices&gt;
>    ...</pre>
> @@ -3280,6 +3280,15 @@ qemu-kvm -net nic,model=? /dev/null
>          <b>In general you should leave this option alone, unless you
>          are very certain you know what you are doing.</b>
>        </dd>
> +      <dt><code>queues</code></dt>
> +      <dd>
> +        The optional <code>queues</code> attribute controls number of
> +        queues for <a href="http://www.linux-kvm.org/page/Multiqueue";>M
> +        ultiqueue virtio-net</a> feature. Long story short, in case of a


"Long story short" is a bit too informal for documentation. Maybe
instead you could say something like:

   If the interface has <model type='virtio'/>, multiple packet
processing queues can be created; each queue will potentially be handled
by a different processor, resulting in much higher throughput.


> +        virtio net device, multiple queues can be created so each queue is
> +        handled by different processor resulting in much higher throughput.
> +        <span class="since">Since 1.0.5 (QEMU and KVM only)</span>


s/1.0.5/1.0.6/


> +      </dd>
>      </dl>
>  
>      <h5><a name="elementsNICSTargetOverride">Overriding the target element</a></h5>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8004428..d671491 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1990,6 +1990,11 @@
>                  </attribute>
>                </optional>
>                <optional>
> +                <attribute name='queues'>
> +                  <ref name="positiveInteger"/>


Should a lower limit be put on it in the RNG? (does qemu have a
documented limit?)


> +                </attribute>
> +              </optional>
> +              <optional>
>                  <attribute name="txmode">
>                    <choice>
>                      <value>iothread</value>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 862b997..429f0ed 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5935,6 +5935,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>      char *txmode = NULL;
>      char *ioeventfd = NULL;
>      char *event_idx = NULL;
> +    char *queues = NULL;
>      char *filter = NULL;
>      char *internal = NULL;
>      char *devaddr = NULL;
> @@ -6046,6 +6047,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>                  txmode = virXMLPropString(cur, "txmode");
>                  ioeventfd = virXMLPropString(cur, "ioeventfd");
>                  event_idx = virXMLPropString(cur, "event_idx");
> +                queues = virXMLPropString(cur, "queues");
>              } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) {
>                  if (filter) {
>                      virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -6336,6 +6338,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>              }
>              def->driver.virtio.event_idx = idx;
>          }
> +        if (queues) {
> +            unsigned int q;
> +            if (virStrToLong_ui(queues, NULL, 10, &q) < 0) {
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                               _("'queues' attribute must be unsigned integer: %s"),


    How about "Invalid queues value %s, must be a positive number" or
"must be 1 - [some value]"?


> +                               queues);
> +                goto error;
> +            }
> +            def->driver.virtio.queues = q;
> +        }
>      }
>  
>      def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT;
> @@ -6389,6 +6401,7 @@ cleanup:
>      VIR_FREE(txmode);
>      VIR_FREE(ioeventfd);
>      VIR_FREE(event_idx);
> +    VIR_FREE(queues);
>      VIR_FREE(filter);
>      VIR_FREE(type);
>      VIR_FREE(internal);
> @@ -14354,6 +14367,8 @@ virDomainNetDefFormat(virBufferPtr buf,
>                  virBufferAsprintf(buf, " event_idx='%s'",
>                                    virDomainVirtioEventIdxTypeToString(def->driver.virtio.event_idx));
>              }
> +            if (def->driver.virtio.queues)
> +                virBufferAsprintf(buf, " queues='%u'", def->driver.virtio.queues);
>              virBufferAddLit(buf, "/>\n");
>          }
>      }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a9d3410..0a30406 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -923,6 +923,7 @@ struct _virDomainNetDef {
>              enum virDomainNetVirtioTxModeType txmode;
>              enum virDomainIoEventFd ioeventfd;
>              enum virDomainVirtioEventIdx event_idx;
> +            unsigned int queues; /* Multiqueue virtio-net */
>          } virtio;
>      } driver;
>      union {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 97a8307..ce22afc 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -727,17 +727,24 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>      virQEMUDriverPtr driver = opaque;
>      virQEMUDriverConfigPtr cfg = NULL;
>  
> -    if (dev->type == VIR_DOMAIN_DEVICE_NET &&
> -        dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> -        !dev->data.net->model) {
> -        if (def->os.arch == VIR_ARCH_S390 ||
> -            def->os.arch == VIR_ARCH_S390X)
> -            dev->data.net->model = strdup("virtio");
> -        else
> -            dev->data.net->model = strdup("rtl8139");
> +    if (dev->type == VIR_DOMAIN_DEVICE_NET) {
> +        if (dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> +            !dev->data.net->model) {
> +            if (def->os.arch == VIR_ARCH_S390 ||
> +                def->os.arch == VIR_ARCH_S390X)
> +                dev->data.net->model = strdup("virtio");
> +            else
> +                dev->data.net->model = strdup("rtl8139");
>  
> -        if (!dev->data.net->model)
> -            goto no_memory;
> +            if (!dev->data.net->model)
> +                goto no_memory;
> +        }
> +
> +        if (STREQ(dev->data.net->model, "virtio") &&
> +            dev->data.net->driver.virtio.queues == 0) {
> +            /* default number of queues for multiqueue NIC */
> +            dev->data.net->driver.virtio.queues = 1;
> +        }

Rather than relying on some code in an out-of-the-way post-parse
callback to set this to 1, I think it would be better to just interpret
0 as 1 where the value is used. Aside from removing the mystery for
someone casually reading the code who doesn't know about the post-parse
function, it would also allow us to use "0" as a sentinel value which
means "don't emit" during formatting. This is one of those attributes
which I think *shouldn't* have its default value auto-filled (as long as
migrating from one machine to another with the number of queues changing
from source to dest doesn't create a problem, that is).


>      }
>  
>      /* set default disk types and drivers */
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml
> index b3b7e89..44ce184 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml
> @@ -38,7 +38,7 @@
>      <interface type='user'>
>        <mac address='52:54:00:e5:48:58'/>
>        <model type='virtio'/>
> -      <driver name='vhost' event_idx='off'/>
> +      <driver name='vhost' event_idx='off' queues='1'/>

... and you would then avoid all of these gratuitous changes to xml test
data.


>      </interface>
>      <serial type='pty'>
>        <target port='0'/>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml
> index 1782831..04f3471 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml
> @@ -25,7 +25,7 @@
>      <interface type='user'>
>        <mac address='00:11:22:33:44:55'/>
>        <model type='virtio'/>
> -      <driver txmode='iothread'/>
> +      <driver txmode='iothread' queues='1'/>
>      </interface>
>      <memballoon model='virtio'/>
>    </devices>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
> new file mode 100644
> index 0000000..76f84f6
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
> @@ -0,0 +1,51 @@
> +<domain type='qemu'>
> +  <name>test</name>
> +  <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid>
> +  <memory unit='KiB'>1048576</memory>
> +  <currentMemory unit='KiB'>1048576</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc-0.13'>hvm</type>
> +    <boot dev='cdrom'/>
> +    <boot dev='hd'/>
> +    <bootmenu enable='yes'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>restart</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <disk type='file' device='disk'>
> +      <driver name='qemu' type='qcow2' event_idx='on'/>
> +      <source file='/var/lib/libvirt/images/f14.img'/>
> +      <target dev='vda' bus='virtio'/>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> +    </disk>
> +    <disk type='file' device='cdrom'>
> +      <driver name='qemu' type='raw'/>
> +      <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/>
> +      <target dev='hdc' bus='ide'/>
> +      <readonly/>
> +      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'/>
> +    <controller type='virtio-serial' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
> +    </controller>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <interface type='user'>
> +      <mac address='52:54:00:e5:48:58'/>
> +      <model type='virtio'/>
> +      <driver name='vhost' queues='5'/>
> +    </interface>
> +    <serial type='pty'>
> +      <target port='0'/>
> +    </serial>
> +    <console type='pty'>
> +      <target type='serial' port='0'/>
> +    </console>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
> index 077ca92..2717439 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
> @@ -37,7 +37,7 @@
>      <interface type='user'>
>        <mac address='52:54:00:e5:48:58'/>
>        <model type='virtio'/>
> -      <driver name='vhost' event_idx='off'/>
> +      <driver name='vhost' event_idx='off' queues='1'/>
>      </interface>
>      <serial type='pty'>
>        <target port='0'/>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 492ac60..c06c189 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -244,6 +244,7 @@ mymain(void)
>      DO_TEST("smp");
>      DO_TEST("lease");
>      DO_TEST("event_idx");
> +    DO_TEST("vhost_queues");
>      DO_TEST("virtio-lun");
>  
>      DO_TEST("usb-redir");


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]