diff mbox series

[RFC,2/2] tools/misc: Add xen-stats tool

Message ID e233c4f60c6fe97b93b3adf9affeb0404c554130.1652797713.git.matias.vara@vates.fr (mailing list archive)
State New
Headers show
Series Add a new acquire resource to query vcpu statistics | expand

Commit Message

Matias Ezequiel Vara Larsen May 17, 2022, 2:33 p.m. UTC
Add a demostration tool that uses the stats_table resource to
query vcpu time for a DomU.

Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@vates.fr>
---
 tools/misc/Makefile    |  5 +++
 tools/misc/xen-stats.c | 83 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 tools/misc/xen-stats.c

Comments

Anthony PERARD May 31, 2022, 11:16 a.m. UTC | #1
Hi Matias,

On Tue, May 17, 2022 at 04:33:15PM +0200, Matias Ezequiel Vara Larsen wrote:
> Add a demostration tool that uses the stats_table resource to
> query vcpu time for a DomU.
> 
> Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@vates.fr>
> ---
> diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> index 2b683819d4..b510e3aceb 100644
> --- a/tools/misc/Makefile
> +++ b/tools/misc/Makefile
> @@ -135,4 +135,9 @@ xencov: xencov.o
>  xen-ucode: xen-ucode.o
>  	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
>  
> +xen-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
> +
> +xen-stats: xen-stats.o

The tools seems to be only about vcpu, maybe `xen-stats` is a bit too
generic. Would `xen-vcpus-stats`, or maybe something with `time` would
be better?

Also, is it a tools that could be useful enough to be installed by
default? Should we at least build it by default so it doesn't rot? (By
adding it to only $(TARGETS).)

> +	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
> +
>  -include $(DEPS_INCLUDE)
> diff --git a/tools/misc/xen-stats.c b/tools/misc/xen-stats.c
> new file mode 100644
> index 0000000000..5d4a3239cc
> --- /dev/null
> +++ b/tools/misc/xen-stats.c
> @@ -0,0 +1,83 @@
> +#include <err.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <signal.h>
> +
> +#include <xenctrl.h>

It seems overkill to use this header when the tool only use
xenforeignmemory interface. But I don't know how to replace
XC_PAGE_SHIFT, so I guess that's ok.

> +#include <xenforeignmemory.h>
> +#include <xen-tools/libs.h>

What do you use this headers for? Is it left over?

> +static sig_atomic_t interrupted;
> +static void close_handler(int signum)
> +{
> +    interrupted = 1;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    xenforeignmemory_handle *fh;
> +    xenforeignmemory_resource_handle *res;
> +    size_t size;
> +    int rc, nr_frames, domid, frec, vcpu;
> +    uint64_t * info;
> +    struct sigaction act;
> +
> +    if (argc != 4 ) {
> +        fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]);
> +        return 1;
> +    }
> +
> +    // TODO: this depends on the resource
> +    nr_frames = 1;
> +
> +    domid = atoi(argv[1]);
> +    frec = atoi(argv[3]);
> +    vcpu = atoi(argv[2]);

Can you swap the last two line? I think it is better if the order is the same
as on the command line.

> +
> +    act.sa_handler = close_handler;
> +    act.sa_flags = 0;
> +    sigemptyset(&act.sa_mask);
> +    sigaction(SIGHUP,  &act, NULL);
> +    sigaction(SIGTERM, &act, NULL);
> +    sigaction(SIGINT,  &act, NULL);
> +    sigaction(SIGALRM, &act, NULL);
> +
> +    fh = xenforeignmemory_open(NULL, 0);
> +
> +    if ( !fh )
> +        err(1, "xenforeignmemory_open");
> +
> +    rc = xenforeignmemory_resource_size(
> +        fh, domid, XENMEM_resource_stats_table,
> +        vcpu, &size);
> +
> +    if ( rc )
> +        err(1, "    Fail: Get size: %d - %s\n", errno, strerror(errno));

It seems that err() already does print strerror(), and add a "\n", so
why print it again? Also, if we have strerror(), what the point of
printing "errno"?

Also, I'm not sure the extra indentation in the error message is really
useful, but that doesn't really matter.

> +
> +    if ( (size >> XC_PAGE_SHIFT) != nr_frames )
> +        err(1, "    Fail: Get size: expected %u frames, got %zu\n",
> +                    nr_frames, size >> XC_PAGE_SHIFT);

err() prints strerror(errno), maybe errx() is better here.


