[Cbe-oss-dev] [RFC/PATCH 1/2] libspe2: Disable spe_stop_info_read if no handler is registered

Kazunori Asayama asayama at sm.sony.co.jp
Wed Jun 4 17:46:23 EST 2008


This patch modifies the SPE event implementation of the libpse2 so
that the libspe2 doesn't write stop_info on the pipe if no stop event
handler is registered.

As a result of this change, it becomes impossible to get stop info by
spe_stop_info_read unless any stop event handlers are registered.

Signed-off-by: Kazunori Asayama <asayama at sm.sony.co.jp>

---
I'd like to ask opinions about the new restriction on usage of
spe_stop_info_read; that is, applications can't get stop info via
spe_stop_info_read without registration of stop event handler.

In my personal opinion, this restriction is acceptable to actual
applications because:

  a. If the thread which calls spe_context_run wants to get the stop
     info, it's available via spe_context_run. So spe_stop_info_read
     is unnecessary for the thread.

  b. If other threads want to get the stop info, the threads will call
     spe_stop_info_read, and it will be necessary to wait for stop
     events by spe_event_wait before spe_stop_info_read so that they
     can know whether stop info is available or not
     (spe_stop_info_read is non-blocking function).
     
     In addition to that, spe_stop_info_read is valid only when
     SPE_EVENTS_ENABLE is set to the SPE context. That implies that
     spe_stop_info_read should be used with event processing.

     I suppose that existing applications which use spe_stop_info_read
     follow this scenario.
---
 speevent/spe_event.c |  156 ++++++++++++++++++++++++++++++++++++++++++---------
 speevent/speevent.h  |    3 
 2 files changed, 132 insertions(+), 27 deletions(-)

Index: b/speevent/spe_event.c
===================================================================
--- a/speevent/spe_event.c	2008-06-04 14:57:49.000000000 +0900
+++ b/speevent/spe_event.c	2008-06-04 16:10:45.000000000 +0900
@@ -56,6 +56,74 @@ void _event_spe_context_unlock(spe_conte
   pthread_mutex_unlock(&__SPE_EVENT_CONTEXT_PRIV_GET(spe)->lock);
 }
 
+static void stop_event_lock(spe_context_event_priv_ptr_t evctx)
+{
+  pthread_mutex_lock(&evctx->stop_event_lock);
+}
+
+static void stop_event_unlock(spe_context_event_priv_ptr_t evctx)
+{
+  pthread_mutex_unlock(&evctx->stop_event_lock);
+}
+
+static int stop_event_pipe_close(spe_context_event_priv_ptr_t evctx)
+{
+  if (evctx->stop_event_pipe[0] != -1) {
+    close(evctx->stop_event_pipe[0]);
+    evctx->stop_event_pipe[0] = -1;
+  }
+  if (evctx->stop_event_pipe[1] != -1) {
+    close(evctx->stop_event_pipe[1]);
+    evctx->stop_event_pipe[1] = -1;
+  }
+
+  return 0;
+}
+
+static int stop_event_pipe_open(spe_context_event_priv_ptr_t evctx)
+{
+  int rc;
+
+  rc = pipe(evctx->stop_event_pipe);
+  if (rc == -1) {
+    return -1;
+  }
+  rc = fcntl(evctx->stop_event_pipe[0], F_GETFL);
+  if (rc != -1) {
+    rc = fcntl(evctx->stop_event_pipe[0], F_SETFL, rc | O_NONBLOCK);
+  }
+  if (rc == -1) {
+    stop_event_pipe_close(evctx);
+    return -1;
+  }
+
+  return 0;
+}
+
+static int stop_event_pipe_acquire(spe_context_event_priv_ptr_t evctx)
+{
+  if (evctx->stop_event_handler_count == 0) {
+    if (stop_event_pipe_open(evctx) == -1) {
+      return -1;
+    }
+  }
+
+  evctx->stop_event_handler_count++;
+
+  return 0;
+}
+
+static int stop_event_pipe_release(spe_context_event_priv_ptr_t evctx)
+{
+  evctx->stop_event_handler_count--;
+
+  if (evctx->stop_event_handler_count == 0) {
+    stop_event_pipe_close(evctx);
+  }
+
+  return 0;
+}
+
 int _event_spe_stop_info_read (spe_context_ptr_t spe, spe_stop_info_t *stopinfo)
 {
   spe_context_event_priv_ptr_t evctx;
@@ -64,13 +132,20 @@ int _event_spe_stop_info_read (spe_conte
   size_t total;
   
   evctx = __SPE_EVENT_CONTEXT_PRIV_GET(spe);
+
+  stop_event_lock(evctx); /* for atomic read */
+
   fd = evctx->stop_event_pipe[0];
 
-  pthread_mutex_lock(&evctx->stop_event_read_lock); /* for atomic read */
+  if (fd == -1) { /* no stop event handler */
+    stop_event_unlock(evctx);
+    errno = EAGAIN;
+    return -1;
+  }
 
   rc = read(fd, stopinfo, sizeof(*stopinfo));
   if (rc == -1) {
-    pthread_mutex_unlock(&evctx->stop_event_read_lock);
+    stop_event_unlock(evctx);
     return -1;
   }
 
@@ -98,7 +173,7 @@ int _event_spe_stop_info_read (spe_conte
     }
   }
 
-  pthread_mutex_unlock(&evctx->stop_event_read_lock);
+  stop_event_unlock(evctx);
 
   return rc == -1 ? -1 : 0;
 }
