[Cbe-oss-dev] [RFC/PATCH 1/2] libspe2: Disable spe_stop_info_read if no handler is registered
D. Herrendoerfer
d.herrendoerfer at herrendoerfer.name
Fri Jun 6 00:51:03 EST 2008
I think the change in essence is ok, but it modifies the
spe_stop_info_read() significantly and will make existing code fail.
For example:
Applications that already call spe_stop_info_read() to keep the
pipe from stalling must now stop doing so.
The Documentation states that this must be done.
So I believe this is an API modification.
If we decide to do this we need to increase the version number and
make sure this is documented apropriately.
Best regards,
D. Herrendoerfer
On Wed, 2008-06-04 at 16:46 +0900, Kazunori Asayama 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;
>
More information about the cbe-oss-dev
mailing list