Re: [PATCH 09 of 10] xenalyze: add a basic plugin infrastructure

[George Dunlap <george.dunlap@eu.citrix.com>]

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<david.vrabel@citrix.com>
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<stdio.h>
> +#include<string.h>
> +#include<list>
> +
> +#include "plugin.hh"
> +
> +typedef std::list<struct plugin *>  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<xenalyze_plugin*>(plugin->data);
> +    p->process(ri);
> +}
> +
> +void plugin_summary_wrapper(struct plugin *plugin)
> +{
> +    xenalyze_plugin *p = static_cast<xenalyze_plugin*>(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<stdbool.h>
> +
> +#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<errno.h>
>   #include<strings.h>
> @@ -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();
>