Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added test to verify flush on clReleaseCommandQueue with multiple queues #2134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shajder
Copy link
Contributor

@shajder shajder commented Oct 30, 2024

Fixes #2087 according to task description.

@shajder shajder changed the title Added a test to verify flush on clReleaseCommandQueue with multiple queues Added test to verify flush on clReleaseCommandQueue with multiple queues Oct 30, 2024
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpet can you check that the test description in #2087 is correct?

This test follows the test description exactly (thanks!), but I would like to ensure it is properly testing that the flush happsn on clReleaseCommandQueue and not somewhere else (e.g. the clFinish).

Comment on lines +109 to +112
bool success = poll_until(2000, 50, [&event_B]() {
cl_int status;
cl_int err = clGetEventInfo(event_B, CL_EVENT_COMMAND_EXECUTION_STATUS,
sizeof(cl_int), &status, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've called clFinish on queue_B here, the event_B must already be CL_COMPLETE.

@kpet
Copy link
Contributor

kpet commented Nov 26, 2024

@bashbaug I've reviewed the description and I think I still mostly agree with this. This won't prevent implementations from flushing queue A when clFinish is called on queue B (or elsewhere, they have the freedom) but, importantly, it should never deadlock because of work in queue B that depends on the unflushed work in queue A (because of the prior call to clReleaseCommandQueue on queue A.

Deadlocking is not a great failure mode for a test though so maybe we could call clFlush instead of clFinish on queue B and pool the event's execution status for the command that went to queue B instead.

The following single queue test could be useful to have too:

  • Enqueue work in a queue and get an event
  • Call clReleaseCommandQueue on that queue
  • Poll the execution status of the event, it should transition to CL_COMPLETE "soon".

@lakshmih
Copy link
Contributor

Agree that clFlush on queue B rather than clFinish is more appropriate to prevent the test from deadlocking when the flush on clReleaseCommandQueue is missing.

@bashbaug
Copy link
Contributor

bashbaug commented Dec 5, 2024

OK, to be clear, I don't think this test is wrong with either the explicit call to clFinish() (or an explicit call to clFlush() instead), but I also think it could very likely be testing behavior other than the implicit flush on clReleaseCommandQueue.

I think that the proposed single-queue test would be more useful:

  • Enqueue work in a queue and get an event
  • Call clReleaseCommandQueue on that queue
  • Poll the execution status of the event, it should transition to CL_COMPLETE "soon".

If we really wanted to test the letter of the spec, we should also call clRetainCommandQueue on the queue first, so the call to clReleaseCommandQueue decrements the reference count but doesn't cause the reference count to reach zero. This is the behavior we are considering changing - linking in KhronosGroup/OpenCL-Docs#1248 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a test to verify that clReleaseCommandQueue does trigger a flush in multi-queue scenario
4 participants