From 539fdc49f1ef02f5a40c486fa940b3bfacb44a48 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Nicolai=20H=C3=A4hnle?= Date: Mon, 28 May 2018 17:30:25 +0200 Subject: [PATCH] ddebug: simplify watchdog loop and fix crash in the no-timeout case MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The following race condition could occur in the no-timeout case: API thread Gallium thread Watchdog ---------- -------------- -------- dd_before_draw u_threaded_context draw dd_after_draw add to dctx->records signal watchdog dump & destroy record execute draw dd_after_draw_async use-after-free! Alternatively, the same scenario would assert in a debug build when destroying the record because record->driver_finished has not signaled. Fix this and simplify the logic at the same time by - handing the record pointers off to the watchdog thread *before* each draw call and - waiting on the driver_finished fence in the watchdog thread Reviewed-by: Marek Olšák --- .../auxiliary/driver_ddebug/dd_context.c | 1 - src/gallium/auxiliary/driver_ddebug/dd_draw.c | 103 ++++++++---------- src/gallium/auxiliary/driver_ddebug/dd_pipe.h | 21 ++-- 3 files changed, 52 insertions(+), 73 deletions(-) diff --git a/src/gallium/auxiliary/driver_ddebug/dd_context.c b/src/gallium/auxiliary/driver_ddebug/dd_context.c index a9ac6ef14ab..15efeccf879 100644 --- a/src/gallium/auxiliary/driver_ddebug/dd_context.c +++ b/src/gallium/auxiliary/driver_ddebug/dd_context.c @@ -596,7 +596,6 @@ dd_context_destroy(struct pipe_context *_pipe) cnd_destroy(&dctx->cond); assert(list_empty(&dctx->records)); - assert(!dctx->record_pending); if (pipe->set_log_context) { pipe->set_log_context(pipe, NULL); diff --git a/src/gallium/auxiliary/driver_ddebug/dd_draw.c b/src/gallium/auxiliary/driver_ddebug/dd_draw.c index cb5db8ab83b..a930299ebb7 100644 --- a/src/gallium/auxiliary/driver_ddebug/dd_draw.c +++ b/src/gallium/auxiliary/driver_ddebug/dd_draw.c @@ -988,10 +988,8 @@ dd_report_hang(struct dd_context *dctx) encountered_hang = true; } - if (num_later || dctx->record_pending) { - fprintf(stderr, "... and %u%s additional draws.\n", num_later, - dctx->record_pending ? "+1 (pending)" : ""); - } + if (num_later) + fprintf(stderr, "... and %u additional draws.\n", num_later); fprintf(stderr, "\nDone.\n"); dd_kill_process(); @@ -1008,9 +1006,6 @@ dd_thread_main(void *input) for (;;) { struct list_head records; - struct pipe_fence_handle *fence; - struct pipe_fence_handle *fence2 = NULL; - list_replace(&dctx->records, &records); list_inithead(&dctx->records); dctx->num_records = 0; @@ -1018,36 +1013,36 @@ dd_thread_main(void *input) if (dctx->api_stalled) cnd_signal(&dctx->cond); - if (!list_empty(&records)) { - /* Wait for the youngest draw. This means hangs can take a bit longer - * to detect, but it's more efficient this way. */ - struct dd_draw_record *youngest = - LIST_ENTRY(struct dd_draw_record, records.prev, list); - fence = youngest->bottom_of_pipe; - } else if (dctx->record_pending) { - /* Wait for pending fences, in case the driver ends up hanging internally. */ - fence = dctx->record_pending->prev_bottom_of_pipe; - fence2 = dctx->record_pending->top_of_pipe; - } else if (dctx->kill_thread) { - break; - } else { + if (list_empty(&records)) { + if (dctx->kill_thread) + break; + cnd_wait(&dctx->cond, &dctx->mutex); continue; } + mtx_unlock(&dctx->mutex); - /* Fences can be NULL legitimately when timeout detection is disabled. */ - if ((fence && - !screen->fence_finish(screen, NULL, fence, - (uint64_t)dscreen->timeout_ms * 1000*1000)) || - (fence2 && - !screen->fence_finish(screen, NULL, fence2, - (uint64_t)dscreen->timeout_ms * 1000*1000))) { - mtx_lock(&dctx->mutex); - list_splice(&records, &dctx->records); - dd_report_hang(dctx); - /* we won't actually get here */ - mtx_unlock(&dctx->mutex); + /* Wait for the youngest draw. This means hangs can take a bit longer + * to detect, but it's more efficient this way. */ + struct dd_draw_record *youngest = + list_last_entry(&records, struct dd_draw_record, list); + + if (dscreen->timeout_ms > 0) { + uint64_t abs_timeout = os_time_get_absolute_timeout( + (uint64_t)dscreen->timeout_ms * 1000*1000); + + if (!util_queue_fence_wait_timeout(&youngest->driver_finished, abs_timeout) || + !screen->fence_finish(screen, NULL, youngest->bottom_of_pipe, + (uint64_t)dscreen->timeout_ms * 1000*1000)) { + mtx_lock(&dctx->mutex); + list_splice(&records, &dctx->records); + dd_report_hang(dctx); + /* we won't actually get here */ + mtx_unlock(&dctx->mutex); + } + } else { + util_queue_fence_wait(&youngest->driver_finished); } list_for_each_entry_safe(struct dd_draw_record, record, &records, list) { @@ -1079,6 +1074,7 @@ dd_create_record(struct dd_context *dctx) record->bottom_of_pipe = NULL; record->log_page = NULL; util_queue_fence_init(&record->driver_finished); + util_queue_fence_reset(&record->driver_finished); dd_init_copy_of_draw_state(&record->draw_state); dd_copy_draw_state(&record->draw_state.base, &dctx->draw_state); @@ -1115,13 +1111,23 @@ dd_before_draw(struct dd_context *dctx, struct dd_draw_record *record) pipe->flush(pipe, &record->top_of_pipe, PIPE_FLUSH_DEFERRED | PIPE_FLUSH_TOP_OF_PIPE); } + } - mtx_lock(&dctx->mutex); - dctx->record_pending = record; - if (list_empty(&dctx->records)) - cnd_signal(&dctx->cond); - mtx_unlock(&dctx->mutex); + mtx_lock(&dctx->mutex); + if (unlikely(dctx->num_records > 10000)) { + dctx->api_stalled = true; + /* Since this is only a heuristic to prevent the API thread from getting + * too far ahead, we don't need a loop here. */ + cnd_wait(&dctx->cond, &dctx->mutex); + dctx->api_stalled = false; } + + if (list_empty(&dctx->records)) + cnd_signal(&dctx->cond); + + list_addtail(&record->list, &dctx->records); + dctx->num_records++; + mtx_unlock(&dctx->mutex); } static void @@ -1134,8 +1140,7 @@ dd_after_draw_async(void *data) record->log_page = u_log_new_page(&dctx->log); record->time_after = os_time_get_nano(); - if (!util_queue_fence_is_signalled(&record->driver_finished)) - util_queue_fence_signal(&record->driver_finished); + util_queue_fence_signal(&record->driver_finished); if (dscreen->dump_mode == DD_DUMP_APITRACE_CALL && dscreen->apitrace_dump_call > dctx->draw_state.apitrace_call_number) { @@ -1158,34 +1163,14 @@ dd_after_draw(struct dd_context *dctx, struct dd_draw_record *record) else flush_flags = PIPE_FLUSH_DEFERRED | PIPE_FLUSH_BOTTOM_OF_PIPE; pipe->flush(pipe, &record->bottom_of_pipe, flush_flags); - - assert(record == dctx->record_pending); } if (pipe->callback) { - util_queue_fence_reset(&record->driver_finished); pipe->callback(pipe, dd_after_draw_async, record, true); } else { dd_after_draw_async(record); } - mtx_lock(&dctx->mutex); - if (unlikely(dctx->num_records > 10000)) { - dctx->api_stalled = true; - /* Since this is only a heuristic to prevent the API thread from getting - * too far ahead, we don't need a loop here. */ - cnd_wait(&dctx->cond, &dctx->mutex); - dctx->api_stalled = false; - } - - if (list_empty(&dctx->records)) - cnd_signal(&dctx->cond); - - list_addtail(&record->list, &dctx->records); - dctx->record_pending = NULL; - dctx->num_records++; - mtx_unlock(&dctx->mutex); - ++dctx->num_draw_calls; if (dscreen->skip_count && dctx->num_draw_calls % 10000 == 0) fprintf(stderr, "Gallium debugger reached %u draw calls.\n", diff --git a/src/gallium/auxiliary/driver_ddebug/dd_pipe.h b/src/gallium/auxiliary/driver_ddebug/dd_pipe.h index 07c4d55017f..12da8280aa6 100644 --- a/src/gallium/auxiliary/driver_ddebug/dd_pipe.h +++ b/src/gallium/auxiliary/driver_ddebug/dd_pipe.h @@ -274,6 +274,7 @@ struct dd_draw_record { int64_t time_after; unsigned draw_call; + /* The fence pointers are guaranteed to be valid once driver_finished is signalled */ struct pipe_fence_handle *prev_bottom_of_pipe; struct pipe_fence_handle *top_of_pipe; struct pipe_fence_handle *bottom_of_pipe; @@ -297,24 +298,18 @@ struct dd_context /* Pipelined hang detection. * - * This is without unnecessary flushes and waits. There is a memory-based - * fence that is incremented by clear_buffer every draw call. Driver fences - * are not used. + * Before each draw call, a new dd_draw_record is created that contains + * a copy of all states. After each draw call, the driver's log is added + * to this record. Additionally, deferred fences are associated to each + * record both before and after the draw. * - * After each draw call, a new dd_draw_record is created that contains - * a copy of all states, the output of pipe_context::dump_debug_state, - * and it has a fence number assigned. That's done without knowing whether - * that draw call is problematic or not. The record is added into the list - * of all records. - * - * An independent, separate thread loops over the list of records and checks - * their fences. Records with signalled fences are freed. On fence timeout, - * the thread dumps the records of in-flight draws. + * The records are handed off to a separate thread which waits on the + * records' fences. Records with signalled fences are freed. When a timeout + * is detected, the thread dumps the records of in-flight draws. */ thrd_t thread; mtx_t mutex; cnd_t cond; - struct dd_draw_record *record_pending; /* currently inside the driver */ struct list_head records; /* oldest record first */ unsigned num_records; bool kill_thread; -- 2.30.2