Re: [RFC PATCH v2 2/2] tools/misc: Add xen-vcpus-stats tool

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall