Command buffers and thread safety

[bashbaug]

Creating a sub-issue based on #1309:

@joshqti asked:

clUpdateMutableCommandsKHR seems to be defined in a similar manner as clSetKernelArg. Is it not thread safe like clSetKernelArg? Clarification on how updates apply for simultaneous use required. If its not, we need to update Appendix A.

We had a long discussion in the February 18th teleconference. Some takeaways:

• We may want to use a different term than "thread safe". Vulkan uses the term "externally synchronized", with a precise definition, for example, see: https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#fundamentals-threadingbehavior
• Whatever we document, it should be in the main spec (normative), not in an appendix (informative).
• Any API that "sets" or "modifies" or "mutates" an OpenCL object is something that we should consider to be non-"thread safe" or "externally synchronized". There are some exceptions to this rule though, for example there are some APIs that modify program objects that are still thread safe, such as clBuildProgram.
• Most command buffer APIs would remain "thread safe", and only the APIs that modify command buffers would need to be non-"thread safe". Aside from clUpdateMutableCommandsKHR, one of the other command buffer APIs we discussed was clFinalizeCommandBufferKHR.
• Can we put information about thread safety into the XML file and auto-generate the right text into the spec?

We will continue discussion on this issue.

[bashbaug]

Regarding:

Can we put information about thread safety into the XML file and auto-generate the right text into the spec?

In the Vulkan XML file, the parameters that are "externally synchronized" are identified by externsync="true". For example, see the VkDeviceMemory memory parameter below:

        <command successcodes="VK_SUCCESS" errorcodes="VK_ERROR_OUT_OF_HOST_MEMORY,VK_ERROR_OUT_OF_DEVICE_MEMORY,VK_ERROR_MEMORY_MAP_FAILED">
            <proto><type>VkResult</type> <name>vkMapMemory</name></proto>
            <param><type>VkDevice</type> <name>device</name></param>
            <param externsync="true"><type>VkDeviceMemory</type> <name>memory</name></param>
            <param><type>VkDeviceSize</type> <name>offset</name></param>
            <param><type>VkDeviceSize</type> <name>size</name></param>
            <param optional="true"><type>VkMemoryMapFlags</type> <name>flags</name></param>
            <param optional="false,true"><type>void</type>** <name>ppData</name></param>
        </command>

[bashbaug]

Discussed in the March 18th teleconference:

• We will split the changes into two.
• The short-term change will simply add command buffer APIs to the existing appendix.
• The longer-term fix will move the description into the main spec and potentially adopt a more precise term like "externally synchronized" and record this information in the XML file.
• The APIs we will add to the appendix for the short-term fix are clFinalizeCommandBufferKHR and clUpdateMutableCommandsKHR.

[EwanC]

Now that the short term fix in #1345 is merged, should we create a separate issue for the long term fix and close this one?

Reducing the amount of open "cl_khr_command_buffer" labelled issues will help focus our attention on outstanding command-buffer issues for discussion, and the long term fix could be forgotten as a single bullet point inside this larger issue thread.