[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH 2/2] tools/misc: Add xen-stats tool
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? > 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |