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

[Matias Ezequiel Vara Larsen <matiasevara@gmail.com>]

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