I think the restrictions do appear sound and reasonable.<br><br><div class="gmail_quote">On Wed, Jun 4, 2008 at 9:46 AM, Kazunori Asayama <<a href="mailto:asayama@sm.sony.co.jp">asayama@sm.sony.co.jp</a>> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
This patch modifies the SPE event implementation of the libpse2 so<br>
that the libspe2 doesn't write stop_info on the pipe if no stop event<br>
handler is registered.<br>
<br>
As a result of this change, it becomes impossible to get stop info by<br>
spe_stop_info_read unless any stop event handlers are registered.<br>
<br>
Signed-off-by: Kazunori Asayama <<a href="mailto:asayama@sm.sony.co.jp" target="_blank">asayama@sm.sony.co.jp</a>><br>
<br>
---<br>
I'd like to ask opinions about the new restriction on usage of<br>
spe_stop_info_read; that is, applications can't get stop info via<br>
spe_stop_info_read without registration of stop event handler.<br>
<br>
In my personal opinion, this restriction is acceptable to actual<br>
applications because:<br>
<br>
a. If the thread which calls spe_context_run wants to get the stop<br>
info, it's available via spe_context_run. So spe_stop_info_read<br>
is unnecessary for the thread.<br>
<br>
b. If other threads want to get the stop info, the threads will call<br>
spe_stop_info_read, and it will be necessary to wait for stop<br>
events by spe_event_wait before spe_stop_info_read so that they<br>
can know whether stop info is available or not<br>
(spe_stop_info_read is non-blocking function).<br>
In addition to that, spe_stop_info_read is valid only when<br>
SPE_EVENTS_ENABLE is set to the SPE context. That implies that<br>
spe_stop_info_read should be used with event processing.<br>
<br>
I suppose that existing applications which use spe_stop_info_read<br>
follow this scenario.<br>
---<br>
speevent/spe_event.c | 156 ++++++++++++++++++++++++++++++++++++++++++---------<br>
speevent/speevent.h | 3 2 files changed, 132 insertions(+), 27 deletions(-)<br>
<br>
Index: b/speevent/spe_event.c<br>
===================================================================<br>
--- a/speevent/spe_event.c 2008-06-04 14:57:49.000000000 +0900<br>
+++ b/speevent/spe_event.c 2008-06-04 16:10:45.000000000 +0900<br>
@@ -56,6 +56,74 @@ void _event_spe_context_unlock(spe_conte<br>
pthread_mutex_unlock(&__SPE_EVENT_CONTEXT_PRIV_GET(spe)->lock);<br>
}<br>
<br>
+static void stop_event_lock(spe_context_event_priv_ptr_t evctx)<br>
+{<br>
+ pthread_mutex_lock(&evctx->stop_event_lock);<br>
+}<br>
+<br>
+static void stop_event_unlock(spe_context_event_priv_ptr_t evctx)<br>
+{<br>
+ pthread_mutex_unlock(&evctx->stop_event_lock);<br>
+}<br>
+<br>
+static int stop_event_pipe_close(spe_context_event_priv_ptr_t evctx)<br>
+{<br>
+ if (evctx->stop_event_pipe[0] != -1) {<br>
+ close(evctx->stop_event_pipe[0]);<br>
+ evctx->stop_event_pipe[0] = -1;<br>
+ }<br>
+ if (evctx->stop_event_pipe[1] != -1) {<br>
+ close(evctx->stop_event_pipe[1]);<br>
+ evctx->stop_event_pipe[1] = -1;<br>
+ }<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
+static int stop_event_pipe_open(spe_context_event_priv_ptr_t evctx)<br>
+{<br>
+ int rc;<br>
+<br>
+ rc = pipe(evctx->stop_event_pipe);<br>
+ if (rc == -1) {<br>
+ return -1;<br>
+ }<br>
+ rc = fcntl(evctx->stop_event_pipe[0], F_GETFL);<br>
+ if (rc != -1) {<br>
+ rc = fcntl(evctx->stop_event_pipe[0], F_SETFL, rc | O_NONBLOCK);<br>
+ }<br>
+ if (rc == -1) {<br>
+ stop_event_pipe_close(evctx);<br>
+ return -1;<br>
+ }<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
+static int stop_event_pipe_acquire(spe_context_event_priv_ptr_t evctx)<br>
+{<br>
+ if (evctx->stop_event_handler_count == 0) {<br>
+ if (stop_event_pipe_open(evctx) == -1) {<br>
+ return -1;<br>
+ }<br>
+ }<br>
+<br>
+ evctx->stop_event_handler_count++;<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
+static int stop_event_pipe_release(spe_context_event_priv_ptr_t evctx)<br>
+{<br>
+ evctx->stop_event_handler_count--;<br>
+<br>
+ if (evctx->stop_event_handler_count == 0) {<br>
+ stop_event_pipe_close(evctx);<br>
+ }<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
int _event_spe_stop_info_read (spe_context_ptr_t spe, spe_stop_info_t *stopinfo)<br>
{<br>
spe_context_event_priv_ptr_t evctx;<br>
@@ -64,13 +132,20 @@ int _event_spe_stop_info_read (spe_conte<br>
size_t total;<br>
evctx = __SPE_EVENT_CONTEXT_PRIV_GET(spe);<br>
+<br>
+ stop_event_lock(evctx); /* for atomic read */<br>
+<br>
fd = evctx->stop_event_pipe[0];<br>
<br>
- pthread_mutex_lock(&evctx->stop_event_read_lock); /* for atomic read */<br>
+ if (fd == -1) { /* no stop event handler */<br>
+ stop_event_unlock(evctx);<br>
+ errno = EAGAIN;<br>
+ return -1;<br>
+ }<br>
<br>
rc = read(fd, stopinfo, sizeof(*stopinfo));<br>
if (rc == -1) {<br>
- pthread_mutex_unlock(&evctx->stop_event_read_lock);<br>
+ stop_event_unlock(evctx);<br>
return -1;<br>
}<br>
<br>
@@ -98,7 +173,7 @@ int _event_spe_stop_info_read (spe_conte<br>
}<br>
}<br>
<br>
- pthread_mutex_unlock(&evctx->stop_event_read_lock);<br>
+ stop_event_unlock(evctx);<br>
<br>
return rc == -1 ? -1 : 0;<br>
}<br>
@@ -248,6 +323,15 @@ int _event_spe_event_handler_register(sp<br>
}<br>
<br>
if (event->events & SPE_EVENT_SPE_STOPPED) {<br>
+ /* prevent reading stop info while registering */<br>
+ stop_event_lock(evctx);<br>
+<br>
+ if (stop_event_pipe_acquire(evctx) == -1) {<br>
+ stop_event_unlock(evctx);<br>
+ _event_spe_context_unlock(event->spe);<br>
+ return -1;<br>
+ }<br>
+<br>
fd = evctx->stop_event_pipe[0];<br>
ev_buf = &evctx->events[__SPE_EVENT_SPE_STOPPED];<br>
@@ -257,9 +341,13 @@ int _event_spe_event_handler_register(sp<br>
ep_event.events = EPOLLIN;<br>
ep_event.data.ptr = ev_buf;<br>
if (epoll_ctl(epfd, ep_op, fd, &ep_event) == -1) {<br>
+ stop_event_pipe_release(evctx);<br>
+ stop_event_unlock(evctx);<br>
_event_spe_context_unlock(event->spe);<br>
return -1;<br>
}<br>
+<br>
+ stop_event_unlock(evctx);<br>
}<br>
<br>
_event_spe_context_unlock(event->spe);<br>
@@ -340,12 +428,20 @@ int _event_spe_event_handler_deregister(<br>
}<br>
if (event->events & SPE_EVENT_SPE_STOPPED) {<br>
+ /* prevent reading stop info while unregistering */<br>
+ stop_event_lock(evctx);<br>
+<br>
fd = evctx->stop_event_pipe[0];<br>
if (epoll_ctl(epfd, ep_op, fd, NULL) == -1) {<br>
+ stop_event_unlock(evctx);<br>
_event_spe_context_unlock(event->spe);<br>
return -1;<br>
}<br>
evctx->events[__SPE_EVENT_SPE_STOPPED].events = 0;<br>
+<br>
+ stop_event_pipe_release(evctx);<br>
+<br>
+ stop_event_unlock(evctx);<br>
}<br>
<br>
_event_spe_context_unlock(event->spe);<br>
@@ -424,12 +520,11 @@ int _event_spe_context_finalize(spe_cont<br>
<br>
evctx = __SPE_EVENT_CONTEXT_PRIV_GET(spe);<br>
__SPE_EVENT_CONTEXT_PRIV_SET(spe, NULL);<br>
- - close(evctx->stop_event_pipe[0]);<br>
- close(evctx->stop_event_pipe[1]);<br>
+<br>
+ stop_event_pipe_close(evctx);<br>
<br>
pthread_mutex_destroy(&evctx->lock);<br>
- pthread_mutex_destroy(&evctx->stop_event_read_lock);<br>
+ pthread_mutex_destroy(&evctx->stop_event_lock);<br>
<br>
free(evctx);<br>
<br>
@@ -439,7 +534,6 @@ int _event_spe_context_finalize(spe_cont<br>
struct spe_context_event_priv * _event_spe_context_initialize(spe_context_ptr_t spe)<br>
{<br>
spe_context_event_priv_ptr_t evctx;<br>
- int rc;<br>
int i;<br>
<br>
evctx = calloc(1, sizeof(*evctx));<br>
@@ -447,29 +541,16 @@ struct spe_context_event_priv * _event_s<br>
return NULL;<br>
}<br>
<br>
- rc = pipe(evctx->stop_event_pipe);<br>
- if (rc == -1) {<br>
- free(evctx);<br>
- return NULL;<br>
- }<br>
- rc = fcntl(evctx->stop_event_pipe[0], F_GETFL);<br>
- if (rc != -1) {<br>
- rc = fcntl(evctx->stop_event_pipe[0], F_SETFL, rc | O_NONBLOCK);<br>
- }<br>
- if (rc == -1) {<br>
- close(evctx->stop_event_pipe[0]);<br>
- close(evctx->stop_event_pipe[1]);<br>
- free(evctx);<br>
- errno = EIO;<br>
- return NULL;<br>
- }<br>
+ /* the pipe will be created when any stop event handler is registered */<br>
+ evctx->stop_event_pipe[0] = -1;<br>
+ evctx->stop_event_pipe[1] = -1;<br>
<br>
for (i = 0; i < sizeof(evctx->events) / sizeof(evctx->events[0]); i++) {<br>
evctx->events[i].spe = spe;<br>
}<br>
<br>
pthread_mutex_init(&evctx->lock, NULL);<br>
- pthread_mutex_init(&evctx->stop_event_read_lock, NULL);<br>
+ pthread_mutex_init(&evctx->stop_event_lock, NULL);<br>
<br>
return evctx;<br>
}<br>
@@ -479,17 +560,40 @@ int _event_spe_context_run (spe_context_<br>
spe_context_event_priv_ptr_t evctx;<br>
spe_stop_info_t stopinfo_buf;<br>
int rc;<br>
+ int errno_saved;<br>
+ int fd;<br>
<br>
if (!stopinfo) {<br>
stopinfo = &stopinfo_buf;<br>
}<br>
rc = _base_spe_context_run(spe, entry, runflags, argp, envp, stopinfo);<br>
+ errno_saved = errno;<br>
<br>
evctx = __SPE_EVENT_CONTEXT_PRIV_GET(spe);<br>
- if (write(evctx->stop_event_pipe[1], stopinfo, sizeof(*stopinfo)) != sizeof(*stopinfo)) {<br>
+<br>
+ stop_event_lock(evctx);<br>
+<br>
+ fd = evctx->stop_event_pipe[1];<br>
+ /* don't write stop info if no stop event handler is registered */<br>
+ if (fd == -1) {<br>
+ stop_event_unlock(evctx);<br>
+ return rc;<br>
+ }<br>
+<br>
+ stop_event_pipe_acquire(evctx); /* to avoid closing the pipe after unlocked */<br>
+ stop_event_unlock(evctx); /* unlock here to avoid deadlocks */<br>
+<br>
+ if (write(fd, stopinfo, sizeof(*stopinfo)) != sizeof(*stopinfo)) {<br>
/* error check. */<br>
}<br>
<br>
+ /* release the pipe */<br>
+ stop_event_lock(evctx);<br>
+ stop_event_pipe_release(evctx);<br>
+ stop_event_unlock(evctx);<br>
+<br>
+ errno = errno_saved;<br>
+<br>
return rc;<br>
}<br>
<br>
Index: b/speevent/speevent.h<br>
===================================================================<br>
--- a/speevent/speevent.h 2008-06-04 14:57:49.000000000 +0900<br>
+++ b/speevent/speevent.h 2008-06-04 14:57:54.000000000 +0900<br>
@@ -35,8 +35,9 @@ enum __spe_event_types {<br>
typedef struct spe_context_event_priv<br>
{<br>
pthread_mutex_t lock;<br>
- pthread_mutex_t stop_event_read_lock;<br>
+ pthread_mutex_t stop_event_lock;<br>
int stop_event_pipe[2];<br>
+ int stop_event_handler_count;<br>
spe_event_unit_t events[__NUM_SPE_EVENT_TYPES];<br>
} spe_context_event_priv_t, *spe_context_event_priv_ptr_t;<br>
<br>
</blockquote></div><br>