[virt-tools-list] [vhostmd 3/5] Activate virtio support in vhostmd
Jim Fehlig
jfehlig at suse.com
Wed Sep 26 22:34:28 UTC 2018
On 8/30/18 4:11 AM, Michael Trapp wrote:
> Activate the virtio code and extend DTD/XML to support a virtio section
> for virtio related configuration and a new transport type virtio.
> ---
> README | 56 +++++++++++++++++++++++++++++++++++++++++++++------
> vhostmd.changes | 5 +++++
> vhostmd.dtd | 6 +++++-
> vhostmd.xml | 6 ++++++
> vhostmd/Makefile.am | 2 +-
> vhostmd/vhostmd.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 6 files changed, 124 insertions(+), 9 deletions(-)
>
> diff --git a/README b/README
> index cb23226..44dbbd1 100644
> --- a/README
> +++ b/README
> @@ -50,8 +50,15 @@ includes a few examples of user-defined metrics, which provide a
> <path>/dev/shm/vhostmd0</path>
> <size unit="k">256</size>
> </disk>
> + <virtio>
> + <max_channels>1024</max_channels>
> + <expiration_time>15</expiration_time>
> + <path>/var/lib/libvirt/qemu/channels</path>
> + </virtio>
> <update_period>5</update_period>
> <path>/usr/bin:/usr/sbin:/usr/share/vhostmd/scripts</path>
> + <transport>vbd</transport>
> + <transport>virtio</transport>
Hmm, in hindsight I suppose we could have structured the XML in such a way that
information about the transport is embedded within <transport>. E.g. something like
<transport type='vbd>
<disk>
<name>host-metrics-disk</name>
<path>/dev/shm/vhostmd0</path>
<size unit="k">256</size>
</disk>
</transport>
<transport type='virtio-serial'>
...
</transport>
But we weren't really expecting other transports and didn't consider the config
syntax carefully when the second one was added. We can keep things simple and go
with the virtio config you have here as long as it is documented.
> </globals>
> <metrics>
> <metric type="string" context="host">
> @@ -116,10 +123,13 @@ includes a few examples of user-defined metrics, which provide a
>
> A valid configuration file must contain the root element <vhostmd>.
> The <globals> element contains configuration global to vhostmd, such as
> -the metrics disk path and the metrics refresh interval. The <metrics>
> -element is a container for all of the <metric> elements. A metric element
> -is used to define a metric, giving it a name and an action that produces
> -the metric value.
> +the metrics disk path and the metrics refresh interval. In addition
> +metrics can also be read over QEMU virtio channels. Active transports
> +can be specified by <transport> element.
And here is a good place to improve the documentation by doing a better job at
tying the <transport> to the configuration for that transport. How about
rewording this to something like
The <globals> element contains configuration global to vhostmd, such as the
metrics refresh interval and the metrics transport mechanism. The <transport>
element defines how the metrics are transported between the host and VMs. The
vbd transport uses a virtual disk, described in the <disk> element, to share
metrics data between host and VM. The virtio transport, described by the
<virtio> element, uses a virtio-serial connection to share the metrics data.
?
> +
> +The <metrics> element is a container for all of the <metric> elements.
> +A metric element is used to define a metric, giving it a name and an action
> +that produces the metric value.
>
> The supplied vhostmd configuration file provides a useful set of default
> metrics to be collected. This can be extended or modified by editing
> @@ -277,14 +287,48 @@ section:
> (Note: Change target dev and bus as appropriate for the domain)
>
>
> +Virtio access to Metrics
Since in the end result is the same whether using vbd, xenstore, or virtio as
the transport, this section should be titled "Notes on Virtio Transport" or similar.
> +------------------------
> +
> +Basically for a virtio serial device, QEMU creates
And maybe a better introductory sentence. E.g.
The virtio transport uses a virtio serial device to transport metrics data
between the host and VMs. Basically for a virtio serial device, QEMU creates...
Also I think this section is a good place to explain the various <virtio>
transport settings.
> +- a unix domain socket on the Host
> +- a serial port on the VM
> +- 'connects' both to a 'communication channel'
> +
> +
> +Sample VM config with virtio serial:
> +
> + <channel type='unix'>
> + <source mode='bind' path='/var/lib/libvirt/qemu/channels/\
> +org.github.vhostmd.1.a70605c8-7d69-8c44-7e1a-5ecd092cb1e1'/>
> + <target type='virtio' name='vhostmd'/>
> + <address type='virtio-serial' controller='0' bus='0' port='1'/>
> + </channel>
> +
> +
> +Vhostmd accepts metric requests 'GET /metrics/XML\n\n' and responds with
> +
> + <metrics>
> + <metric type='real64' context='host'>
> + <name>TotalCPUTime</name>
> + <value>179645.910000</value>
> + </metric>
> + ...
> + <metric type='uint64' context='vm' id='9' uuid='a70605c8-7d69-8c44-7e1a-5ecd092cb1e1'>
> + <name>UsedMemory</name>
> + <value>524288</value>
> + </metric>
> + </metrics>
> +
> +
> Guest Tool/Library for Accessing Metrics Data
> ---------------------------------------------
>
> Tool: vm_dump_metrics
> Stand alone static utility will read all the metrics and write them
> to stdout or optionally an argumented file.
> - Usaga:
> - vhostmd [-f dest_file]
> + Usage:
> + vhostmd -d|-i|-x [-f dest_file]
Heh, you already fixed one typo (s/Usaga/Usage/), but isn't 'vhostmd' a typo as
well? Should it be 'vm_dump_metrics'?
>
> Library: libmetrics.so.0
> Dynamic library that supports individual metrics gathering
> diff --git a/vhostmd.changes b/vhostmd.changes
> index c7b453d..d32ba5a 100644
> --- a/vhostmd.changes
> +++ b/vhostmd.changes
> @@ -1,3 +1,8 @@
> +-------------------------------------------------------------------
> +Wed Jul 25 10:00:20 CEST 2018 - michael.trapp at sap.com
> +
> +- add virtio support
add virtio as a transport mechanism
> +
> -------------------------------------------------------------------
> Mon Jun 29 16:42:39 MDT 2009 - jfehlig at novell.com
>
> diff --git a/vhostmd.dtd b/vhostmd.dtd
> index 471c72f..6480532 100644
> --- a/vhostmd.dtd
> +++ b/vhostmd.dtd
> @@ -9,7 +9,7 @@ Virtual Host Metrics Daemon (vhostmd). Configuration file DTD
> -->
>
> <!ELEMENT vhostmd (globals,metrics)>
> -<!ELEMENT globals (disk,update_period,path,transport+)>
> +<!ELEMENT globals (disk,virtio,update_period,path,transport+)>
>
> <!ELEMENT disk (name,path,size)>
> <!ELEMENT name (#PCDATA)>
> @@ -20,6 +20,10 @@ Virtual Host Metrics Daemon (vhostmd). Configuration file DTD
> <!ELEMENT update_period (#PCDATA)>
> <!ELEMENT transport (#PCDATA)>
>
> +<!ELEMENT virtio (max_channels,expiration_time,path)>
> +<!ELEMENT max_channels (#PCDATA)>
> +<!ELEMENT expiration_time (#PCDATA)>
> +
> <!ELEMENT metrics (metric*)>
> <!ELEMENT metric (name,action,variable*)>
> <!ELEMENT action (#PCDATA)>
> diff --git a/vhostmd.xml b/vhostmd.xml
> index 9b048df..5400981 100644
> --- a/vhostmd.xml
> +++ b/vhostmd.xml
> @@ -33,9 +33,15 @@ the logical && operator must be replaced with "&&".
> <path>/dev/shm/vhostmd0</path>
> <size unit="k">256</size>
> </disk>
> + <virtio>
> + <max_channels>1024</max_channels>
> + <expiration_time>15</expiration_time>
> + <path>/var/lib/libvirt/qemu/channels</path>
> + </virtio>
> <path>/usr/sbin:/sbin:/usr/bin:/bin:/usr/share/vhostmd/scripts</path>
> <transport>vbd</transport>
> + <transport>virtio</transport>
> <!-- <transport>xenstore</transport> -->
> </globals>
> <metrics>
> diff --git a/vhostmd/Makefile.am b/vhostmd/Makefile.am
> index 3585970..860f019 100644
> --- a/vhostmd/Makefile.am
> +++ b/vhostmd/Makefile.am
> @@ -3,7 +3,7 @@ INCLUDES = \
> -I../include
>
> sbin_PROGRAMS = vhostmd
> -vhostmd_SOURCES = vhostmd.c util.c metric.c virt-util.c
> +vhostmd_SOURCES = vhostmd.c util.c metric.c virt-util.c virtio.c
> vhostmd_CFLAGS = $(LIBXML_CFLAGS) $(LIBVIRT_CFLAGS)
> vhostmd_LDADD = -lm $(LIBXML_LIBS) $(LIBVIRT_LIBS)
>
> diff --git a/vhostmd/vhostmd.c b/vhostmd/vhostmd.c
> index dc80345..e6a1283 100644
> --- a/vhostmd/vhostmd.c
> +++ b/vhostmd/vhostmd.c
> @@ -41,10 +41,11 @@
> #include <sys/stat.h>
> #include <libxml/parser.h>
> #include <libxml/xpath.h>
> +#include <pthread.h>
>
> #include "util.h"
> #include "metric.h"
> -
> +#include "virtio.h"
>
> /*
> * vhostmd will periodically write metrics to a disk. The metrics
> @@ -84,6 +85,7 @@ typedef struct _mdisk_header
> */
> #define VBD (1 << 0)
> #define XENSTORE (1 << 1)
> +#define VIRTIO (1 << 2)
>
> /* Global variables */
> static int down = 0;
> @@ -102,6 +104,9 @@ static mdisk_header md_header =
> };
> static char *search_path = NULL;
> static int transports = 0;
> +static char *virtio_path = "/var/lib/libvirt/qemu/channels";
> +static unsigned virtio_max_channels = 1024;
> +static unsigned virtio_expiration_time = 15;
>
>
> /**********************************************************************
> @@ -469,6 +474,8 @@ static int parse_transports(xmlDocPtr xml,
> return -1;
> #endif
> }
> + if (strncasecmp((char *)str, "virtio", strlen("virtio")) == 0)
> + transports |= VIRTIO;
> free(str);
> }
> }
> @@ -604,6 +611,19 @@ static int parse_config_file(const char *filename)
> goto out;
> }
>
> + if (transports & VIRTIO) {
> + char *p = NULL;
> +
> + if (vu_xpath_long("string(./globals/virtio/max_channels[1])", ctxt, &l) == 0)
> + virtio_max_channels = (unsigned)l;
> +
> + if (vu_xpath_long("string(./globals/virtio/expiration_time[1])", ctxt, &l) == 0)
> + virtio_expiration_time = (unsigned)l;
> +
> + if ((p = vu_xpath_string("string(./globals/virtio/path[1])", ctxt)) != NULL)
> + virtio_path = p;
> + }
> +
> /* Parse requested metrics definitions */
> if (parse_metrics(xml, ctxt)) {
> vu_log(VHOSTMD_ERR, "Unable to parse metrics definition "
> @@ -838,6 +858,8 @@ static int metrics_host_get(vu_buffer *buf)
> {
> metric *m = metrics;
>
> + unsigned start = buf->use;
> +
> while (m) {
> if (m->ctx != METRIC_CONTEXT_HOST) {
> m = m->next;
> @@ -849,6 +871,10 @@ static int metrics_host_get(vu_buffer *buf)
>
> m = m->next;
> }
> +
> + if (transports & VIRTIO)
> + virtio_metrics_update(&buf->content[start], buf->use - start, NULL, METRIC_CONTEXT_HOST);
> +
> return 0;
> }
>
> @@ -856,6 +882,8 @@ static int metrics_vm_get(vu_vm *vm, vu_buffer *buf)
> {
> metric *m = metrics;
>
> + unsigned start = buf->use;
> +
> while (m) {
> if (m->ctx != METRIC_CONTEXT_VM) {
> m = m->next;
> @@ -867,6 +895,10 @@ static int metrics_vm_get(vu_vm *vm, vu_buffer *buf)
>
> m = m->next;
> }
> +
> + if (transports & VIRTIO)
> + virtio_metrics_update(&buf->content[start], buf->use - start, vm->uuid, METRIC_CONTEXT_VM);
> +
> return 0;
> }
>
> @@ -911,11 +943,29 @@ static int vhostmd_run(int diskfd)
> int *ids = NULL;
> int num_vms = 0;
> vu_buffer *buf = NULL;
> + pthread_t virtio_tid;
>
> if (vu_buffer_create(&buf, MDISK_SIZE_MIN - MDISK_HEADER_SIZE)) {
> vu_log(VHOSTMD_ERR, "Unable to allocate memory");
> return -1;
> }
> +
> + if (transports & VIRTIO) {
> + int rc;
> +
> + if (virtio_expiration_time < (update_period * 3))
vhostmd.c:956:34: error: comparison of integer expressions of different
signedness: ‘unsigned int’ and ‘int’ [-Werror=sign-compare]
if (virtio_expiration_time < (update_period * 3))
Regards,
Jim
> + virtio_expiration_time = (update_period * 3);
> +
> + if (virtio_init(virtio_path, virtio_max_channels, virtio_expiration_time))
> + return -1;
> +
> + rc = pthread_create(&virtio_tid, NULL, virtio_run, NULL);
> +
> + if (rc != 0) {
> + vu_log(VHOSTMD_ERR, "Failed to start virtio thread '%s'\n", strerror(rc));
> + return -1;
> + }
> + }
>
> while (!down) {
> vu_buffer_add(buf, "<metrics>\n", -1);
> @@ -940,6 +990,12 @@ static int vhostmd_run(int diskfd)
> vu_buffer_erase(buf);
> }
> vu_buffer_delete(buf);
> +
> + if (transports & VIRTIO) {
> + virtio_stop();
> + pthread_join(virtio_tid, NULL);
> + }
> +
> return 0;
> }
>
>
More information about the virt-tools-list
mailing list