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

Goffredo Marocchi panajev at gmail.com
Wed Jun 4 18:22:06 EST 2008


I think the restrictions do appear sound and reasonable.

On Wed, Jun 4, 2008 at 9:46 AM, Kazunori Asayama <asayama at sm.sony.co.jp>
wrote:

> 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;
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/cbe-oss-dev/attachments/20080604/c69cf9d7/attachment.htm>


More information about the cbe-oss-dev mailing list