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

[Matias Ezequiel Vara Larsen <matiasevara@xxxxxxxxx>]

Hello Anthony, 

On Fri, Jun 03, 2022 at 01:08:20PM +0200, Matias Ezequiel Vara Larsen wrote:
> Hello Anthony and thanks for your comments. I addressed them below:
> 
> On Tue, May 31, 2022 at 12:16:02PM +0100, Anthony PERARD wrote:
> > Hi Matias,
> > 
> > On Tue, May 17, 2022 at 04:33:15PM +0200, Matias Ezequiel Vara Larsen wrote:
> > > Add a demostration tool that uses the stats_table resource to
> > > query vcpu time for a DomU.
> > > 
> > > Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@xxxxxxxx>
> > > ---
> > > diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> > > index 2b683819d4..b510e3aceb 100644
> > > --- a/tools/misc/Makefile
> > > +++ b/tools/misc/Makefile
> > > @@ -135,4 +135,9 @@ xencov: xencov.o
> > >  xen-ucode: xen-ucode.o
> > >   $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
> > >  
> > > +xen-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
> > > +
> > > +xen-stats: xen-stats.o
> > 
> > The tools seems to be only about vcpu, maybe `xen-stats` is a bit too
> > generic. Would `xen-vcpus-stats`, or maybe something with `time` would
> > be better?
> 
> Do you think `xen-vcpus-stats` would be good enough?
> 

I will pick up `xen-vcpus-stats` for v1 if you are not against it.

Thanks,

Matias

> > Also, is it a tools that could be useful enough to be installed by
> > default? Should we at least build it by default so it doesn't rot? (By
> > adding it to only $(TARGETS).)
> 
> I will add to build this tool by default in the next patches' version.
>  
> > > + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) 
> > > $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
> > > +
> > >  -include $(DEPS_INCLUDE)
> > > diff --git a/tools/misc/xen-stats.c b/tools/misc/xen-stats.c
> > > new file mode 100644
> > > index 0000000000..5d4a3239cc
> > > --- /dev/null
> > > +++ b/tools/misc/xen-stats.c
> > > @@ -0,0 +1,83 @@
> > > +#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>
> > 
> > It seems overkill to use this header when the tool only use
> > xenforeignmemory interface. But I don't know how to replace
> > XC_PAGE_SHIFT, so I guess that's ok.
> > 
> > > +#include <xenforeignmemory.h>
> > > +#include <xen-tools/libs.h>
> > 
> > What do you use this headers for? Is it left over?
> 
> `xenforeignmemory.h` is used for `xenforeignmemory_*` functions.
> `xen-tools/libs.h` is left over so I will remove it in next version.
> 
> > > +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, nr_frames, domid, frec, vcpu;
> > > +    uint64_t * info;
> > > +    struct sigaction act;
> > > +
> > > +    if (argc != 4 ) {
> > > +        fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]);
> > > +        return 1;
> > > +    }
> > > +
> > > +    // TODO: this depends on the resource
> > > +    nr_frames = 1;
> > > +
> > > +    domid = atoi(argv[1]);
> > > +    frec = atoi(argv[3]);
> > > +    vcpu = atoi(argv[2]);
> > 
> > Can you swap the last two line? I think it is better if the order is the 
> > same
> > as on the command line.
> 
> Yes, I can.
> 
> > > +
> > > +    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,
> > > +        vcpu, &size);
> > > +
> > > +    if ( rc )
> > > +        err(1, "    Fail: Get size: %d - %s\n", errno, strerror(errno));
> > 
> > It seems that err() already does print strerror(), and add a "\n", so
> > why print it again? Also, if we have strerror(), what the point of
> > printing "errno"?
> 
> I will remove errno, strerror(errno), and the extra "\n".
> 
> > Also, I'm not sure the extra indentation in the error message is really
> > useful, but that doesn't really matter.
> 
> I will remove the indentation.
> 
> > > +
> > > +    if ( (size >> XC_PAGE_SHIFT) != nr_frames )
> > > +        err(1, "    Fail: Get size: expected %u frames, got %zu\n",
> > > +                    nr_frames, size >> XC_PAGE_SHIFT);
> > 
> > err() prints strerror(errno), maybe errx() is better here. 
> 
> I will use errx().
> 
> Thanks,
>  
> > 
> > Thanks,
> > 
> > -- 
> > Anthony PERARD