RE: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible

[Felipe Contreras <felipe.contreras@nokia.com>]

fernando.lugo@ti.com wrote:
> > fernando.lugo@ti.com wrote:
> > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo 
> > > > <x0095840@ti.com> wrote:
> > > > > So that avoid non-killable process.
> > > > 
> > > > It would be useful to interrupt these tasks from user-space. 
> > > > A separate ioctl to do that would be needed.
> > > 
> > > I don't see use case where that could be needed. It is only 
> > To avoid a 
> > > nonkillable task in the case the user pass an infinite Timeout.
> > > 
> > > If you have some test case where that ioctl would be needed Please 
> > > share it in order to find the best solution.
> > 
> > Well, imagine the application is using a library to access 
> > the DSP, and the library has a dedicated thread listening for 
> > DSP events in a loop.
> > This happens to be how libomxil-ti and gst-dsp work.
> > 
> > Now, the thread received the last message, but has set a 
> > timeout of 10s, or even worst, no timeout at all.
> > 
> > After realizing that was the last message, the main thread 
> > decides to shut down, but it has to wait for the DSP thread 
> > to join. Unfortunately the DSP thread is stuck waiting for 
> > events, and there's nothing that can be done.
> > 
> > However, if we have a separate ioctl to interrupt that task, 
> > then the main thread can issue that ioctl, and unlock the DSP 
> > thread without having to wait 10s, or forever.
> > 
> > Does that make sense?
> 
> Maybe sending a signal to yourselft and having a dummy signal
> Handle should work, it that would not like good.

Signals on libraries is a no-no.

> I am thinking On having a ioctl to create and set an event the you
> could Something like this:
> 
> struct dsp_notification events[3];
> 
> proc_register_notify(proc, event_type, &events[0]);
> ...
> proc_register_notify(proc, event_type, &events[1]);
> ...
> Sync_open_event(&events[2]);
> 
> 
> second thread:
> 
> 	mgr_wait_for_bridge_events(proc, events, 3, index);
> 
> 	if (index == 2) 
> 		/* main thread force exit */
> 
> 
> Main thread:
> 
> 	/* if some execption happened then finish the second thread */
> 	sync_set_event(events[2]);
> 
> 	pthread_join(...);
> 
> 
> However it is in progess a task for change replacing dspbridge sync.c
> Module with event_fd to signal events to userspace. Where now simple
> File descriptor will be used as event elements. So the mgr_wait_for_bridge_events
> Will be implemented using "select" system call inside to wait for multiple events.
> So you will be able to do something like this:
> 
> int events[3];
> 
> proc_register_notify(proc, event_type, &events[0]);
> ...
> proc_register_notify(proc, event_type, &events[1]);
> ...
> events[2] = eventfd(0, 0);
> 
> 
> second thread:
> 
> 	mgr_wait_for_bridge_events(proc, events, 3, index);
> 
> 	if (index == 2) 
> 		/* main thread force exit */
> 
> Main thread:
> 
> 	/* if some execption happened then finish the second thread */
> 	write(events[2], "s", 1);
> 
> 	pthread_join(...);
> 
> You won't need any aditional ioctl in order to do what you want to do.
> 
> So, I think it is not worth to make much changes to some module that will
> Dissapear (my patch is just a fix it is not implementing something new),
> It is just a matter of time to that task is finished and tested properly
> And then send to LO.

All right, that makes sense. I just wanted to make suere you are aware
of that need.

-- 
Felipe Contreras