Thanks,
Matias Ezequiel Vara Larsen June 3, 2022, 11:08 a.m. UTC | #2
Hello Anthony and thanks for your comments. I addressed them below:

On Tue, May 31, 2022 at 12:16:02PM +0100, Anthony PERARD wrote:
> Hi Matias,
> 
> On Tue, May 17, 2022 at 04:33:15PM +0200, Matias Ezequiel Vara Larsen wrote:
> > Add a demostration tool that uses the stats_table resource to
> > query vcpu time for a DomU.
> > 
> > Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@vates.fr>
> > ---
> > diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> > index 2b683819d4..b510e3aceb 100644
> > --- a/tools/misc/Makefile
> > +++ b/tools/misc/Makefile
> > @@ -135,4 +135,9 @@ xencov: xencov.o
> >  xen-ucode: xen-ucode.o
> >  	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
> >  
> > +xen-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
> > +
> > +xen-stats: xen-stats.o
> 
> The tools seems to be only about vcpu, maybe `xen-stats` is a bit too
> generic. Would `xen-vcpus-stats`, or maybe something with `time` would
> be better?

Do you think `xen-vcpus-stats` would be good enough?

> Also, is it a tools that could be useful enough to be installed by
> default? Should we at least build it by default so it doesn't rot? (By
> adding it to only $(TARGETS).)

I will add to build this tool by default in the next patches' version.
 
> > +	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
> > +
> >  -include $(DEPS_INCLUDE)
> > diff --git a/tools/misc/xen-stats.c b/tools/misc/xen-stats.c
> > new file mode 100644
> > index 0000000000..5d4a3239cc
> > --- /dev/null
> > +++ b/tools/misc/xen-stats.c
> > @@ -0,0 +1,83 @@
> > +#include <err.h>
> > +#include <errno.h>
> > +#include <error.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +#include <signal.h>
> > +
> > +#include <xenctrl.h>
> 
> It seems overkill to use this header when the tool only use
> xenforeignmemory interface. But I don't know how to replace
> XC_PAGE_SHIFT, so I guess that's ok.
> 
> > +#include <xenforeignmemory.h>
> > +#include <xen-tools/libs.h>
> 
> What do you use this headers for? Is it left over?

`xenforeignmemory.h` is used for `xenforeignmemory_*` functions.
`xen-tools/libs.h` is left over so I will remove it in next version.

> > +static sig_atomic_t interrupted;
> > +static void close_handler(int signum)
> > +{
> > +    interrupted = 1;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    xenforeignmemory_handle *fh;
> > +    xenforeignmemory_resource_handle *res;
> > +    size_t size;
> > +    int rc, nr_frames, domid, frec, vcpu;
> > +    uint64_t * info;
> > +    struct sigaction act;
> > +
> > +    if (argc != 4 ) {
> > +        fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]);
> > +        return 1;
> > +    }
> > +
> > +    // TODO: this depends on the resource
> > +    nr_frames = 1;
> > +
> > +    domid = atoi(argv[1]);
> > +    frec = atoi(argv[3]);
> > +    vcpu = atoi(argv[2]);
> 
> Can you swap the last two line? I think it is better if the order is the same
> as on the command line.

Yes, I can.

> > +
> > +    act.sa_handler = close_handler;
> > +    act.sa_flags = 0;
> > +    sigemptyset(&act.sa_mask);
> > +    sigaction(SIGHUP,  &act, NULL);
> > +    sigaction(SIGTERM, &act, NULL);
> > +    sigaction(SIGINT,  &act, NULL);
> > +    sigaction(SIGALRM, &act, NULL);
> > +
> > +    fh = xenforeignmemory_open(NULL, 0);
> > +
> > +    if ( !fh )
> > +        err(1, "xenforeignmemory_open");
> > +
> > +    rc = xenforeignmemory_resource_size(
> > +        fh, domid, XENMEM_resource_stats_table,
> > +        vcpu, &size);
> > +
> > +    if ( rc )
> > +        err(1, "    Fail: Get size: %d - %s\n", errno, strerror(errno));
> 
> It seems that err() already does print strerror(), and add a "\n", so
> why print it again? Also, if we have strerror(), what the point of
> printing "errno"?

I will remove errno, strerror(errno), and the extra "\n".

> Also, I'm not sure the extra indentation in the error message is really
> useful, but that doesn't really matter.

I will remove the indentation.