@@ -248,6 +323,15 @@ int _event_spe_event_handler_register(sp
   }
 
   if (event->events & SPE_EVENT_SPE_STOPPED) {
+    /* prevent reading stop info while registering */
+    stop_event_lock(evctx);
+
+    if (stop_event_pipe_acquire(evctx) == -1) {
+      stop_event_unlock(evctx);
+      _event_spe_context_unlock(event->spe);
+      return -1;
+    }
+
     fd = evctx->stop_event_pipe[0];
     
     ev_buf = &evctx->events[__SPE_EVENT_SPE_STOPPED];
@@ -257,9 +341,13 @@ int _event_spe_event_handler_register(sp
     ep_event.events = EPOLLIN;
     ep_event.data.ptr = ev_buf;
     if (epoll_ctl(epfd, ep_op, fd, &ep_event) == -1) {
+      stop_event_pipe_release(evctx);
+      stop_event_unlock(evctx);
       _event_spe_context_unlock(event->spe);
       return -1;
     }
+
+    stop_event_unlock(evctx);
   }
 
   _event_spe_context_unlock(event->spe);
@@ -340,12 +428,20 @@ int _event_spe_event_handler_deregister(
   }
   
   if (event->events & SPE_EVENT_SPE_STOPPED) {
+    /* prevent reading stop info while unregistering */
+    stop_event_lock(evctx);
+
     fd = evctx->stop_event_pipe[0];
     if (epoll_ctl(epfd, ep_op, fd, NULL) == -1) {
+      stop_event_unlock(evctx);
       _event_spe_context_unlock(event->spe);
       return -1;
     }
     evctx->events[__SPE_EVENT_SPE_STOPPED].events = 0;
+
+    stop_event_pipe_release(evctx);
+
+    stop_event_unlock(evctx);
   }
 
   _event_spe_context_unlock(event->spe);
@@ -424,12 +520,11 @@ int _event_spe_context_finalize(spe_cont
 
   evctx = __SPE_EVENT_CONTEXT_PRIV_GET(spe);
   __SPE_EVENT_CONTEXT_PRIV_SET(spe, NULL);
-  
-  close(evctx->stop_event_pipe[0]);
-  close(evctx->stop_event_pipe[1]);
+
+  stop_event_pipe_close(evctx);
 
   pthread_mutex_destroy(&evctx->lock);
-  pthread_mutex_destroy(&evctx->stop_event_read_lock);
+  pthread_mutex_destroy(&evctx->stop_event_lock);
 
   free(evctx);
 
@@ -439,7 +534,6 @@ int _event_spe_context_finalize(spe_cont
 struct spe_context_event_priv * _event_spe_context_initialize(spe_context_ptr_t spe)
 {
   spe_context_event_priv_ptr_t evctx;
-  int rc;
   int i;
 
   evctx = calloc(1, sizeof(*evctx));
@@ -447,29 +541,16 @@ struct spe_context_event_priv * _event_s
     return NULL;
   }
 
-  rc = pipe(evctx->stop_event_pipe);
-  if (rc == -1) {
-    free(evctx);
-    return NULL;
-  }
-  rc = fcntl(evctx->stop_event_pipe[0], F_GETFL);
-  if (rc != -1) {
-    rc = fcntl(evctx->stop_event_pipe[0], F_SETFL, rc | O_NONBLOCK);
-  }
-  if (rc == -1) {
-    close(evctx->stop_event_pipe[0]);
-    close(evctx->stop_event_pipe[1]);
-    free(evctx);
-    errno = EIO;
-    return NULL;
-  }
+  /* the pipe will be created when any stop event handler is registered */
+  evctx->stop_event_pipe[0] = -1;
+  evctx->stop_event_pipe[1] = -1;
 
   for (i = 0; i < sizeof(evctx->events) / sizeof(evctx->events[0]); i++) {
     evctx->events[i].spe = spe;
   }
 
   pthread_mutex_init(&evctx->lock, NULL);
-  pthread_mutex_init(&evctx->stop_event_read_lock, NULL);
+  pthread_mutex_init(&evctx->stop_event_lock, NULL);
 
   return evctx;
 }
@@ -479,17 +560,40 @@ int _event_spe_context_run	(spe_context_
   spe_context_event_priv_ptr_t evctx;
   spe_stop_info_t stopinfo_buf;
   int rc;
+  int errno_saved;
+  int fd;
 
   if (!stopinfo) {
     stopinfo = &stopinfo_buf;
   }
   rc = _base_spe_context_run(spe, entry, runflags, argp, envp, stopinfo);
+  errno_saved = errno;
 
   evctx = __SPE_EVENT_CONTEXT_PRIV_GET(spe);
-  if (write(evctx->stop_event_pipe[1], stopinfo, sizeof(*stopinfo)) != sizeof(*stopinfo)) {
+
+  stop_event_lock(evctx);
+
+  fd = evctx->stop_event_pipe[1];
+  /* don't write stop info if no stop event handler is registered */
+  if (fd == -1) {
+    stop_event_unlock(evctx);
+    return rc;
+  }
+
+  stop_event_pipe_acquire(evctx); /* to avoid closing the pipe after unlocked */
+  stop_event_unlock(evctx); /* unlock here to avoid deadlocks */
+
+  if (write(fd, stopinfo, sizeof(*stopinfo)) != sizeof(*stopinfo)) {
     /* error check. */
   }
 
+  /* release the pipe */
+  stop_event_lock(evctx);
+  stop_event_pipe_release(evctx);
+  stop_event_unlock(evctx);
+
+  errno = errno_saved;
+
   return rc;
 }
 
Index: b/speevent/speevent.h
===================================================================
--- a/speevent/speevent.h	2008-06-04 14:57:49.000000000 +0900
+++ b/speevent/speevent.h	2008-06-04 14:57:54.000000000 +0900
@@ -35,8 +35,9 @@ enum __spe_event_types {
 typedef struct spe_context_event_priv
 {
   pthread_mutex_t lock;
-  pthread_mutex_t stop_event_read_lock;
+  pthread_mutex_t stop_event_lock;
   int stop_event_pipe[2];
+  int stop_event_handler_count;
   spe_event_unit_t events[__NUM_SPE_EVENT_TYPES];
 } spe_context_event_priv_t, *spe_context_event_priv_ptr_t;
 



More information about the cbe-oss-dev mailing list