Plugin end state

[oceandlr]

@morrone @valleydlr @tom95858 @baallan
We need some clarity on the end state for plugins --- what goes into the term, fini etc.
There is some inconsistency in the current plugins and it would be good to clear this up both for LDMSCON and for the stream_store_csv plugin.

Can we settle this quickly in this thread?
An existing plugin to model off of would be great!

[baallan]

Let's limit the scope of this discussion to the present V4 plugins api, not an improved V5 where plugins with multiple instance no longer need to use so many globals. The underlying considerations apply to both of course.

For store plugins specifically an a normal ldmsd exit, the store's ".close" function is called by ldmsd on all the running store handles. In an abnormal termination (which happens often enough in production scenarios), the ((destructor)) function gets called. In either scenario, we need to keep the code maintainable/debuggable (particularly for memory leaks) by doing the following:

• stop all locally owned threads (rollover, flush)
• attempt to stop anything the framework is doing on the plugin's behalf, such as using a callback referencing the plugin.
• close any open files, free memory, etc.

If we don't do an orderly cleanup, we end up with daemons that hang or core dump, which is very very disconcerting to production system administrators.

The v4 api includes ((constructor)), config, open, store, flush, close, term, ((destructor)).
To avoid duplicate code while ensuring proper cleanup, ((destructor)), term, and close may be written in terms of each other.
In typical ldms v4 deployment, term() is never called from the configuration language level.
In ldmsd exit processing the main cleanup() function closes all the storage policies which in turn .close() the associated store plugin handles.

It is desirable where possible, as indicated elsewhere by @morrone, to avoid putting code in ((const/dest-ructor)) at all.
It is particularly desirable where possible, as indicated elsewhere by @baallan, to avoid putting code in ((const/dest-ructor)) that depends on specific global ldmsd data structures being fully initialized, as this complicates the process of scanning plugins for their usages.

[baallan]

In the particular case of the pending stream csv store plugin, on a reasonable cleanup is:

• stop the plugin's rollover thread
• stop the plugin's flush thread
• perform an ldmsd_stream_close on the client token from each subscribed stream.
• deallocate all the per-stream plugin data.
• deallocate allocated global variables.

Obviously, deallocating structures before the threads that use them are stopped (or informed somehow) is likely to cause hang/crash inducing behaviors due to invalidated pointers.

The plugin actually doesn't export the conventional store .close function, so the normal storage policy close process is not applied.
This leaves the cleanup to be done by the term() function and the ((destructor)). Because the the requirements for term/dest are essentially the same, ((destructor)) can just be a call to term() before cleanup up any globals initialized in ((constructor)).

The tricky part to this plugin is that is driven by more than one thread; specifically, the stream message delivery to the plugin's callback function can happen at any time unless ldmsd has been successfully notified to stop using the callback.

[baallan]

I should note that I have a patched version of the plugin that runs valgrind clean (with respect to the plugin for both memory and thread issues); it uses what seems to be the unsubscribe method:

ldmsd_stream_close(stream_handle->client);

where stream_handle is the object registered with:

stream_handle->client = ldmsd_stream_subscribe(stream, stream_cb, stream_handle);

[oceandlr]

why would stream_close happen in the store?
there isn't a unsubscribe which would be more in alignment.

[baallan]

@oceandlr that is a good question. Since the close operation applied here takes the client token from the corresponding ldmsd_stream_subscribe, it shouldn't affect anything else, right? @tom95858 may be able to elucidate further as to the naming and underlying design intent of ldmsd_stream_close. The implementation of this close seems to simply remove the specific client from a message recipients list, not to affect any other subscriber (such as the aggregator's subscription).

[oceandlr]

Confirmation from @tom95858 "the call to 'unsubscribe' is ldmsd_stream_close(...). The parameter is he client handle that was returned by ldmsd_stream_subscribe(). I'll add better docs in a subsequent patch."

But this is a detour from the larger question.

[baallan]

On the v4 'sampler' plugin api (which arguably the stream csv store should be using), the rdc_sampler/rdc_plugin.c:term() similarly cleans up global gpu resources in a thread safe manner.

[morrone]

@morrone @valleydlr @tom95858 @baallan
We need some clarity on the end state for plugins --- what goes into the term, fini etc.
There is some inconsistency in the current plugins and it would be good to clear this up both for LDMSCON and for the stream_store_csv plugin.

Can we settle this quickly in this thread?
An existing plugin to model off of would be great!

Unfortunately there probably needs to be further software development needed to allow us to give a clean and consistent answer to that question. For instance, I have a long standing issue #51 open on the fact that ldms fails to call the term() function for sampler plugins before it exits. If that has been addressed, I would love to know about it.

Generally speaking, a plugin should stop everything it was doing in term(), close all file descriptors, free all memory that it allocated. Essentially it must ensure that it erases all evidence of itself from memory when the term() API is called. term() is where all normal plugin cleanup should happen.

When you mention "fini", I assume you are talking about the gcc attribute ((destructor)). I would argue that this should almost never be used. Most places I have seen it used so far is because the entity calling the plugin (ldmsd) is being exceedingly sloppy and failing to call term() on the plugins before exit, so the "fini" destructor is used as a cludge.

I don't think we should advertise the use of the destructor (fini) in an LDMSCON setting. Rather, we should repair ldmsd's interaction with plugins.

[oceandlr]

It would be good if we can agree on some rules of thumb that we can apply and a clear example.
In the absence of that, I will make a good faith attempt and that will be that.

[morrone]

Proposed rule of thumb:

• Use the function that you register as .term in the struct ldmsd_plugin to clean up everything in your plugin.

The ibmad sampler plugin might be a reasonable example. It isn't too complicated if you ignore the IB specific parts. :)

[oceandlr]

Yes, I was asking about fini being the static void attribute_ (destructor)), since there seems to be naming convention going on there with init and fini.

Using the function that you register as .term seems reasonable.
I could also see why there would be a constructor that would be outside of the config and could then be reasonably paired with items in the destructor, though.

[morrone]

I would suggest that general pre-config() initialization should happen in "get_plugin()". get_plugin is the first, required entry into any plugin, so it is probably the best place to pre-allocate things "outside of config". I think that is a much better approach then using constructors and destructors. The matching place to deallocate those things (as well as everything else) would be term().

Ideally our API would have more symmetrically named calls. I'll continue to argue for that in the new API. :)

[tom95858]

The constructor/destructor approach creates lots of problems. Most of them have to do with thread creation and fork, but in general, to be useful the approach is only useful if you're managing state globally, i.e. the entry points don't have handles. So I agree Chris.

…

On Wed, Sep 1, 2021 at 5:57 PM Christopher J. Morrone < ***@***.***> wrote: I would suggest that general pre-config() initialization should happen in "get_plugin()". get_plugin is the first, required entry into any plugin, so it is probably the best place to pre-allocate things "outside of config". I think that is a much better approach then using constructors and destructors. The matching place to deallocate those things (as well as everything else) would be term(). Ideally our API would have more symmetrically named calls. I'll continue to argue for that in the new API. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#785 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVTPXEIDV6G5FFJDXW7VGLT72VUDANCNFSM5DE2A3LQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Thomas Tucker, President, Open Grid Computing, Inc.