Add "xl info" command

Hi,

as already mentioned in previous emails, the "info" subcommand is missing from the xl tool.
The attached patchset adds support for this, extending libxl on the way to provide the necessary info only by using own functions. On my system the output of xm info and xl info was identical, I omitted the recent NUMA additions from xl info for now and will provide the necessary patches later.

Most of the additions are straightforward, just some words on libxl_get_version_info():
The xen_version hypercall uses statically allocated memory for passing strings (e.g. 1024 Bytes for commandline). To gather all information, one has to hypercall multiple times with different arg values, each information also has a different (static) result type. To make this more user friendly in libxl, I created a structure holding all information in dynamically allocated pointers. These will be malloced and filled on calling libxl_get_version_info() and have to be freed by the caller (with libxl_free_version_info()). To avoid doing several hypercalls and copying unnecessary memory, I used a bitmask to select the pieces of information the caller is interested. This is especially useful if one wants to know the Xen version or the PAGESIZE only. For xl info there is a LIBXL_VERSION_ALL_MASK to get all of them.

Please apply!

Regards,
Andre.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

The libxl version of the physinfo sysctl does not contain some fields like nr_nodes or capabilities needed for xl info output. Add them to the structure and the retrieving function.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

diff -r 7ee8bb40200a tools/libxl/libxl.c
@@ -2344,6 +2344,10 @@
     physinfo->total_pages = xcphysinfo.total_pages;
     physinfo->free_pages = xcphysinfo.free_pages;
     physinfo->scrub_pages = xcphysinfo.scrub_pages;
+    physinfo->nr_nodes = xcphysinfo.nr_nodes;
+    memcpy(physinfo->hw_cap,xcphysinfo.hw_cap, sizeof(physinfo->hw_cap));
+    physinfo->phys_cap = xcphysinfo.capabilities;
+
     return 0;
 }
 

diff -r 7ee8bb40200a tools/libxl/libxl.h
@@ -415,6 +415,9 @@
     uint64_t *cpumap; /* current cpu's affinities */
 };
 
+#define PHYS_CAP_HVM          1
+#define PHYS_CAP_HVM_DIRECTIO 2
+
 struct libxl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;

@@ -426,6 +429,10 @@
     uint64_t total_pages;
     uint64_t free_pages;
     uint64_t scrub_pages;
+
+    uint32_t nr_nodes;
+    uint32_t hw_cap[8];
+    uint32_t phys_cap;
 };
 
 int libxl_get_physinfo(struct libxl_ctx *ctx, struct libxl_physinfo *physinfo);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

I think, It would be much nicer to provide the phys_caps already decoded instead of copying the xc value as is.

so what about something like having phys_cap_hvm and phys_cap_hvm_directio field directly in the structure ? If things extends, we should add new field in libxl anyway.

Vincent

Andre Przywara writes ("[Xen-devel] [PATCH 1/4] libxl: extend physinfo structure"):

Rather than this:

--- a/tools/libxl/libxl.h Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.h Sun Apr 18 14:34:32 2010 +0200
+#define PHYS_CAP_HVM 1
+#define PHYS_CAP_HVM_DIRECTIO 2

I think it would be better to expect libxl callers to include the public Xen header file sysctl.h. While libxl callers should not need to know about information about the underlying libraris, the Xen interface is a key public interface and is fine to expose - at least as far as these kind of bitmasks.

That also avoids the possibility of getting these #defines out of step with the underlying code (or even the possibility of having to write conversion code!)

Certainly if you _don't_ convert, you shouldn't redefine the bitfield constants.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 1/4] libxl: extend physinfo structure"):

Is it really worth reprocessing a bitfield like this which is after all not actually going to be used much by calling code ?

That's a possibility which I wouldn't object to but it seems unnecessary. In that case it should be a series of 1-bit unsigned C bitfields rather than a set of flags in a uint32.

Ian.

To get the name of the currently used scheduler, Xen provides a sched_id sysctl. Add a libxl wrapper around the libxc function to query this.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

diff -r 7ee8bb40200a tools/libxl/libxl.c
@@ -2421,3 +2421,13 @@
     }
     return 0;
 }
