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

Dealing with blocking. #22

Closed
ghost opened this issue Nov 2, 2013 · 7 comments
Closed

Dealing with blocking. #22

ghost opened this issue Nov 2, 2013 · 7 comments

Comments

@ghost
Copy link

ghost commented Nov 2, 2013

I was doing a few experiments with running hundreds of tasks each enqueueing and waiting on kernels before sending data back to the main task. And this ran head first into an expected problem with light weight threads. You can't block them and expect things to work correctly.

OpenCL has one way of working around this, The use of a callbacks. If you can setup a callback to do a message send when an event completes there is no need to do a event wait.

This would look something like this:

extern fn trampoline(_: cl_event, _: cl_int, arg: *libc::c_void)
{
    println(format!("trampoline"));
    let f = unsafe {
        let f: ~Cell<&fn()> = cast::transmute(arg);
        f.take()
    };

    f();
}

impl Event {
    #[fixed_stack_segment] #[inline(never)]
    fn callback(&self, cn_type :cl_uint, f: extern "C" fn(cl_event, cl_int, *libc::c_void), arg: *libc::c_void)
    {
        unsafe {
            clSetEventCallback(self.event,
                               cn_type,
                               f,
                               arg);
        }
    }

    fn rwait(&self) {
        let (p, c) : (PortOne<()>, ChanOne<()>) = oneshot();
        let c = Cell::new(c);
        let f = ~Cell::new(|| {c.take().send(())});
        unsafe {
            self.callback(CL_COMPLETE, trampoline, cast::transmute(f));
        }
        p.recv();
    }
}

I would not be writing this as an issue if that worked. The rust scheduler relies on some thread local storage to be set for it to work. So as soon as the trampoline fires an abort() occurs.

OpenCL does not offer a callback to initialize it's threads, so I can't see an easy way to set this thread local storage.

There are probably a few alternatives that could work instead of using channels. Using a pipe could work, the callback writes a byte to it, and the waiting function uses the native rust runtime read on the pipe. That sounds like an extremely round about and slow mechanise.

@eholk
Copy link
Collaborator

eholk commented Nov 4, 2013

If you can make this work with pipes, that's the way to go. The last time I measured, the overhead of sending pipe messages was very small, and this is the desired mechanism in Rust for synchronizing tasks.

Along these lines, it seems like we should be careful to make sure the Rust OpenCL context only runs from one task or thread. We make this happen now by using RUST_THREADS=1, but this is less than ideal. Unfortunately, there isn't a good way yet of making sure there is only one provider of a service in Rust.

@ghost
Copy link
Author

ghost commented Nov 4, 2013

I think the RUST_THREADS=1 is mostly caused by the clGetPlatforms api failing. From my understanding of Appendix A.2, it is safe to share the context pointer. Buffers and kernels are obviously not thread safe. But I'm surprised that the command queue is also considered unsafe.

I think the best route might be to just use ownership via ~. With how the API is designed right now it would just be a matter of not implementing the Clone trait to prevent more then one task from accessing it.

@eholk
Copy link
Collaborator

eholk commented Nov 5, 2013

One difficulty is still making sure only one task tries to create an OpenCL
object. I guess we could use an atomic option to say whether a context
already exists and just fail otherwise. The clients could take care of
sharing it by message passing if they need to.

On Mon, Nov 4, 2013 at 6:05 PM, Colin Sherratt notifications@github.comwrote:

I think the RUST_THREADS=1 is mostly caused by the clGetPlatforms api
failing. From my understanding of Appendix A.2, it is safe to share the
context pointer. Buffers and kernels are obviously not thread safe. But I'm
surprised that the command queue is also considered unsafe.

I think the best route might be to just use ownership via ~. With how the
API is designed right now it would just be a matter of not implementing
the Clone trait to prevent more then one task from accessing it.


Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-27732317
.

@ghost
Copy link
Author

ghost commented Nov 10, 2013

I played with adding a spinlock to work around this issue earlier this week. https://github.com/csherratt/rust-opencl/commit/a6258024bf76ca6001db73d52d971515e736bb1c

I'd rather not push these changes up because the fix is pretty ugly. If rust-lang/rust#9105 is resolved I think this could work.

@ghost
Copy link
Author

ghost commented Jan 14, 2014

With the new libgreen/libnative split and the fact that #9105 is complete it should be possible to resolve this issue with a little work. I might investigate this later this week. It'd be nice to remove the RUST_THREADS=1 for the make check.

@eholk
Copy link
Collaborator

eholk commented Dec 26, 2014

Is this issue still active? I notice we have most things protected by mutexes, and cargo test works just fine for me without RUST_THREADS=1.

@ghost
Copy link
Author

ghost commented Dec 27, 2014

This is actually no longer relevant. Rust no longer supports light weight threads so what opencl does by default is the correct behavior.

@ghost ghost closed this as completed Dec 27, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant