Re: [RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool
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 +, 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
--
Julien Grall
Re: [RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool
On Thu, Feb 23, 2023 at 08:31:29PM +, 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 00..29d0efb124
> > > --- /dev/null
> > > +++ b/tools/misc/xen-vcpus-stats.c
> > > @@ -0,0 +1,87 @@
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#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
Re: [RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool
Hello Andrew and thanks for the comments,
On Thu, Feb 23, 2023 at 04:01:09PM +, 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 00..29d0efb124
> > --- /dev/null
> > +++ b/tools/misc/xen-vcpus-stats.c
> > @@ -0,0 +1,87 @@
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#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 \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(_mask);
> > + sigaction(SIGHUP, , NULL);
> > + sigaction(SIGTERM, , NULL);
> > + sigaction(SIGINT, , NULL);
> > + sigaction(SIGALRM, , NULL);
> > +
> > + fh = xenforeignmemory_open(NULL, 0);
> > +
> > + if ( !fh )
> > + err(1, "xenforeignmemory_open");
> > +
> > + rc = xenforeignmemory_resource_size(
> > + fh, domid, XENMEM_resource_stats_table,
> > + 0, );
> > +
> > + 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 **), 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 !=
Re: [RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool
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 00..29d0efb124
--- /dev/null
+++ b/tools/misc/xen-vcpus-stats.c
@@ -0,0 +1,87 @@
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#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,
--
Julien Grall
Re: [RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool
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 00..29d0efb124
> --- /dev/null
> +++ b/tools/misc/xen-vcpus-stats.c
> @@ -0,0 +1,87 @@
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#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 \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(_mask);
> + sigaction(SIGHUP, , NULL);
> + sigaction(SIGTERM, , NULL);
> + sigaction(SIGINT, , NULL);
> + sigaction(SIGALRM, , NULL);
> +
> + fh = xenforeignmemory_open(NULL, 0);
> +
> + if ( !fh )
> + err(1, "xenforeignmemory_open");
> +
> + rc = xenforeignmemory_resource_size(
> + fh, domid, XENMEM_resource_stats_table,
> + 0, );
> +
> + 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 **), 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;
> +}
[RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool
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
---
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
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 00..29d0efb124
--- /dev/null
+++ b/tools/misc/xen-vcpus-stats.c
@@ -0,0 +1,87 @@
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#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 \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(_mask);
+sigaction(SIGHUP, , NULL);
+sigaction(SIGTERM, , NULL);
+sigaction(SIGINT, , NULL);
+sigaction(SIGALRM, , NULL);
+
+fh = xenforeignmemory_open(NULL, 0);
+
+if ( !fh )
+err(1, "xenforeignmemory_open");
+
+rc = xenforeignmemory_resource_size(
+fh, domid, XENMEM_resource_stats_table,
+0, );
+
+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 **), 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;
+}
--
2.25.1