+
+int libxl_get_sched_id(struct libxl_ctx *ctx)
+{
+    int sched, ret;
+
+    if ((ret = xc_sched_id(ctx->xch, &sched)) != 0)
+        return -ret;
+    return sched;
+}
+

diff -r 7ee8bb40200a tools/libxl/libxl.h
@@ -441,5 +441,7 @@
 int libxl_set_vcpuaffinity(struct libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int cpusize);
 int libxl_set_vcpucount(struct libxl_ctx *ctx, uint32_t domid, uint32_t count);
+
+int libxl_get_sched_id(struct libxl_ctx *ctx);
 #endif /* LIBXL_H */
 

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

maybe it should returns an enum libxl_sched_id instead of just a plain int ?

Vincent

Andre Przywara writes ("[Xen-devel] [PATCH 2/4] libxl: add sched_get_id function"):

There should be a doc comemnt in libxl.h saying what the return value is. In this case it seems to be one of the values XEN_SCHEDULER_* from xen/include/public/domctl.h.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 2/4] libxl: add sched_get_id function"):

That would be an alternative but it does involve extra source code which TBH I think we can do without.

Ian.

Xen provides a xen_version hypercall to query the values of several interesting things (like hypervisor version, commandline used, actual changeset, etc.). Create a user-friendly and efficient wrapper around the libxc function to provide values for xl info output.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

diff -r 7ee8bb40200a tools/libxl/libxl.c
@@ -2351,6 +2351,92 @@
     return 0;
 }
 