> > +
> > +    if ( (size >> XC_PAGE_SHIFT) != nr_frames )
> > +        err(1, "    Fail: Get size: expected %u frames, got %zu\n",
> > +                    nr_frames, size >> XC_PAGE_SHIFT);
> 
> err() prints strerror(errno), maybe errx() is better here. 

I will use errx().

Thanks,
 
> 
> Thanks,
> 
> -- 
> Anthony PERARD
Matias Ezequiel Vara Larsen June 20, 2022, 8:56 a.m. UTC | #3
Hello Anthony, 

On Fri, Jun 03, 2022 at 01:08:20PM +0200, Matias Ezequiel Vara Larsen wrote:
> Hello Anthony and thanks for your comments. I addressed them below:
> 
> On Tue, May 31, 2022 at 12:16:02PM +0100, Anthony PERARD wrote:
> > Hi Matias,
> > 
> > On Tue, May 17, 2022 at 04:33:15PM +0200, Matias Ezequiel Vara Larsen wrote:
> > > Add a demostration tool that uses the stats_table resource to
> > > query vcpu time for a DomU.
> > > 
> > > Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@vates.fr>
> > > ---
> > > diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> > > index 2b683819d4..b510e3aceb 100644
> > > --- a/tools/misc/Makefile
> > > +++ b/tools/misc/Makefile
> > > @@ -135,4 +135,9 @@ xencov: xencov.o
> > >  xen-ucode: xen-ucode.o
> > >  	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
> > >  
> > > +xen-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
> > > +
> > > +xen-stats: xen-stats.o
> > 
> > The tools seems to be only about vcpu, maybe `xen-stats` is a bit too
> > generic. Would `xen-vcpus-stats`, or maybe something with `time` would
> > be better?
> 
> Do you think `xen-vcpus-stats` would be good enough?
> 

I will pick up `xen-vcpus-stats` for v1 if you are not against it.

Thanks,

Matias

> > Also, is it a tools that could be useful enough to be installed by
> > default? Should we at least build it by default so it doesn't rot? (By
> > adding it to only $(TARGETS).)
> 
> I will add to build this tool by default in the next patches' version.
>  
> > > +	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
> > > +
> > >  -include $(DEPS_INCLUDE)
> > > diff --git a/tools/misc/xen-stats.c b/tools/misc/xen-stats.c
> > > new file mode 100644
> > > index 0000000000..5d4a3239cc
> > > --- /dev/null
> > > +++ b/tools/misc/xen-stats.c
> > > @@ -0,0 +1,83 @@
> > > +#include <err.h>
> > > +#include <errno.h>
> > > +#include <error.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <sys/mman.h>
> > > +#include <signal.h>
> > > +
> > > +#include <xenctrl.h>
> > 
> > It seems overkill to use this header when the tool only use
> > xenforeignmemory interface. But I don't know how to replace
> > XC_PAGE_SHIFT, so I guess that's ok.
> > 
> > > +#include <xenforeignmemory.h>
> > > +#include <xen-tools/libs.h>
> > 
> > What do you use this headers for? Is it left over?
> 
> `xenforeignmemory.h` is used for `xenforeignmemory_*` functions.
> `xen-tools/libs.h` is left over so I will remove it in next version.
> 
> > > +static sig_atomic_t interrupted;
> > > +static void close_handler(int signum)
> > > +{
> > > +    interrupted = 1;
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +    xenforeignmemory_handle *fh;
> > > +    xenforeignmemory_resource_handle *res;
> > > +    size_t size;
> > > +    int rc, nr_frames, domid, frec, vcpu;
> > > +    uint64_t * info;
> > > +    struct sigaction act;
> > > +
> > > +    if (argc != 4 ) {
> > > +        fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]);
> > > +        return 1;
> > > +    }
> > > +
> > > +    // TODO: this depends on the resource
> > > +    nr_frames = 1;
> > > +
> > > +    domid = atoi(argv[1]);
> > > +    frec = atoi(argv[3]);
> > > +    vcpu = atoi(argv[2]);
> > 
> > Can you swap the last two line? I think it is better if the order is the same
> > as on the command line.
> 
> Yes, I can.
> 
> > > +
> > > +    act.sa_handler = close_handler;
> > > +    act.sa_flags = 0;
> > > +    sigemptyset(&act.sa_mask);
> > > +    sigaction(SIGHUP,  &act, NULL);
> > > +    sigaction(SIGTERM, &act, NULL);
> > > +    sigaction(SIGINT,  &act, NULL);
> > > +    sigaction(SIGALRM, &act, NULL);
> > > +
> > > +    fh = xenforeignmemory_open(NULL, 0);
> > > +
> > > +    if ( !fh )
> > > +        err(1, "xenforeignmemory_open");
> > > +
> > > +    rc = xenforeignmemory_resource_size(
> > > +        fh, domid, XENMEM_resource_stats_table,
> > > +        vcpu, &size);
> > > +
> > > +    if ( rc )
> > > +        err(1, "    Fail: Get size: %d - %s\n", errno, strerror(errno));
> > 
> > It seems that err() already does print strerror(), and add a "\n", so
> > why print it again? Also, if we have strerror(), what the point of
> > printing "errno"?
> 
> I will remove errno, strerror(errno), and the extra "\n".
> 
> > Also, I'm not sure the extra indentation in the error message is really
> > useful, but that doesn't really matter.
> 
> I will remove the indentation.
> 
> > > +
> > > +    if ( (size >> XC_PAGE_SHIFT) != nr_frames )
> > > +        err(1, "    Fail: Get size: expected %u frames, got %zu\n",
> > > +                    nr_frames, size >> XC_PAGE_SHIFT);
> > 
> > err() prints strerror(errno), maybe errx() is better here. 
> 
> I will use errx().
> 
> Thanks,
>  
> > 
> > Thanks,
> > 
> > -- 
> > Anthony PERARD
diff mbox series

Patch

diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 2b683819d4..b510e3aceb 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -135,4 +135,9 @@  xencov: xencov.o
 xen-ucode: xen-ucode.o
 	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
+xen-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
+
+xen-stats: xen-stats.o
+	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
+
 -include $(DEPS_INCLUDE)
diff --git a/tools/misc/xen-stats.c b/tools/misc/xen-stats.c
new file mode 100644
index 0000000000..5d4a3239cc
--- /dev/null
+++ b/tools/misc/xen-stats.c
@@ -0,0 +1,83 @@ 
+#include <err.h>
+#include <errno.h>
+#include <error.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <signal.h>
+
+#include <xenctrl.h>
+#include <xenforeignmemory.h>
+#include <xen-tools/libs.h>
+
+static sig_atomic_t interrupted;
+static void close_handler(int signum)
+{
+    interrupted = 1;
+}
+
+int main(int argc, char **argv)
+{
+    xenforeignmemory_handle *fh;
+    xenforeignmemory_resource_handle *res;
+    size_t size;
+    int rc, nr_frames, domid, frec, vcpu;
+    uint64_t * info;
+    struct sigaction act;
+
+    if (argc != 4 ) {
+        fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]);
+        return 1;
+    }
+
+    // TODO: this depends on the resource
+    nr_frames = 1;
+
+    domid = atoi(argv[1]);
+    frec = atoi(argv[3]);
+    vcpu = atoi(argv[2]);
+
+    act.sa_handler = close_handler;
+    act.sa_flags = 0;
+    sigemptyset(&act.sa_mask);
+    sigaction(SIGHUP,  &act, NULL);
+    sigaction(SIGTERM, &act, NULL);
+    sigaction(SIGINT,  &act, NULL);
+    sigaction(SIGALRM, &act, NULL);
+
+    fh = xenforeignmemory_open(NULL, 0);
+
+    if ( !fh )
+        err(1, "xenforeignmemory_open");
+
+    rc = xenforeignmemory_resource_size(
+        fh, domid, XENMEM_resource_stats_table,
+        vcpu, &size);
+
+    if ( rc )
+        err(1, "    Fail: Get size: %d - %s\n", errno, strerror(errno));
+
+    if ( (size >> XC_PAGE_SHIFT) != nr_frames )
+        err(1, "    Fail: Get size: expected %u frames, got %zu\n",
+                    nr_frames, size >> XC_PAGE_SHIFT);
+
+    res = xenforeignmemory_map_resource(
+        fh, domid, XENMEM_resource_stats_table,
+        vcpu, 0, size >> XC_PAGE_SHIFT,
+        (void **)&info, PROT_READ, 0);
+
+    if ( !res )
+        err(1, "    Fail: Map %d - %s\n", errno, strerror(errno));
+
+    while ( !interrupted ) {
+        sleep(frec);
+        printf("running cpu_time: %ld\n", *info);
+    }
+
+    rc = xenforeignmemory_unmap_resource(fh, res);
+    if ( rc )
+        err(1, "    Fail: Unmap %d - %s\n", errno, strerror(errno));
+
+    return 0;
+}