Re: [Qemu-devel] [RFC v2 05/38] plugin: add user-facing API

[Aaron Lindsay <aaron@os.amperecomputing.com>]

On Dec 14 12:08, Emilio G. Cota wrote:
> On Fri, Dec 14, 2018 at 15:57:32 +0000, Aaron Lindsay wrote:
> > Emilio,
> > 
> > First, thanks for putting this together - I think everyone doing this
> > sort of thing will benefit if we're able to agree on one upstream plugin
> > interface.
> > 
> > One thing I'd like to see is support for unregistering callbacks once
> > they are registered. For instance, you can imagine that a plugin may
> > have 'modality', where it may care about tracing very detailed
> > information in one mode, but trace only limited information otherwise -
> > perhaps only enough to figure out when it needs to switch back to the
> > other mode.
> > 
> > I added a function to the user-facing plugin API in my own version of
> > Pavel's plugin patchset to clear all existing plugin instrumentation,
> > allowing the plugin to drop instrumentation if it was no longer
> > interested. Of course, you could always add logic to a plugin to ignore
> > unwanted callbacks, but it could be significantly more efficient to
> > remove them entirely. In Pavel's patchset, this was essentially just a
> > call to tb_flush(), though I think it would be a little more involved
> > for yours.
> 
> This is a good point.
> 
> "Regular" callbacks can be unregistered by just passing NULL as the
> callback (see plugin_unregister_cb__locked, which is called from
> do_plugin_register_cb).

Ah, okay, thanks - I missed this detail when looking through your
patches earlier.

> "Direct" callbacks, however (i.e. the ones
> that embed a callback directly in the translated code),
> would have to be unregistered by flushing the particular TB
> (or all TBs, which is probably better from an API viewpoint).
> 
> I think the following API call would do what you need:
> 
>   typedef int (*qemu_plugin_reset_cb_t)(qemu_plugin_id_t id);
>   void qemu_plugin_reset(qemu_plugin_id_t id, qemu_plugin_reset_cb_t cb);
> 
> (or maybe qemu_plugin_reinstall?)

I think I prefer your initial suggestion of qemu_plugin_reset - to me it
does a better job communicating that existing callbacks will be removed.
But, then again, _reinstall makes it more clear that you are responsible
for re-adding any callbacks you still want...

> The idea is that a plugin can "reset" itself, so that (1) all
> its CBs are cleared and (2) the plugin can register new callbacks.
> This would all happen in an atomic context (no vCPU running), so
> that the plugin would miss no CPU events.

The implication being that there would not be the same possibility of
other callbacks being called between when qemu_plugin_reset and the
qemu_plugin_reset_cb_t callback are called as there is at plugin
un-installation time?

> How does this sound?

I think what you describe is exactly what I'm interested in.

-Aaron