+int libxl_get_version_info(struct libxl_ctx *ctx,
+                           libxl_version_info *info,
+                           uint32_t query_mask)
+{
+    union {
+        xen_extraversion_t xen_extra;
+        xen_compile_info_t xen_cc;
+        xen_changeset_info_t xen_chgset;
+        xen_capabilities_info_t xen_caps;
+        xen_platform_parameters_t p_parms;
+        xen_commandline_t xen_commandline;
+    } u;
+    long xen_version;
+
+    if (query_mask & LIBXL_VERSION_VERSION_MASK) {
+        xen_version = xc_version(ctx->xch, XENVER_version, NULL);
+        info->xen_version_major = xen_version >> 16;
+        info->xen_version_minor = xen_version & 0xFF;
+        xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
+        info->xen_version_extra = strdup(u.xen_extra);
+    } else {
+        info->xen_version_major = 0;
+        info->xen_version_minor = 0;
+        info->xen_version_extra = NULL;
+    }
+
+    if (query_mask & LIBXL_VERSION_COMPILER_MASK) {
+        xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
+        info->compiler = strdup(u.xen_cc.compiler);
+        info->compile_by = strdup(u.xen_cc.compile_by);
+        info->compile_domain = strdup(u.xen_cc.compile_domain);
+        info->compile_date = strdup(u.xen_cc.compile_date);
+    } else {
+        info->compiler = NULL;
+        info->compile_by = NULL;
+        info->compile_domain = NULL;
+        info->compile_date = NULL;
+    }
+
+    if (query_mask & LIBXL_VERSION_CAPS_MASK) {
+        xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
+        info->capabilities = strdup(u.xen_caps);
+    } else
+        info->capabilities = NULL;
+
+    if (query_mask & LIBXL_VERSION_CHANGESET_MASK) {
+        xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
+        info->changeset = strdup(u.xen_chgset);
+    } else 
+        info->changeset = NULL;
+
+    if (query_mask & LIBXL_VERSION_VIRT_START_MASK) {
+        xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
+        info->virt_start = u.p_parms.virt_start;
+    } else
+        info->virt_start = 0;
+
+    if (query_mask & LIBXL_VERSION_PAGESIZE_MASK)
+        info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
+    else
+        info->pagesize = 0;
+
+    if (query_mask & LIBXL_VERSION_COMMANDLINE_MASK) {
+        xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
+        info->commandline = strdup(u.xen_commandline);
+    } else 
+        info->commandline = NULL;
+
+    return 0;
+}
+
+#define FREE_IF_NOT_ZERO(ptr) if((ptr) != NULL) {free(ptr); ptr = NULL;}
+
+void libxl_free_version_info(libxl_version_info *info)
+{
+    FREE_IF_NOT_ZERO(info->xen_version_extra)
+    FREE_IF_NOT_ZERO(info->compiler)
+    FREE_IF_NOT_ZERO(info->compile_by)
+    FREE_IF_NOT_ZERO(info->compile_domain)
+    FREE_IF_NOT_ZERO(info->compile_date)
+    FREE_IF_NOT_ZERO(info->capabilities)
+    FREE_IF_NOT_ZERO(info->changeset)
+    FREE_IF_NOT_ZERO(info->commandline)
+}
+#undef FREE_IF_NOT_ZERO
+
 struct libxl_vcpuinfo *libxl_list_vcpu(struct libxl_ctx *ctx, uint32_t domid,
                                        int *nb_vcpu, int *cpusize)
 {

diff -r 7ee8bb40200a tools/libxl/libxl.h
@@ -59,6 +59,35 @@
 };
 
 typedef struct {
+    int xen_version_major;
+    int xen_version_minor;
+    char *xen_version_extra;
+    char *compiler;
+    char *compile_by;
+    char *compile_domain;
+    char *compile_date;
+    char *capabilities;
+    char *changeset;
+    unsigned long virt_start;
+    unsigned long pagesize;
+    char *commandline;
+} libxl_version_info;
+
+#define LIBXL_VERSION_VERSION_MASK       (1 <<  0)
+#define LIBXL_VERSION_COMPILER_MASK      (1 <<  1)
+#define LIBXL_VERSION_CAPS_MASK          (1 <<  2)
+#define LIBXL_VERSION_CHANGESET_MASK     (1 <<  3)
+#define LIBXL_VERSION_VIRT_START_MASK    (1 <<  4)
+#define LIBXL_VERSION_PAGESIZE_MASK      (1 <<  5)
+#define LIBXL_VERSION_COMMANDLINE_MASK   (1 <<  6)
+#define LIBXL_VERSION_ALL_MASK          ((1 <<  7) - 1)
+
+int libxl_get_version_info(struct libxl_ctx *ctx,
+                           libxl_version_info *info,
+                           uint32_t query_mask);
+void libxl_free_version_info(libxl_version_info *info);
+
+typedef struct {
     bool hvm;
     bool hap;
     int ssidref;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

instead of this chain of if else, why don't you just init the structure to NULL at the beginning of the call, and not do any else ?

also the query capability seems a bit too much; the info call is probably unlikely called into a tight loop, and I expect the number of call to this to be 1 per starting toolstack or 1 per vm (at worst). from an API point of view this would make it simpler too.

last thing, is instead of using strdup, you should use libxl_sprintf(ctx, "%s", string) so that you don't have to worry about freeing memory at all, and you can just omit the free_version_info call completely.

Vincent

Andre Przywara writes ("[Xen-devel] [PATCH 3/4] libxl: add version_info function"):

As Vincent says, I think the query mask is overkill. I would suggeset just doing without it, and always acquire and return all the information. Ie, do away with query_mask and LIBXL_VERSION_FOO_MASK and the correspondings ifs.

+#define FREE_IF_NOT_ZERO(ptr) if((ptr) != NULL) {free(ptr); ptr = NULL;}

This is redundant because free(NULL) is a legal no-op. It is also bad macro practice to include an unterminated if like that - it might eat subsequent elses. Furthermore, you should wrap ptr up in parens to avoid macro precedence problems.

So you should write something like one of these:

+#define FREE_AND_ZERO(ptr) (free((ptr), (ptr) = 0) +#define FREE_AND_ZERO(ptr) do{ free((ptr)); (ptr) = 0; }while(0)

Finally, this macro should be in libxl_internal.h where every part of libxl can use it.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add version_info function"):

Yes.

Furthermore the documentation comment should make it clear that the version info structure can be undefined beforehand (and the reader can therefore conclude that libxl_get_version_info won't free it).

I don't think that's correct because memory allocated by libxl_sprintf does not survive return from libxl into the caller.

Ian.

It survives as long as the context is not ctx_free. i.e. if the data need to preserve longer, they will have do be duplicated before freeing the context.

This is to get away with having to deal with memory leak as much as possible and potential more complex code related to errors handling.

Vincent

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add version_info function [and 1 more messages]"):

This is sadly not a really tenable behaviour in the long term because
(a) a long-running caller does not want to free the context (and necessarily reestablish xenstore connections etc. etc.) (b) the caller may anyway not be able to free the context if doing so frees data they are still using.

There are not (if I'm not mistaken) currently any uses of the libxl allocators for data which persists beyond the current libxl entrypoint and we shouldn't introduce any now. If there are any they should be removed.

Ian.

This is not perfect, and has a (tiny) cost, but it makes writing the current ocaml bindings (and the future python bindings) much easier and much less memory leak prone.

Limiting the number of possible boxes of error type, will definitely help getting XCP ported on libxl.

Vincent

I am not fully convinced of this. There is quite a lot of information copied (up to 4KB), and some fields (like pagesize or the version numbers) are just plain integers. And this approach just propagates the underlying design. In the xl info implementation I used this feature to just get the pagesize. One could think of just querying the version number, too. If you don't like the additional parameter, what about a wrapping macro, then? I don't see the problem with the complexity, though, as it is hidden inside the library.
I can use libxl_sprintf function to get rid of the free() function, but I personally don't like the idea of accumulated mallocs much, left alone the "hidden" free operation.

I will incorporate your other comments.

Thanks for the review!

Regards,
Andre.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

The query mask is exposed to the user, and the structure is partially filled. I can imagine that it's going to create those kind of issues, where the wrong mask is passed, or change later, and then the field the user was expecting would be left null.

The macro would help in the previous case, but I still feel it's unnecessary. As a user, I would just not bother, and call the function always with full mask, until I reach the optimisation phase, and its appears on the graph.

A couple of K of data copied shouldn't make a difference, and would be completely lost in noise, however if you really think it makes a difference, you could have special dedicated info call to get specific things like version number.

There is a real advantage of permitting a fast development of libxl (and its bindings).

This was to get started at the beggining; when we reach a point where everything is working we could improve or remove the memory system. But I'm fairly convinced it's not going to make a difference for this kind of library.

Vincent

I just wanted to avoid introducing a lot of functions which could be actually one. And if a user passes a wrong mask, then well, he is just doing the wrong thing and cannot expect it to work. If Xen provides a possibility to query single parts of the information, why should we hide this capability from the upper layers?
Anyway, my plan was now to make the current mask function static and create three exported functions, one for getting all the information, one for getting the pagesize and one for getting the Xen version number only. Together with a comment one could later expose the mask function again if the need arises.
But when I was looking at the libxl_sprintf thing, I realized that all the information in the info structure is static and will never change during runtime. Am I right? If so, we can solve this whole thing by not only storing the strings in libxl_ctx, but the whole structure. The first call to the function would query all the members, subsequent calls would just return the pointer. This avoids duplication should the function be called multiple times. It would be automatically freed when destroying the xl context.

What do you think of this?

Regards,
Andre.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

well that's just for simplicity.

I think that's quite reasonable and a good idea all in all. the API stays simple, and most potential performance issues are just dealt with.

Vincent

The info subcommand was missing from the xl tool. Use the new libxl wrapper functions to create a clone of "xm info". The splitting into several smaller functions is enspired by the implementation in XendNode.py.

Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

diff -r 7ee8bb40200a tools/libxl/xl.c
@@ -30,6 +30,7 @@
 #include <sys/socket.h>
 #include <sys/select.h>
 #include <arpa/inet.h>
+#include <sys/utsname.h> /* for utsname in xl info */
 #include <xenctrl.h>
 #include <ctype.h>
 #include <inttypes.h>

@@ -2636,6 +2637,122 @@
     exit(0);
 }
 
+static void output_xeninfo(void)
+{
+    libxl_version_info info;
+    int sched_id;
+
+    if (libxl_get_version_info(&ctx, &info, LIBXL_VERSION_ALL_MASK) != 0) {
+        fprintf(stderr, "version_info sysctl failed.\n");
+        return;
+    }
+    if ((sched_id = libxl_get_sched_id(&ctx)) < 0) {
+        fprintf(stderr, "get_sched_id sysctl failed.\n");
+        return;
+    }
+
+    printf("xen_major              : %d\n", info.xen_version_major);
+    printf("xen_minor              : %d\n", info.xen_version_minor);
+    printf("xen_extra              : %s\n", info.xen_version_extra);
+    printf("xen_caps               : %s\n", info.capabilities);
+    printf("xen_scheduler          : %s\n",
+        sched_id == XEN_SCHEDULER_SEDF ? "sedf" :
+        sched_id == XEN_SCHEDULER_CREDIT ? "credit" :
+        sched_id == XEN_SCHEDULER_CREDIT2 ? "credit2" : "unknown");
+    printf("xen_pagesize           : %lu\n", info.pagesize);
+    printf("platform_params        : virt_start=0x%lx\n", info.virt_start);
+    printf("xen_changeset          : %s\n", info.changeset);
+    printf("xen_commandline        : %s\n", info.commandline);
+    printf("cc_compiler            : %s\n", info.compiler);
+    printf("cc_compile_by          : %s\n", info.compile_by);
+    printf("cc_compile_domain      : %s\n", info.compile_domain);
+    printf("cc_compile_date        : %s\n", info.compile_date);
+
+    libxl_free_version_info(&info);
+    return;
+}
+
+static void output_nodeinfo(void)
+{
+    struct utsname utsbuf;
+
+    uname(&utsbuf);
+
+    printf("host                   : %s\n", utsbuf.nodename);
+    printf("release                : %s\n", utsbuf.release);
+    printf("version                : %s\n", utsbuf.version);
+    printf("machine                : %s\n", utsbuf.machine);
+
+    return;
+}
+
+static void output_physinfo(void)
+{
+    struct libxl_physinfo info;
+    libxl_version_info vinfo;
+    unsigned int i;
+
+    if (libxl_get_physinfo(&ctx, &info) != 0) {
+        fprintf(stderr, "libxl_physinfo failed.\n");
+        return;
+    }
+
+    printf("nr_cpus                : %d\n", info.nr_cpus);
+    printf("nr_nodes               : %d\n", info.nr_nodes);
+    printf("cores_per_socket       : %d\n", info.cores_per_socket);
+    printf("threads_per_core       : %d\n", info.threads_per_core);
+    printf("cpu_mhz                : %d\n", info.cpu_khz / 1000);
+    printf("hw_caps                : ");
+    for (i = 0; i < 8; i++)
+        printf("%08x%c", info.hw_cap[i], i < 7 ? ':' : '\n');
+    printf("virt_caps              :");
+    if (info.phys_cap & PHYS_CAP_HVM)
+        printf(" hvm");
+    if (info.phys_cap & PHYS_CAP_HVM_DIRECTIO)
+        printf(" hvm_directio");
+    printf("\n");
+    libxl_get_version_info(&ctx, &vinfo, LIBXL_VERSION_PAGESIZE_MASK);
+    i = (1 << 20) / vinfo.pagesize;
+    libxl_free_version_info(&vinfo);
+    printf("total_memory           : %lu\n", info.total_pages / i);
+    printf("free_memory            : %lu\n", info.free_pages / i);
+
+    return;
+}
+
+void info(int verbose)
+{
+    output_nodeinfo();
+
+    output_physinfo();
+
+    output_xeninfo();
+
+    printf("xend_config_format     : 4\n");
+
+    return;
+}
+
+void main_info(int argc, char **argv)
+{
+    int opt, verbose;
+
+    verbose = 0;
+    while ((opt = getopt(argc, argv, "h")) != -1) {
+        switch (opt) {
+        case 'h':
+            help("vcpu-list");
+            exit(0);
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    info(verbose);
+    exit(0);
+}
+
 int main(int argc, char **argv)
 {
     if (argc < 2) {

@@ -2696,6 +2813,8 @@
         main_vcpupin(argc - 1, argv + 1);
     } else if (!strcmp(argv[1], "vcpu-set")) {
         main_vcpuset(argc - 1, argv + 1);
+    } else if (!strcmp(argv[1], "info")) {
+        main_info(argc - 1, argv + 1);
     } else if (!strcmp(argv[1], "help")) {
         if (argc > 2)
             help(argv[2]);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH 4/4] xl: add "xl info" command"):

This patch is fine in itself, if you make the edits needed to deal with the changes we have suggested in your libxl patch.

Thanks for your contribution.

Ian.