write a DEBUG macro and change the render_threads stuff to use it. Shuffle the mutexes in render_threads around again to fix yet another bug... seems to work, but meh
authorTero Marttila <terom@fixme.fi>
Tue, 17 Jun 2008 18:05:08 +0300
changeset 20 7512207c9041
parent 19 d18606bb6f20
child 21 e2916f8ebaa6
write a DEBUG macro and change the render_threads stuff to use it. Shuffle the mutexes in render_threads around again to fix yet another bug... seems to work, but meh

committer: Tero Marttila <terom@fixme.fi>
common.h
render_threads.c
--- a/common.h	Tue Jun 17 16:39:55 2008 +0300
+++ b/common.h	Tue Jun 17 18:05:08 2008 +0300
@@ -3,14 +3,25 @@
  * error handling
  */
 
-void _generic_err (const char *func, int perr, const char *fmt, ...)
+void _generic_err ( /*int level, */ const char *func, int perr, const char *fmt, ...)
         __attribute__ ((format (printf, 3, 4)));
 
 // needs to be defined as its own function for the noreturn attribute
-void _generic_err_exit (const char *func, int perr, const char *fmt, ...)
+void _generic_err_exit ( /* int level, */ const char *func, int perr, const char *fmt, ...)
         __attribute__ ((format (printf, 3, 4)))
         __attribute__ ((noreturn));
 
+/*
+enum _debug_level {
+    DEBUG_FATAL,
+    DEBUG_ERROR,
+    DEBUG_WARNING,
+    DEBUG_INFO,
+    DEBUG_DEBUG,
+};
+
+extern enum _debug_level _cur_debug_level;
+*/
 
 // various kinds of ways to handle an error, 2**3 of them, *g*
 #define error(...)                  _generic_err(       NULL, 0, __VA_ARGS__)
@@ -30,6 +41,12 @@
 #define WARNING(...) err_func(__func__, __VA_ARGS__)
 #define PWARNING(...) perr_func(__func__, __VA_ARGS__)
 
+#ifdef DEBUG_ENABLED
+#define DEBUG(...) err_func(__func__, __VA_ARGS__)
+#else
+#define DEBUG(...) if (0) { }
+#endif
+
 /*
  * Parse a host:port string.
  *
--- a/render_threads.c	Tue Jun 17 16:39:55 2008 +0300
+++ b/render_threads.c	Tue Jun 17 18:05:08 2008 +0300
@@ -3,6 +3,8 @@
 
 #include <pthread.h>
 
+#define DEBUG_ENABLED
+
 #include "common.h"
 #include "render_threads.h"
 #include "render_slices_struct.h"
@@ -135,8 +137,17 @@
     
     // used to protect slices_info
     pthread_mutex_t slices_mut;
+
+    /*
+     * Return value from the manager thread
+     */
+    enum render_threads_retval {
+        RETVAL_SUCCESS,
+        RETVAL_FAILURE,
+    } manager_retval;
 };
 
+
 void render_threads_deinit (struct render_threads *ctx) {
     render_slices_deinit(&ctx->slices_info);
 }
@@ -151,6 +162,9 @@
 int _render_threads_slice_row (void *arg, unsigned char *rowbuf) {
     struct render_thread *subctx = arg;
     int status, i;
+
+    // test for cancelation, we don't hold any locks right now
+    pthread_testcancel();
     
     // we need to handle the render_slices state atomically, so lock it
     pthread_mutex_lock(&subctx->self->slices_mut);
@@ -163,23 +177,15 @@
     status = render_slices_segment_done(&subctx->self->slices_info, subctx->slice_info->index);
     
     // debugging
-/*    subctx->segments_done++;
-    printf("slice %zu: segment_done, status=(%s %s), row=%d\n", subctx->slice_info->index,
+    DEBUG("slice %zu: segment_done, status=(%s %s), row=%d", subctx->slice_info->index,
         status & SLICE_PROCESS_ROW ? "SLICE_PROCESS_ROW" : "", 
         status & SLICE_CONTINUE ? "SLICE_CONTINUE" : "",
-        subctx->segments_done
+        ++subctx->segments_done
     );
-*/
-
-    // done with the slices stuff
-    pthread_mutex_unlock(&subctx->self->continue_mut);
-    pthread_mutex_unlock(&subctx->self->slices_mut);
     
     // do we need to notify the other worker threads?
     if (status & SLICE_CONTINUE) {
-        pthread_mutex_lock(&subctx->self->continue_mut);
-        
-//        printf("slice %zu: signal continue\n", subctx->slice_info->index);
+        DEBUG("slice %zu: signal continue", subctx->slice_info->index);
 
         // mark them as continueable
         for (i = 0; i < subctx->self->slice_count; i++)
@@ -187,10 +193,12 @@
 
         // signal then to continue
         pthread_cond_broadcast(&subctx->self->continue_cond);
+    }
 
-        pthread_mutex_unlock(&subctx->self->continue_mut);
-    }
-    
+    // done with the slices stuff
+    pthread_mutex_unlock(&subctx->self->continue_mut);
+    pthread_mutex_unlock(&subctx->self->slices_mut);
+   
     // tell the manager thread that it can process a row
     if (status & SLICE_PROCESS_ROW) {
         // grab the process_row lock so that we can do stuff with it
@@ -205,38 +213,43 @@
         // release the process_row mutex
         pthread_mutex_unlock(&subctx->self->process_row_mut);
     }
