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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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;
> +}