Semantics around clIcdSetPlatformDispatchDataKHR and clGetDeviceIDs needs further clarification

[karolherbst]

I'm trying to implement cl_khr_icd version 2.0.0 and I'm curious on the semantics of clIcdSetPlatformDispatchDataKHR in regards to cl_device_id objects.

The spec states that "The vendor driver is responsible for propagating the dispatch_data value when returning objects to users.", however implementations might not return new cl_device_id every time clGetDeviceIDs gets called and reuse them internally. At least I don't think there was a reason that prevented this.

Now with cl_khr_icd version 2.0.0 each object returned has this dispatch_data field, and an implementation may initialize the cl_device_id before or after the clIcdSetPlatformDispatchDataKHR call.

My assumption is, that the ICD loader will do so on its initialization routine, so an implementation could just update the dispatch_data field of all already created cl_device_id objects inside clIcdSetPlatformDispatchDataKHR.

What I'm more curious about is, is the ICD loader allowed to call clIcdSetPlatformDispatchDataKHR later on again? What if clGetDeviceIDs gets called again? Do the devices have to contain the updated dispatch_data value potentially updating it also for previously returned cl_device_id objects?

Is the assumption that clIcdSetPlatformDispatchDataKHR gets only called exactly once and none of those issues are of any concern? I'm assuming this was the intention, but I think it would be better to clarify the expectations here.

[Kerilk]

No, I think I had specified that the loader would set the value once during initialization. There shouldn't be any race conditions either as we are in the init once routine.

The only scenario where I could see this called twice would be in the case where the loader library gets unloaded and reloaded, while the driver does not because it is not unloadable (see KhronosGroup/OpenCL-ICD-Loader#250). In this case, I expect only the platforms and root devices to be set, and the old data to be discarded (it would have been freed during deinitialization). The other objects, if they have not been freed, should have been leaked at this point, or the user is using dead handles.

Hopefully this clarifies things? Let me know if you think the wording of the spec needs to be changed.

[karolherbst]

No, I think I had specified that the loader would set the value once during initialization. There shouldn't be any race conditions either as we are in the init once routine.

The only scenario where I could see this called twice would be in the case where the loader library gets unloaded and reloaded, while the driver does not because it is not unloadable (see KhronosGroup/OpenCL-ICD-Loader#250). In this case, I expect only the platforms and root devices to be set, and the old data to be discarded (it would have been freed during deinitialization). The other objects, if they have not been freed, should have been leaked at this point, or the user is using dead handles.

Hopefully this clarifies things? Let me know if you think the wording of the spec needs to be changed.

Right, that was my assumption, I think it would be best to simply specify that clIcdSetPlatformDispatchDataKHR is called at most once and never again, because atm this isn't the case in the specification and it would be legal use to call it multiple times.

Could even require the function to return CL_INVALID_VALUE or CL_INVALID_OPERATION when called more than once, so that applications won't be able to use this function either.

[Kerilk]

The wording for now is:

During initialization, after calling clIcdGetPlatformIDsKHR to query the available platforms, the
ICD Loader will, for each ICD 2 compatible platform, query dispatchable entry points using
clIcdGetFunctionAddressForPlatformKHR and then set the platform dispatch data using
clIcdSetPlatformDispatchDataKHR.

Any suggestion of what you would have liked to read?

Not a fan of painting myself in a corner for the deinitialization part of the extension by specifying an error code if users try to use this function themselves after initialization.

[karolherbst]

The wording for now is:

During initialization, after calling clIcdGetPlatformIDsKHR to query the available platforms, the
ICD Loader will, for each ICD 2 compatible platform, query dispatchable entry points using
clIcdGetFunctionAddressForPlatformKHR and then set the platform dispatch data using
clIcdSetPlatformDispatchDataKHR.

Any suggestion of what you would have like to read?

It's more about if the implementation has to assume that clIcdSetPlatformDispatchDataKHR can be called more than once. I think specifying an error code on a 2nd call to it would fix this problem and also prevent abuse of the interface. But might also be enough to simply state that the ICD loader calls it at most once here.

Not a fan of painting myself in a corner for the deinitialization part of the extension by specifying an error code if users try to use this function themselves after initialization.

I think it's better to explicitly fail it, because otherwise applications might just try to do weird things with it taking ICD loader specific implementation details into account. I hope they wouldn't, but the check is cheap, and there are 0 performance implications. This API could easily be abused by applications to mess with the internals of the ICD loader, and I'd rather they wouldn't be able to.

[karolherbst]

The wording for now is:

During initialization, after calling clIcdGetPlatformIDsKHR to query the available platforms, the
ICD Loader will, for each ICD 2 compatible platform, query dispatchable entry points using
clIcdGetFunctionAddressForPlatformKHR and then set the platform dispatch data using
clIcdSetPlatformDispatchDataKHR.

Any suggestion of what you would have like to read?

It's more about if the implementation has to assume that clIcdSetPlatformDispatchDataKHR can be called more than once. I think specifying an error code on a 2nd call to it would fix this problem and also prevent abuse of the interface. But might also be enough to simply state that the ICD loader calls it at most once here.

Not a fan of painting myself in a corner for the deinitialization part of the extension by specifying an error code if users try to use this function themselves after initialization.

I think it's better to explicitly fail it, because otherwise applications might just try to do weird things with it taking ICD loader specific implementation details into account. I hope they wouldn't, but the check is cheap, and there are 0 performance implications. This API could easily be abused by applications to mess with the internals of the ICD loader, and I'd rather they wouldn't be able to.

though the ICD loaders could also just hide all the ICD entry points from applications...

[Kerilk]

though the ICD loaders could also just hide all the ICD entry points from applications...

I was pondering this as well.

[Kerilk]

though the ICD loaders could also just hide all the ICD entry points from applications...

Implemented here: KhronosGroup/OpenCL-ICD-Loader#253

[Kerilk]

I think we'll need to revisit your idea of returning an error code in case of double initialization. The scenario I could see this happening (aside from the already mentioned re-initialization which would probably be fine) is if one or more loaders are statically linked into an application via shared libraries, or if an application dynamically linked with the loader is also linked with such a library. I know that we do not support statically linking the loader, but I am not sure we can ignore it either...

Maybe we should disable the ICD2 support in static builds.

Tagging @bashbaug since it could impact how we architect the deinitialization work, especially re-initializing a driver that we could not unload, either because it did not advertise support for unloading, or because it was kept loaded by some other mean.

[Kerilk]

@karolherbst please let me know if #1527 addresses your concerns about re-initialization.