-    
-    // handle our can-continue status
-    pthread_mutex_lock(&subctx->self->continue_mut);
-
-    if (status & SLICE_CONTINUE) {
-        // don't wait for anything, we can just continue right away
-//        printf("slice %zu: direct continue\n", subctx->slice_info->index);
 
-    } else {
-        if (!subctx->can_continue) {
-            // we need to wait until someone signals it
-//            printf("slice %zu: waiting...\n", subctx->slice_info->index);
+#define BEGIN_MUTEX(mut) pthread_cleanup_push((void (*)(void *)) pthread_mutex_unlock, &(mut)); pthread_mutex_lock(&(mut));
+#define END_MUTEX pthread_cleanup_pop(1);
 
-            pthread_cond_wait(&subctx->self->continue_cond, &subctx->self->continue_mut);
-
-//            printf("slice %zu: continue after wait\n", subctx->slice_info->index);
+    // handle our can-continue status
+    // we need to release this lock if we get canceled in pthread_cond_wait
+    BEGIN_MUTEX(subctx->self->continue_mut)
+        if (status & SLICE_CONTINUE) {
+            // don't wait for anything, we can just continue right away
+            DEBUG("slice %zu: direct continue", subctx->slice_info->index);
 
         } else {
-            // no need to wait, because someone else took care of that before we got a chance to wait
-//            printf("slice %zu: indirect continue\n", subctx->slice_info->index);
-         
+            if (!subctx->can_continue) {
+                // we need to wait until someone signals it
+                DEBUG("slice %zu: waiting...", subctx->slice_info->index);
+
+                pthread_cond_wait(&subctx->self->continue_cond, &subctx->self->continue_mut);
+
+                DEBUG("slice %zu: continue after wait", subctx->slice_info->index);
+
+            } else {
+                // no need to wait, because someone else took care of that before we got a chance to wait
+                DEBUG("slice %zu: indirect continue", subctx->slice_info->index);
+             
+            }
         }
-    }
-    
-    // we should now be marked as continueable
-    assert(subctx->can_continue);
+        
+        // we should now be marked as continueable
+        assert(subctx->can_continue);
 
-    // proceed to continue, so clear this
-    subctx->can_continue = 0;
-    
-    // done handling the continue state
-    pthread_mutex_unlock(&subctx->self->continue_mut);
+        // proceed to continue, so clear this
+        subctx->can_continue = 0;
+        
+        // done handling the continue state
+        pthread_mutex_unlock(&subctx->self->continue_mut);
+
+    END_MUTEX
 
     // row handled succesfully
     return 0;
@@ -260,13 +273,13 @@
     pthread_mutex_unlock(&subctx->self->continue_mut);
 
     // start rendering...
-//    printf("slice %zu: start\n", subctx->slice_info->index);
+    DEBUG("slice %zu: start", subctx->slice_info->index);
 
     if (render_mandelbrot(subctx->slice_info->render_info))
         goto error;
 
     // success, slice render complete
-//    printf("slice %zu: done\n", subctx->slice_info->index);
+    DEBUG("slice %zu: done", subctx->slice_info->index);
     
     // sanity checks
     assert(subctx->self->slices_running > 0);
@@ -275,7 +288,7 @@
 
     if (--subctx->self->slices_running == 0) {
         // this was the last row, send the manager a final signal in case it's waiting
-//        printf("slice %zu: signal\n", subctx->slice_info->index);
+        DEBUG("slice %zu: signal", subctx->slice_info->index);
         pthread_cond_signal(&subctx->self->process_row_cond);
     }
 
@@ -307,12 +320,12 @@
     // then we wait around and call process_row when told to
     do {
         // the process_row must be locked here
-//        printf("manager: to wait\n");
+        DEBUG("manager: to wait");
         
         // figure out if we need to wait
         if (ctx->waiting_rows == 0 && ctx->slices_running > 0) {
             // no work to do right now, so wait for some
-//            printf("manager: waiting\n");
+            DEBUG("manager: waiting");
             
             // wait until a worker thread signals us
             pthread_cond_wait(&ctx->process_row_cond, &ctx->process_row_mut);
@@ -338,18 +351,14 @@
             // call into render_slices
             status = render_slices_process_row(&ctx->slices_info);
             
-/*            printf("manager: did process_row, status=(%s %s)\n", 
+            DEBUG("manager: did process_row, status=(%s %s)", 
                 status & SLICE_PROCESS_ROW ? "SLICE_PROCESS_ROW" : "", 
                 status & SLICE_CONTINUE ? "SLICE_CONTINUE" : ""
             );
-*/            
-            
-            // done with render_slices
-            pthread_mutex_unlock(&ctx->slices_mut);
-
-
+           
             // any errors?
             if (status == -1) {
+                pthread_mutex_unlock(&ctx->slices_mut);
                 goto error;
             }
 
@@ -358,11 +367,11 @@
                 // grab the continue_mut lock while we update this state
                 pthread_mutex_lock(&ctx->continue_mut);
 
-                // increment continue_count for all slices
+                // set can_continue for all slices
                 for (i = 0; i < ctx->slice_count; i++)
                     ctx->threads[i].can_continue = 1;
 
-//                printf("manager: signal continue\n");
+                DEBUG("manager: signal continue");
 
                 // signal any waiting worker threads to continue, giving them a chance to call cond_wait first.
                 pthread_cond_broadcast(&ctx->continue_cond);
@@ -370,6 +379,9 @@
                 // done with that
                 pthread_mutex_unlock(&ctx->continue_mut);
             }
+            
+            // done with render_slices
+            pthread_mutex_unlock(&ctx->slices_mut);
 
             // XXX: ignore SLICE_PROCESS_ROW
     
@@ -392,25 +404,47 @@
     if (render_slices_done(&ctx->slices_info))
         goto error;
 
-//    printf("manager: done\n");
+    DEBUG("manager: done");
     
-    // reap all the workers
     void *retval;
 
+    // reap all the workers
     for (i = 0; i < ctx->slice_count; i++) {
         if (pthread_join(ctx->threads[i].thread, &retval))
             PERROR("pthread_join");
         
-        // XXX: assume they don't get canceled (SIGSEV etc?)
+        // XXX: assume they don't get canceled
         assert(retval == NULL);
     }
-
-    // ok, exit ourself
-    pthread_exit(NULL);
+    
+    // succesfull return
+    ctx->manager_retval = RETVAL_SUCCESS;
+    
+    goto exit;
 
 error:
-    /* XXX: handle errors gracefully... */
-    FATAL("the threaded render operation failed, taking down the rest of the daemon application as well...");
+    /* cancel and join all worker threads */
+    WARNING("canceling and reaping all threads");
+
+    for (i = 0; i < ctx->slice_count; i++) {
+        if (pthread_cancel(ctx->threads[i].thread))
+            PWARNING("pthread_cancel(worker:%d)", i);
+        else {
+            if (pthread_join(ctx->threads[i].thread, &retval)) {
+                PWARNING("pthread_join(worker:%d)", i);
+            } else {
+                if (retval != PTHREAD_CANCELED)
+                    PWARNING("worker:%d retval = %p", i, retval);
+            }
+        }
+    }
+    
+    // failure
+    ctx->manager_retval = RETVAL_FAILURE;
+
+exit:    
+    // ok, exit ourself
+    pthread_exit(ctx);
 }
 
 int render_threads_init (struct render_threads *ctx, struct render *render_info) {
@@ -494,8 +528,13 @@
     
     if (retval == PTHREAD_CANCELED)
         ERROR("manager thread was canceled");
-    else if (retval != NULL)
+    else if (retval != ctx)
         ERROR("manager thread exited with unknown return value");
+    else {
+        if (ctx->manager_retval != RETVAL_SUCCESS) {
+            ERROR("manger thread exited with an error");
+        }
+    }
     
     // ok, success
     return 0;