| Message ID | df706d6dcfba965909d776dda163fdb261d1a0ea.1665138677.git.matias.vara@vates.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a new acquire resource to query vcpu statistics | expand |
On 07/10/2022 1:39 pm, Matias Ezequiel Vara Larsen wrote: A couple of observations, all unrelated to the stats themselves. Although overall, I'm not entirely certain that a tool like this is going to be very helpful after initial development. Something to consider would be to alter libxenstat to use this new interface? > diff --git a/tools/misc/Makefile b/tools/misc/Makefile > index 2b683819d4..837e4b50da 100644 > --- a/tools/misc/Makefile > +++ b/tools/misc/Makefile > @@ -49,6 +49,7 @@ TARGETS_COPY += xenpvnetboot > > # Everything which needs to be built > TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL)) > +TARGETS_BUILD += xen-vcpus-stats This patch is whitespace corrupted. If at all possible, you need to see about getting `git send-email` working to send patches with, as it deals with most of the whitespace problems for you. I'm afraid you can't simply copy the patch text into an email and send that. > > # ... including build-only targets > TARGETS_BUILD-$(CONFIG_X86) += xen-vmtrace > @@ -135,4 +136,9 @@ xencov: xencov.o > xen-ucode: xen-ucode.o > $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) > > +xen-vcpus-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory) > + > +xen-vcpus-stats: xen-vcpus-stats.o > + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) > $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS) > + > -include $(DEPS_INCLUDE) > diff --git a/tools/misc/xen-vcpus-stats.c b/tools/misc/xen-vcpus-stats.c > new file mode 100644 > index 0000000000..29d0efb124 > --- /dev/null > +++ b/tools/misc/xen-vcpus-stats.c > @@ -0,0 +1,87 @@ > +#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/vcpu.h> > + > +#define rmb() asm volatile("lfence":::"memory") This is rmb(), but rmb() isn't what you want. You want smp_rmb(), which is #define smp_rmb() asm volatile ("" ::: "memory") I'm surprised we haven't got this in a common location, considering how often it goes wrong. (Doesn't help that there's plenty of buggy examples to copy, even in xen.git) > + > +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, domid, period, vcpu; > + shared_vcpustatspage_t * info; shared_vcpustatspage_t *info; no space after the *. But you also cannot have a single structure describing that. I'll reply to the cover letter discussing ABIs. > + struct sigaction act; > + uint32_t version; > + uint64_t value; > + > + if (argc != 4 ) { { on a new line. > + fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]); > + return 1; > + } > + > + domid = atoi(argv[1]); > + vcpu = atoi(argv[2]); > + period = atoi(argv[3]); > + > + 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, > + 0, &size); > + > + if ( rc ) > + err(1, "Fail: Get size"); > + > + res = xenforeignmemory_map_resource( > + fh, domid, XENMEM_resource_stats_table, > + 0, XENMEM_resource_stats_frame_vcpustats, size >> XC_PAGE_SHIFT, > + (void **)&info, PROT_READ, 0); > + > + if ( !res ) > + err(1, "Fail: Map"); > + > + while ( !interrupted ) { { on newline again. > + sleep(period); > + do { > + version = info->vcpu_info[vcpu].version; > + rmb(); > + value = info->vcpu_info[vcpu].runstate_running_time; > + rmb(); > + } while ((info->vcpu_info[vcpu].version & 1) || > + (version != info->vcpu_info[vcpu].version)); So I think this will function correctly. But I do recall seeing a rather nice way of wrapping a sequence lock in C99. I'll see if I can find it. > + printf("running_vcpu_time[%d]: %ld\n", vcpu, value); > + } > + > + rc = xenforeignmemory_unmap_resource(fh, res); > + if ( rc ) > + err(1, "Fail: Unmap"); Given that you unmap(), you ought to close the fh handle too. ~Andrew > + > + return 0; > +}
Hi, On 23/02/2023 16:01, Andrew Cooper wrote: > On 07/10/2022 1:39 pm, Matias Ezequiel Vara Larsen wrote: > > A couple of observations, all unrelated to the stats themselves. > > Although overall, I'm not entirely certain that a tool like this is > going to be very helpful after initial development. Something to > consider would be to alter libxenstat to use this new interface? > >> diff --git a/tools/misc/Makefile b/tools/misc/Makefile >> index 2b683819d4..837e4b50da 100644 >> --- a/tools/misc/Makefile >> +++ b/tools/misc/Makefile >> @@ -49,6 +49,7 @@ TARGETS_COPY += xenpvnetboot >> >> # Everything which needs to be built >> TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL)) >> +TARGETS_BUILD += xen-vcpus-stats > > This patch is whitespace corrupted. If at all possible, you need to see > about getting `git send-email` working to send patches with, as it deals > with most of the whitespace problems for you. > > I'm afraid you can't simply copy the patch text into an email and send that. > >> >> # ... including build-only targets >> TARGETS_BUILD-$(CONFIG_X86) += xen-vmtrace >> @@ -135,4 +136,9 @@ xencov: xencov.o >> xen-ucode: xen-ucode.o >> $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) >> >> +xen-vcpus-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory) >> + >> +xen-vcpus-stats: xen-vcpus-stats.o >> + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) >> $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS) >> + >> -include $(DEPS_INCLUDE) >> diff --git a/tools/misc/xen-vcpus-stats.c b/tools/misc/xen-vcpus-stats.c >> new file mode 100644 >> index 0000000000..29d0efb124 >> --- /dev/null >> +++ b/tools/misc/xen-vcpus-stats.c >> @@ -0,0 +1,87 @@ >> +#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/vcpu.h> >> + >> +#define rmb() asm volatile("lfence":::"memory") > > This is rmb(), but rmb() isn't what you want. > > You want smp_rmb(), which is > > #define smp_rmb() asm volatile ("" ::: "memory") From the generic PoV, I find smp_rmb() a bit misleading because it is not clear in this context whether we are referring to the SMP-ness of the hypervisor or the tools domain. If the latter, then technically it could be uniprocessor domain and one could argue that for Arm it could be downgraded to just a compiler barrier. AFAICT, this would not be the case here because we are getting data from Xen. So we always need a "dmb ish". So, I would suggest to name it virt_*() (to match Linux's naming). Also, is this tool meant to be arch-agnostic? If so, then we need to introduce the proper barrier for the other arch. Cheers,
Hello Andrew and thanks for the comments, On Thu, Feb 23, 2023 at 04:01:09PM +0000, Andrew Cooper wrote: > On 07/10/2022 1:39 pm, Matias Ezequiel Vara Larsen wrote: > > A couple of observations, all unrelated to the stats themselves. > > Although overall, I'm not entirely certain that a tool like this is > going to be very helpful after initial development. Something to > consider would be to alter libxenstat to use this new interface? > Yes. We discussed about this in a design sesion at the summit. I could not move forward on that direction yet but it is the right way to go. I use this tool only to play with the interface and I could just remove it from the RFC in next versions. > > diff --git a/tools/misc/Makefile b/tools/misc/Makefile > > index 2b683819d4..837e4b50da 100644 > > --- a/tools/misc/Makefile > > +++ b/tools/misc/Makefile > > @@ -49,6 +49,7 @@ TARGETS_COPY += xenpvnetboot > > > > # Everything which needs to be built > > TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL)) > > +TARGETS_BUILD += xen-vcpus-stats > > This patch is whitespace corrupted. If at all possible, you need to see > about getting `git send-email` working to send patches with, as it deals > with most of the whitespace problems for you. > > I'm afraid you can't simply copy the patch text into an email and send that. > I am using `git send-email` to send patches. I may have missed some flag. I'll double-check. > > > > # ... including build-only targets > > TARGETS_BUILD-$(CONFIG_X86) += xen-vmtrace > > @@ -135,4 +136,9 @@ xencov: xencov.o > > xen-ucode: xen-ucode.o > > $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) > > > > +xen-vcpus-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory) > > + > > +xen-vcpus-stats: xen-vcpus-stats.o > > + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) > > $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS) > > + > > -include $(DEPS_INCLUDE) > > diff --git a/tools/misc/xen-vcpus-stats.c b/tools/misc/xen-vcpus-stats.c > > new file mode 100644 > > index 0000000000..29d0efb124 > > --- /dev/null > > +++ b/tools/misc/xen-vcpus-stats.c > > @@ -0,0 +1,87 @@ > > +#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/vcpu.h> > > + > > +#define rmb() asm volatile("lfence":::"memory") > > This is rmb(), but rmb() isn't what you want. > > You want smp_rmb(), which is > > #define smp_rmb() asm volatile ("" ::: "memory") > > > I'm surprised we haven't got this in a common location, considering how > often it goes wrong. (Doesn't help that there's plenty of buggy > examples to copy, even in xen.git) > Got it. I'll rework on it in the next version. For inspiration, I used the code at arch/x86/kernel/pvclock.c:pvclock_read_wallclock(). > > + > > +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, domid, period, vcpu; > > + shared_vcpustatspage_t * info; > > shared_vcpustatspage_t *info; > > no space after the *. > > But you also cannot have a single structure describing that. I'll reply > to the cover letter discussing ABIs. I am reading it and I will comment on this soon. > > > + struct sigaction act; > > + uint32_t version; > > + uint64_t value; > > + > > + if (argc != 4 ) { > > { on a new line. > > > + fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]); > > + return 1; > > + } > > + > > + domid = atoi(argv[1]); > > + vcpu = atoi(argv[2]); > > + period = atoi(argv[3]); > > + > > + 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, > > + 0, &size); > > + > > + if ( rc ) > > + err(1, "Fail: Get size"); > > + > > + res = xenforeignmemory_map_resource( > > + fh, domid, XENMEM_resource_stats_table, > > + 0, XENMEM_resource_stats_frame_vcpustats, size >> XC_PAGE_SHIFT, > > + (void **)&info, PROT_READ, 0); > > + > > + if ( !res ) > > + err(1, "Fail: Map"); > > + > > + while ( !interrupted ) { > > { on newline again. > > > + sleep(period); > > + do { > > + version = info->vcpu_info[vcpu].version; > > + rmb(); > > + value = info->vcpu_info[vcpu].runstate_running_time; > > + rmb(); > > + } while ((info->vcpu_info[vcpu].version & 1) || > > + (version != info->vcpu_info[vcpu].version)); > > So I think this will function correctly. > > But I do recall seeing a rather nice way of wrapping a sequence lock in > C99. I'll see if I can find it. > > > + printf("running_vcpu_time[%d]: %ld\n", vcpu, value); > > + } > > + > > + rc = xenforeignmemory_unmap_resource(fh, res); > > + if ( rc ) > > + err(1, "Fail: Unmap"); > > Given that you unmap(), you ought to close the fh handle too. > Thanks, I'll fix these issues in the next version. I think Jan's review have already spotted some of them. Matias
On Thu, Feb 23, 2023 at 08:31:29PM +0000, Julien Grall wrote: > Hi, > > On 23/02/2023 16:01, Andrew Cooper wrote: > > On 07/10/2022 1:39 pm, Matias Ezequiel Vara Larsen wrote: > > > > A couple of observations, all unrelated to the stats themselves. > > > > Although overall, I'm not entirely certain that a tool like this is > > going to be very helpful after initial development. Something to > > consider would be to alter libxenstat to use this new interface? > > > > > diff --git a/tools/misc/Makefile b/tools/misc/Makefile > > > index 2b683819d4..837e4b50da 100644 > > > --- a/tools/misc/Makefile > > > +++ b/tools/misc/Makefile > > > @@ -49,6 +49,7 @@ TARGETS_COPY += xenpvnetboot > > > > > > # Everything which needs to be built > > > TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL)) > > > +TARGETS_BUILD += xen-vcpus-stats > > > > This patch is whitespace corrupted. If at all possible, you need to see > > about getting `git send-email` working to send patches with, as it deals > > with most of the whitespace problems for you. > > > > I'm afraid you can't simply copy the patch text into an email and send that. > > > > > > > > # ... including build-only targets > > > TARGETS_BUILD-$(CONFIG_X86) += xen-vmtrace > > > @@ -135,4 +136,9 @@ xencov: xencov.o > > > xen-ucode: xen-ucode.o > > > $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) > > > > > > +xen-vcpus-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory) > > > + > > > +xen-vcpus-stats: xen-vcpus-stats.o > > > + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) > > > $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS) > > > + > > > -include $(DEPS_INCLUDE) > > > diff --git a/tools/misc/xen-vcpus-stats.c b/tools/misc/xen-vcpus-stats.c > > > new file mode 100644 > > > index 0000000000..29d0efb124 > > > --- /dev/null > > > +++ b/tools/misc/xen-vcpus-stats.c > > > @@ -0,0 +1,87 @@ > > > +#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/vcpu.h> > > > + > > > +#define rmb() asm volatile("lfence":::"memory") > > > > This is rmb(), but rmb() isn't what you want. > > > > You want smp_rmb(), which is > > > > #define smp_rmb() asm volatile ("" ::: "memory") > > From the generic PoV, I find smp_rmb() a bit misleading because it is not > clear in this context whether we are referring to the SMP-ness of the > hypervisor or the tools domain. > > If the latter, then technically it could be uniprocessor domain and one > could argue that for Arm it could be downgraded to just a compiler barrier. > > AFAICT, this would not be the case here because we are getting data from > Xen. So we always need a "dmb ish". > > So, I would suggest to name it virt_*() (to match Linux's naming). > > Also, is this tool meant to be arch-agnostic? If so, then we need to > introduce the proper barrier for the other arch. > Thanks Julien for the comment. Is it `xen_rmb()` meant for that? Matias
Hi, Sorry for the late reply. On 17/03/2023 11:01, Matias Ezequiel Vara Larsen wrote: > On Thu, Feb 23, 2023 at 08:31:29PM +0000, Julien Grall wrote: >>>> +#define rmb() asm volatile("lfence":::"memory") >>> >>> This is rmb(), but rmb() isn't what you want. >>> >>> You want smp_rmb(), which is >>> >>> #define smp_rmb() asm volatile ("" ::: "memory") >> >> From the generic PoV, I find smp_rmb() a bit misleading because it is not >> clear in this context whether we are referring to the SMP-ness of the >> hypervisor or the tools domain. >> >> If the latter, then technically it could be uniprocessor domain and one >> could argue that for Arm it could be downgraded to just a compiler barrier. >> >> AFAICT, this would not be the case here because we are getting data from >> Xen. So we always need a "dmb ish". >> >> So, I would suggest to name it virt_*() (to match Linux's naming). >> >> Also, is this tool meant to be arch-agnostic? If so, then we need to >> introduce the proper barrier for the other arch. >> > Thanks Julien for the comment. Is it `xen_rmb()` meant for that? I believe so. > > Matias
diff --git a/tools/misc/Makefile b/tools/misc/Makefile index 2b683819d4..837e4b50da 100644 --- a/tools/misc/Makefile +++ b/tools/misc/Makefile @@ -49,6 +49,7 @@ TARGETS_COPY += xenpvnetboot # Everything which needs to be built TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL)) +TARGETS_BUILD += xen-vcpus-stats # ... including build-only targets TARGETS_BUILD-$(CONFIG_X86) += xen-vmtrace @@ -135,4 +136,9 @@ xencov: xencov.o xen-ucode: xen-ucode.o $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) +xen-vcpus-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory) + +xen-vcpus-stats: xen-vcpus-stats.o + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS) + -include $(DEPS_INCLUDE) diff --git a/tools/misc/xen-vcpus-stats.c b/tools/misc/xen-vcpus-stats.c new file mode 100644 index 0000000000..29d0efb124 --- /dev/null +++ b/tools/misc/xen-vcpus-stats.c @@ -0,0 +1,87 @@ +#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/vcpu.h> + +#define rmb() asm volatile("lfence":::"memory") + +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, domid, period, vcpu; + shared_vcpustatspage_t * info; + struct sigaction act; + uint32_t version; + uint64_t value; + + if (argc != 4 ) { + fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]); + return 1; + } + + domid = atoi(argv[1]); + vcpu = atoi(argv[2]); + period = atoi(argv[3]); + + 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, + 0, &size); + + if ( rc ) + err(1, "Fail: Get size"); + + res = xenforeignmemory_map_resource( + fh, domid, XENMEM_resource_stats_table, + 0, XENMEM_resource_stats_frame_vcpustats, size >> XC_PAGE_SHIFT, + (void **)&info, PROT_READ, 0); + + if ( !res ) + err(1, "Fail: Map"); + + while ( !interrupted ) { + sleep(period); + do { + version = info->vcpu_info[vcpu].version; + rmb(); + value = info->vcpu_info[vcpu].runstate_running_time; + rmb(); + } while ((info->vcpu_info[vcpu].version & 1) || + (version != info->vcpu_info[vcpu].version)); + printf("running_vcpu_time[%d]: %ld\n", vcpu, value); + } + + rc = xenforeignmemory_unmap_resource(fh, res); + if ( rc ) + err(1, "Fail: Unmap"); + + return 0; +}
Add a demonstration tool that uses the stats_table resource to query vcpus' RUNSTATE_running counter for a DomU. Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@vates.fr> --- Changes in v2: - use period instead of frec - rely on version to ensure reading is coherent Changes in v1: - change the name of the tool to xen-vcpus-stats - set command line parameters in the same order that are passed - remove header libs.h - build by default - remove errno, strerrno, "\n", and identation - use errx when errno is not needed - address better the number of pages requested and error msgs - use the shared_vcpustatspage_t structure - use the correct frame id when requesting the resource --- tools/misc/Makefile | 6 +++ tools/misc/xen-vcpus-stats.c | 87 ++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 tools/misc/xen-vcpus-stats.c