OpenCL ICD Loader Deinitialization Support

[Kerilk]

This is the follow up of #238 and the implementation of KhronosGroup/OpenCL-Docs#1003.

It depends on KhronosGroup/OpenCL-Docs#1350 and KhronosGroup/OpenCL-Headers#278.

It implements OpenCL ICD Loader de-initialization.

[Kerilk]

Will rebase.

[bashbaug]

Thanks for continuing to work on this!

A few other high-level comments:

• We'll eventually need to update the XML file with these changes, especially the new CL_PLATFORM_UNLOADABLE_KHR enum. Organizationally, would we consider this an update to cl_khr_icd?
• We should rev the loader version as part of these changes, see: OPENCL_ICD_LOADER_VERSION_REV.

[Kerilk]

Thanks for continuing to work on this!

A few other high-level comments:

* We'll eventually need to update the XML file with these changes, especially the new `CL_PLATFORM_UNLOADABLE_KHR` enum.  Organizationally, would we consider this an update to `cl_khr_icd`?

* We should rev the loader version as part of these changes, see: `OPENCL_ICD_LOADER_VERSION_REV`.

Thanks for reviewing this!
Yes, this is the intent, and the PRs linked into the description should implement these changes.
I do consider it an update on cl_khr_icd but it applies to both 1.0 and 2.0 implementations, so I don't know how this works in practice. Just advertising support is enough to enable it, irrespective of the loader version used.

[bashbaug]

Thanks, I think this looks really great! I'll pull this into my source tree and give it a test drive to see if there are any issues.

There are only two remaining TODOs I'd like to see before merging:

1. Document the new environment variable in the README.
2. Bump the project version.

If we bump the project version in (2), can layers use this to figure out whether they can safely return version 200? I was considering updating my layers to version 200, but if I do this unconditionally it means that older ICD loaders will refuse to load them.

[Kerilk]

If we bump the project version in (2), can layers use this to figure out whether they can safely return version 200? I was considering updating my layers to version 200, but if I do this unconditionally it means that older ICD loaders will refuse to load them.

That is one of the things I wanted to brainstorm about. There are ways we could keep all layers version 100 of the API, use a new property that would advertise if the layer can be unloaded. We could then use a new initialization function (InitWithProperties maybe to address future problem like this one). If the layer is initialized with the old function, it would need to do cleanup (if required) in an at_exit callback, otherwise, the loader supports de-initialization of the layer, and will call the appropriate function if found. That is quite different from the current proposal, but could be a better, more future proof approach.

[bashbaug]

Brief discussion summary:

• Layers that call atexit have the potential to be a big problem, although unfortunately this is the only way to reliably do any sort of post-processing when an application is finishing before this PR. This is especially true if the layer needs to call into subsequent layers or drivers to do its post-processing. The key problem is that atexit gets called before library destructors (like the one being used by this PR), so older layers in the chain could be de-initialized via atexit before newer layers are deinitialized via clDeinitLayer (reference link). Additionally, newer layers that are using atexit for compatibility with older loaders will be double-deinitialized by newer loaders, first via atexit and then via clDeinitLayer.

  • Note that library destructors are not a viable replacement for atexit, since may be called in any order.

• We could "fix" this by bumping the layer version to 200. This requires layers to opt-in to the new behavior, which is to de-initialize via clDeinitLayer instead of atexit. This is the behavior currently in this PR, and it nominally "works", but it has a different problem, specifically that the new layers reporting version 200 will not be loaded by old loaders looking for version 100. So, we need a way both for the layer to communicate to the loader that it supports the new de-initialization functionality and for the loader to communicate to the layer that the new de-initialization functionality will be used and atexit is not needed.

• So, instead of bumping the layer version to 200, a better proposal may be:

  1. Keep the layer version unchanged at 100.
  2. Define a new layer initialization function, say clInitLayerV2, which may have the same arguments are clInitLayer or it could have additional arguments. By default, the new loader would only load and insert layers that export this function.
  3. Layers that are initialized via clInitLayerV2 would know that they are being loaded by new loaders and therefore will be de-initialized via clDeinitLayer and hence would not need to use atexit.
  4. Layers may continue to export clInitLayer as well, for compatibility with older loaders. If a layer is initialized with clInitLayer, it will know it is being loaded by an older loader, and it should use atexit or some other mechanism for de-initialization if required, since clDeinitLayer will not be called even if it is exported.
  5. To allow older layers to function with the newer loader, we could additionally add an environment variable instructing the newer loader to initialize layers using the old clInitLayer. When this environment variable is set, older layers (or newer layers also exporting clInitLayer) would continue to function the same as they did with older loaders.

[Kerilk]

I have implemented the above plan. A couple of notes:

• I have added OCL_ICD_FORCE_LEGACY_TERMINATION to disable the new termination scheme, and allow using layer that export the old API entry points. Naming can be revised.
• The new initialization entry point is clInitLayerWithProperties. No properties exist yet.

The new environment variables (OCL_ICD_FORCE_LEGACY_TERMINATION, OCL_ICD_DISABLE_DYNAMIC_LIBRARY_UNLOADING) need to be documented once we agree on naming and behavior.

[bashbaug]

Reminder: Still need to bump the project version and document the new environment variables.

[bashbaug]

In case it helps other revewers, or to prompt other naming suggestions, I believe this option:

1. Initializes layers using the older clInitLayer instead of the newer clInitLayerWithProperties. This means that older layers are supported that do not export clInitLayerWithProperties.
2. Skips all forms of layer and ICD de-initialization. This includes:
   1. Calling clDeinitLayer. Layers will receive no notification from the ICD loader during deinitialization.
   2. Closing layer library handles. Layer library handles will never be closed and will therefore "leak".
   3. Freeing ICD loader memory for layers. This memory will also "leak". We might want to clean this up regardless, but for the full legacy termination experience, we should leak this memory also.
   4. Closing ICD library handles. ICD library handles will also never be closed and will therefore "leak".
   5. Freeing ICD loader memory for ICDs, so this memory will also "leak".

We could try squeezing in something about legacy layers for (1), but the name is already pretty long, so I don't think it's necessary.

[Kerilk]

Added documentation or the environment variables, as well as bumped the version number.

[Kerilk]

OK, I am done tweaking it or now. I think it is ready for a broader review.

[bashbaug]

LGTM, thanks!

I left a few wording suggestions for the descriptions of the new environment variables, but feel free to ignore the suggestions or to apply further rewording if you prefer.

[Kerilk]

Implemented the suggestion from the working group.