From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 09 of 10] xenalyze: add a basic plugin infrastructure Date: Thu, 7 Jun 2012 12:05:40 +0100 Message-ID: <4FD08B04.30709@eu.citrix.com> References: <794833ac346b80acbed5.1338462985@qabil.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <794833ac346b80acbed5.1338462985@qabil.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 31/05/12 12:16, David Vrabel wrote: > Allow xenalyze to be include (at build time) plugins that can do > per-record actions and a summary. These plugins can be in C or C++. > > The plugins entry points are in struct plugin and pointers to all the > plugins linked in xenalyze are placed in a "plugin" section so > plugin_init() can find them all. > > A new command line option (-p, --plugin=PLUGIN) is added to enable one > or more plugins. > > A sample plugin (skeleton) is included (mostly because at least one > plugin must be present for the build to work). > > Signed-off-by: David Vrabel So what's the main motivation of having this plugin infrastructure? The one plugin example you have ("batch-size", patch 10) seems simple enough that it should be fairly straightforward to just add as an option, with not much more boilerplate than C++ already requires. Looks like potential advantages may include: * Ability to use C++ language (for those who care for such things) * Ability to use STL for complex data structures * Ability to add an option like the "batch-size" plugin in a concise, self-contained way Potential disadvantages include: * An extra O(N) loop on the hot path (where N = # of enabled plugins) * For each enabled plugin, an extra full function call on the hot path; and a C++ function at that, which (my prejudice tells me) is likely to be more wasteful time and space-wise than a C function. Did I miss anything? Obviosuly thanks for being conscious of this cost by, for example, having a separate list for "enabled" vs "available". I guess I want to be convinced that allowing this kind of plug-in is really worth it, and won't cost too much; especially when something like "batch-size" could be implemented in a pretty straightforward way without needing a plugin. I suppose that if there's a plugin that turns out to be useful, but is expensive when run as a plug-in, we could always pull it into the main file as an optimization. Also, could you do a simple test to measure the overhead of having no plugins? Just finding a 700MB-ish file you have lying around and running "xenalyze -s" with and with this patch (and with this patch and -p skeleton or -p batch-size) would be pretty informative, and shouldn't take too long. Thanks, -George > --- > Makefile | 10 +++--- > analyze.h | 55 ++++++++++++++++++++++++++++++++++ > plugin.cc | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > plugin.h | 48 +++++++++++++++++++++++++++++ > plugin.hh | 42 ++++++++++++++++++++++++++ > plugins/skeleton.cc | 31 +++++++++++++++++++ > xenalyze.c | 72 +++++++++++++------------------------------- > 7 files changed, 287 insertions(+), 55 deletions(-) > > diff --git a/Makefile b/Makefile > --- a/Makefile > +++ b/Makefile > @@ -1,4 +1,4 @@ > -CFLAGS += -g -O2 > +CFLAGS += -g -O2 -I. > CFLAGS += -fno-strict-aliasing > CFLAGS += -std=gnu99 > CFLAGS += -Wall -Wstrict-prototypes > @@ -9,11 +9,15 @@ CFLAGS += -D_LARGEFILE_SOURCE -D_LARGEFI > CFLAGS += -mno-tls-direct-seg-refs > CFLAGS += -Werror > > +CXXFLAGS := -g -O2 -I. -Wall -Werror -std=c++0x > + > BIN := xenalyze dump-raw > > xenalyze_OBJS := \ > mread.o \ > - xenalyze.o > + plugin.o \ > + xenalyze.o \ > + plugins/skeleton.o > > dump-raw_OBJS := \ > dump-raw.o > @@ -21,7 +25,7 @@ dump-raw_OBJS := \ > all: $(BIN) > > xenalyze: $(xenalyze_OBJS) > - $(CC) $(LDFLAGS) -o $@ $^ > + $(CXX) $(LDFLAGS) -o $@ $^ > > dump-raw: $(dump-raw_OBJS) > $(CC) $(LDFLAGS) -o $@ $^ > @@ -29,6 +33,9 @@ dump-raw: $(dump-raw_OBJS) > %.o: %.c > $(CC) $(CFLAGS) -MD -MP -c -o $@ $< > > +%.o: %.cc > + $(CXX) $(CXXFLAGS) -MD -MP -c -o $@ $< > + > all_objs := $(foreach b,$(BIN),$($(b)_OBJS)) > all_deps := $(all_objs:.o=.d) > > diff --git a/plugin.cc b/plugin.cc > new file mode 100644 > --- /dev/null > +++ b/plugin.cc > @@ -0,0 +1,84 @@ > +/* > + * Xenalyze plugin infrastructure. > + * > + * Copyright (C) 2012, Citrix Systems R&D Ltd > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + */ > +#include > +#include > +#include > + > +#include "plugin.hh" > + > +typedef std::list plugin_list; > + > +static plugin_list available; > +static plugin_list enabled; > + > +bool plugin_enable(const char *name) > +{ > + for (auto p = available.begin(); p != available.end(); p++) { > + struct plugin *plugin = *p; > + if (strcmp(plugin->name, name) == 0) { > + enabled.push_back(plugin); > + if (plugin->enable) { > + plugin->enable(plugin); > + } > + return true; > + } > + } > + return false; > +} > + > +void plugin_process(const struct record_info *ri) > +{ > + for (auto p = enabled.begin(); p != enabled.end(); p++) { > + struct plugin *plugin = *p; > + if (plugin->process) { > + plugin->process(plugin, ri); > + } > + } > +} > + > +void plugin_summary(void) > +{ > + for (auto p = enabled.begin(); p != enabled.end(); p++) { > + struct plugin *plugin = *p; > + if (plugin->summary) { > + printf("Summary for %s plugin:\n", plugin->name); > + plugin->summary(plugin); > + } > + } > +} > + > +static void plugin_add(struct plugin *plugin) > +{ > + available.push_back(plugin); > +} > + > +void plugin_init(void) > +{ > + extern struct plugin *__start_plugin; > + extern struct plugin *__stop_plugin; > + struct plugin **p; > + > + for (p =&__start_plugin; p< &__stop_plugin; p++) { > + plugin_add(*p); > + } > +} > + > +void plugin_process_wrapper(struct plugin *plugin, const struct record_info *ri) > +{ > + xenalyze_plugin *p = static_cast(plugin->data); > + p->process(ri); > +} > + > +void plugin_summary_wrapper(struct plugin *plugin) > +{ > + xenalyze_plugin *p = static_cast(plugin->data); > + p->summary(); > + delete p; > +} > diff --git a/plugin.h b/plugin.h > new file mode 100644 > --- /dev/null > +++ b/plugin.h > @@ -0,0 +1,47 @@ > +/* > + * Xenalyze plugin C API. > + * > + * Copyright (C) 2012, Citrix Systems R&D Ltd > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + */ > +#ifndef PLUGIN_H > +#define PLUGIN_H > + > +#include > + > +#include "analyze.h" > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +struct plugin; > + > +typedef void (*plugin_enable_f)(struct plugin *plugin); > +typedef void (*plugin_process_f)(struct plugin *plugin, const struct record_info *ri); > +typedef void (*plugin_summary_f)(struct plugin *plugin); > + > +struct plugin { > + const char *name; > + plugin_enable_f enable; > + plugin_process_f process; > + plugin_summary_f summary; > + void *data; > +}; > + > +#define DEFINE_PLUGIN(p) \ > + struct plugin *__plugin_ ## p __attribute__((section("plugin"))) =&p > + > +void plugin_init(void); > +bool plugin_enable(const char *name); > +void plugin_process(const struct record_info *ri); > +void plugin_summary(void); > + > +#ifdef __cplusplus > +} /* extern "C" */ > +#endif > + > +#endif /* #ifndef PLUGIN_H */ > diff --git a/plugin.hh b/plugin.hh > new file mode 100644 > --- /dev/null > +++ b/plugin.hh > @@ -0,0 +1,42 @@ > +/* > + * Xenalyze plugin C++ API. > + * > + * Copyright (C) 2012, Citrix Systems R&D Ltd > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + */ > +#ifndef PLUGIN_HH > +#define PLUGIN_HH > + > +#include "plugin.h" > + > +class xenalyze_plugin { > +public: > + virtual ~xenalyze_plugin() {} > + > + virtual void process(const struct record_info *ri) = 0; > + virtual void summary() = 0; > +}; > + > +#define DEFINE_CXX_PLUGIN(name, cls) \ > + static void __plugin_ ## cls ## _enable(struct plugin *plugin) \ > + { \ > + plugin->data = new cls(); \ > + } \ > + \ > + static struct plugin __plugin ## cls = { \ > + name, \ > + __plugin_ ## cls ## _enable, \ > + plugin_process_wrapper, \ > + plugin_summary_wrapper, \ > + }; \ > + DEFINE_PLUGIN(__plugin ## cls) > + > +extern "C" { > +void plugin_process_wrapper(struct plugin *plugin, const struct record_info *ri); > +void plugin_summary_wrapper(struct plugin *plugin); > +} > + > +#endif /* #ifndef PLUGIN_HH */ > diff --git a/plugins/skeleton.cc b/plugins/skeleton.cc > new file mode 100644 > --- /dev/null > +++ b/plugins/skeleton.cc > @@ -0,0 +1,31 @@ > +/* > + * Skeleton xenalyze plugin. > + * > + * Copyright (C) 2012, Citrix Systems R&D Ltd > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + */ > +#include "plugin.hh" > + > +class skeleton_plugin : xenalyze_plugin { > +public: > + skeleton_plugin() {} > + ~skeleton_plugin() {} > + > + void process(const struct record_info *ri); > + void summary(void); > +}; > + > +void skeleton_plugin::process(const struct record_info *ri) > +{ > + /* Put per-trace record stuff here. */ > +} > + > +void skeleton_plugin::summary(void) > +{ > + /* Print a summary of the results (if applicable). */ > +} > + > +DEFINE_CXX_PLUGIN("skeleton", skeleton_plugin); > diff --git a/xenalyze.c b/xenalyze.c > --- a/xenalyze.c > +++ b/xenalyze.c > @@ -32,6 +32,7 @@ > #include "trace.h" > #include "analyze.h" > #include "mread.h" > +#include "plugin.h" > #include "pv.h" > #include > #include > @@ -8763,6 +8764,8 @@ void process_record(struct pcpu_info *p) > default: > process_generic(ri); > } > + > + plugin_process(ri); > } > > UPDATE_VOLUME(p, toplevel[toplevel], ri->size); > @@ -9346,6 +9349,7 @@ enum { > OPT_DUMP_ALL='a', > OPT_INTERVAL_LENGTH='i', > OPT_SUMMARY='s', > + OPT_PLUGIN='p', > }; > > enum { > @@ -9816,6 +9820,15 @@ error_t cmd_parser(int key, char *arg, s > opt.tsc_loop_fatal = 1; > break; > > + case OPT_PLUGIN: > + if (plugin_enable(arg)) { > + G.output_defined = 1; > + } else { > + fprintf(stderr, "ERROR: No such plugin `%s'.\n", arg); > + exit(1); > + } > + break; > + > case ARGP_KEY_ARG: > { > /* FIXME - strcpy */ > @@ -10108,6 +10121,10 @@ const struct argp_option cmd_opts[] = { > .arg = "errlevel", > .doc = "Sets tolerance for errors found in the file. Default is 3; max is 6.", }, > > + { .name = "plugin", > + .key = OPT_PLUGIN, > + .arg = "PLUGIN", > + .doc = "Enable a decoder or summary plugin.", }, > > { 0 }, > }; > @@ -10127,6 +10144,8 @@ int main(int argc, char *argv[]) { > /* Start with warn at stderr. */ > warn = stderr; > > + plugin_init(); > + > argp_parse(&parser_def, argc, argv, 0, NULL, NULL); > > if (G.trace_file == NULL) > @@ -10163,6 +10182,8 @@ int main(int argc, char *argv[]) { > if(opt.summary) > summary(); > > + plugin_summary(); > + > if(opt.report_pcpu) > report_pcpu(); >