| Message ID | e233c4f60c6fe97b93b3adf9affeb0404c554130.1652797713.git.matias.vara@vates.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a new acquire resource to query vcpu statistics | expand |
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,
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
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 --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; +}
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