[OpenCL] Do not try to release kernel without check whether it was created

[victoryforce]

@jenshannoschwalm - I’ve never touched OpenCL-related code before, whereas you speak OpenCL fluently :). Could you please review if this fix makes sense?

As I understand it, creating a kernel can fail (BTW, is that something that actually happens in practice, or is it more of a theoretical possibility?). In this case, without a check, we might attempt to release a kernel handle that is actually invalid (zero/garbage/stale, but not a genuinely created object). This could lead to undefined behavior (UB) and, likely, a crash...

[jenshannoschwalm]

I see your point.

I just checked opencl specs clReleaseProgram which includes a reference count. We keep track in the flag if that kernel has been compiled correctly and might not want to reset the flag to false at all. Not sure ...

[jenshannoschwalm]

I checked it again, we should indeed

1. not set the flag to FALSE here
2. test the flag before releasing the kernel

as correctly done in _cleanup_cl_device_context(). Otherwise we possibly could leave kernels in the cl memory!

[jenshannoschwalm]

@victoryforce will you modify your pr or would you prefer that i take over?

[victoryforce]

will you modify your pr or would you prefer that i take over?

@jenshannoschwalm - Sorry, I kept putting it off until I could find the time to sit down and get a better grasp of how things work here... but real life kept getting in the way.

Your feedback was clear; I just wanted to figure it out for myself rather than simply implementing your advice mechanically.

I finally read the man page for clReleaseProgram (which I should have done before the PR) and saw that nothing bad would have happened even if it were called with an invalid argument. It would simply have returned CL_INVALID_PROGRAM. But if we can check whether the function should be called, then naturally, performing that check is the right approach.

Changed (removed the flag reset), pushed.

[jenshannoschwalm]

Maybe even report such an error in logs, we then could detect issues.

[victoryforce]

Maybe even report such an error in logs, we then could detect issues.

I’ll leave it at that; if you want to add reporting, you could do it here or in your PR. I’m just not sure what to write in the message, or if it even makes sense to add a log reporting right here. Essentially, if we try to release a kernel where kernel_used is FALSE, it means that kernel wasn't successfully created by the clCreateKernel call in the _check_kernel function. And there’s already log reporting there.

[jamalkamaladdin]

One measured note below.

[jamalkamaladdin]

dt_opencl_cleanup calls the ten *_free_cl_global functions at opencl.c:1628-1637 and then _cleanup_cl_device_context at opencl.c:1640. Those ten reach dt_opencl_free_kernel, which now leaves kernel_used set, and the guard at opencl.c:1184 still passes on the same index, giving a second clReleaseKernel on the same handle.

At this head sha kernel_used is cleared in two places: the memset at opencl.c:503 and the clCreateKernel failure path at opencl.c:2654. Before this diff the third site was this function.

Suggested change

	if(cl->dev[dev].kernel_used[kernel])
	(cl->dlocl->symbols->dt_clReleaseKernel)(cl->dev[dev].kernel[kernel]);
	if(cl->dev[dev].kernel_used[kernel])
	{
	cl->dev[dev].kernel_used[kernel] = FALSE;
	(cl->dlocl->symbols->dt_clReleaseKernel)(cl->dev[dev].kernel[kernel]);
	}

[jenshannoschwalm]

I don't understand this comment.

[jamalkamaladdin]

patch drops kernel_used[kernel] = FALSE. That line stopped a double release during cleanup.

dt_opencl_cleanup calls the *_free_cl_global functions at opencl.c:1628. Those reach dt_opencl_free_kernel. It releases the kernel and no longer clears the flag. At opencl.c:1640
dt_opencl_cleanup calls _cleanup_cl_device_context. That one releases every kernel whose flag is still set, opencl.c:1184. Same handle, second clReleaseKernel.

Clearing the flag inside your new check fixes the crash and keeps one release:

Suggested change

	if(cl->dev[dev].kernel_used[kernel])
	(cl->dlocl->symbols->dt_clReleaseKernel)(cl->dev[dev].kernel[kernel]);
	if(cl->dev[dev].kernel_used[kernel])
	{
	cl->dev[dev].kernel_used[kernel] = FALSE;
	(cl->dlocl->symbols->dt_clReleaseKernel)(cl->dev[dev].kernel[kernel]);
	}