From 9ecaedff8409cb8c3ecac72121a7dc5212c3cd24 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 25 Jul 2026 21:39:52 +0200 Subject: [PATCH 1/2] progress: show hashing workers as idle when the queue starves A csum worker keeps its progress slot for its whole life and rolls it from file to file, so between files the line keeps whatever the last file left behind - "commit", the status set for the final DB write. That is invisible on a hashing-bound run (the next file overwrites it within microseconds) but wrong the moment the walk becomes the bottleneck: on a big NAS tree where only a handful of files need hashing, all four workers sat on "commit " for the entire rest of the run, reading like a hang in the commit path. Park the slot as idle once a worker has waited SCAN_IDLE_PARK_MS (250ms, > one 100ms redraw) for work: long enough that the sub-millisecond gaps between small files still never flicker, short enough that a genuinely starved pool stops lying within a blink. The first wait of a starvation is bounded, the rest is the plain blocking wait as before. A parked slot stays *owned*, so pscan_claim_slot() cannot hand it to a sibling worker while its owner is only waiting - pscan_slot_idle() now means "released for good" (drain, or the churning dedupe pool finishing a work item) and clears that ownership. pscan_register_thread() sets status/owned before publishing the slot for the same reason, and is now static (nothing outside progress.c used it). Co-Authored-By: Claude --- src/file_scan.c | 39 +++++++++++++++++++++++++++--- src/progress.c | 51 +++++++++++++++++++++++++++++++-------- src/progress.h | 13 ++++++---- src/tests.c | 63 ++++++++++++++++++++++++++++++++++++++++++++----- 4 files changed, 143 insertions(+), 23 deletions(-) diff --git a/src/file_scan.c b/src/file_scan.c index 0b88c7803cfc..835f33b9f033 100644 --- a/src/file_scan.c +++ b/src/file_scan.c @@ -1828,17 +1828,47 @@ static void scan_workq_push(struct file_to_scan *file) g_mutex_unlock(&q->lock); } +/* + * How long a worker may sit empty-handed before its progress line is parked as + * "idle". Longer than a redraw (100ms on a tty), so the sub-millisecond gaps + * between two small files never flicker; short enough that a genuinely starved + * pool - the walk is the bottleneck, or almost everything is up to date - stops + * showing the last file's "commit" within a blink. + */ +#define SCAN_IDLE_PARK_MS 250 + /* Pop from the largest non-empty bucket, or NULL once drained. Blocks. O(1). */ -static struct file_to_scan *scan_workq_pop(struct scan_workq *q) +static struct file_to_scan *scan_workq_pop(struct scan_workq *q, + struct pscan_thread *slot) { struct file_to_scan *file; unsigned b; bool waited = false; + bool parked = false; g_mutex_lock(&q->lock); while (q->occupied == 0 && !q->draining) { waited = true; /* starved: no work, blocking on the producer */ - g_cond_wait(&q->cond, &q->lock); + if (parked) { + g_cond_wait(&q->cond, &q->lock); + continue; + } + /* + * First wait of this starvation: bounded, so that a wait which + * turns out to be a long one can be reflected in the display. + * The park itself takes the progress mutex, so drop q->lock for + * it rather than nesting the two locks - the loop re-checks the + * queue anyway. + */ + if (g_cond_wait_until(&q->cond, &q->lock, + g_get_monotonic_time() + + SCAN_IDLE_PARK_MS * G_TIME_SPAN_MILLISECOND)) + continue; /* signalled (or spurious): re-check */ + + g_mutex_unlock(&q->lock); + pscan_slot_wait(slot); /* still nothing to do: show idle */ + parked = true; + g_mutex_lock(&q->lock); } if (q->occupied == 0) { g_mutex_unlock(&q->lock); @@ -1876,10 +1906,13 @@ static gpointer scan_worker(gpointer arg) * actually empty (see pscan_finish_file). Claimed lazily on the first file * with a non-idle status so a still-unclaimed slot is never reused by a * sibling worker; a worker that gets no files never shows a phantom line. + * While the queue starves, scan_workq_pop() shows the slot as idle + * (without giving it up), so a walk-bound run doesn't freeze the line on + * the last file it hashed. */ struct pscan_thread *slot = NULL; - while ((file = scan_workq_pop(q))) { + while ((file = scan_workq_pop(q, slot))) { if (!slot) slot = pscan_claim_slot(gettid(), thread_scanning); csum_whole_file(file, &buffer, slot); diff --git a/src/progress.c b/src/progress.c index 8cf6b3c78954..519e01da9068 100644 --- a/src/progress.c +++ b/src/progress.c @@ -962,10 +962,19 @@ static void pscan_free_threads(void) pscan.thread_count = 0; } -struct pscan_thread *pscan_register_thread(pid_t tid) +/* + * Allocate a fresh slot, already claimed by `tid`, and publish it. status and + * owned are set before publication: the moment the slot is in pscan.threads a + * concurrent pscan_claim_slot() may look at it, and a slot that is still + * idle/unowned there would be handed to a second worker. + */ +static struct pscan_thread *pscan_register_thread(pid_t tid, + enum pscan_thread_status status) { struct pscan_thread *tprogress = calloc(1, sizeof(struct pscan_thread)); tprogress->tid = tid; + tprogress->status = status; + tprogress->owned = true; g_mutex_lock(&pscan.mutex); pscan.threads = realloc(pscan.threads, (pscan.thread_count + 1) * @@ -1105,21 +1114,41 @@ void pscan_set_file(struct pscan_thread *slot, const char *path, } /* - * Park a persistently-held slot as idle once its worker has no more work (drain). - * No per-file accounting - pscan_finish_file() already ran for the last file. + * Show a persistently-held slot as idle while its worker waits for more work, + * without releasing it: the worker keeps the slot and rolls it into its next + * file. Used when the hashing queue starves - the walk is the bottleneck, or + * nearly everything is up to date - so the line reads "idle" instead of + * freezing on the last file's "commit" for the rest of the run. + * No per-file accounting: pscan_finish_file() already ran for the last file. + */ +void pscan_slot_wait(struct pscan_thread *slot) +{ + if (!slot) + return; + /* file_path is only written under this mutex (the renderer reads it). */ + g_mutex_lock(&pscan.mutex); + slot->file_path[0] = '\0'; + slot->status = thread_idle; + g_mutex_unlock(&pscan.mutex); +} + +/* + * Release a slot: its worker is done with it for good (drain, or the churning + * dedupe pool finishing one work item), so another worker may claim it. */ void pscan_slot_idle(struct pscan_thread *slot) { if (!slot) return; /* - * Park the slot under the same mutex pscan_claim_slot() scans with, so - * that lock is the handoff edge to whichever worker picks it up next. + * Release the slot under the same mutex pscan_claim_slot() scans with, + * so that lock is the handoff edge to whichever worker picks it up next. * (Every other slot field is either _Atomic or written under this mutex.) */ g_mutex_lock(&pscan.mutex); slot->file_path[0] = '\0'; slot->status = thread_idle; + slot->owned = false; g_mutex_unlock(&pscan.mutex); } @@ -1171,19 +1200,21 @@ struct pscan_thread *pscan_claim_slot(pid_t tid, g_mutex_lock(&pscan.mutex); for (unsigned int i = 0; i < pscan.thread_count; i++) { - if (pscan.threads[i]->status == thread_idle) { + /* Idle but still owned = a persistent worker waiting for its + * next file; it will be back, so leave that line alone. */ + if (pscan.threads[i]->status == thread_idle && + !pscan.threads[i]->owned) { slot = pscan.threads[i]; slot->tid = tid; slot->status = status; + slot->owned = true; break; } } g_mutex_unlock(&pscan.mutex); - if (!slot) { - slot = pscan_register_thread(tid); - slot->status = status; - } + if (!slot) + slot = pscan_register_thread(tid, status); return slot; } diff --git a/src/progress.h b/src/progress.h index d6916a64865a..487f86d07dc6 100644 --- a/src/progress.h +++ b/src/progress.h @@ -40,6 +40,11 @@ struct pscan_thread { * the renderer reads it every redraw - hence atomic. The idle/claim * handoff itself is ordered by the mutex, not by this field. */ _Atomic enum pscan_thread_status status; + + /* A worker still owns this slot and will come back to it, so no other + * worker may claim it - even while it sits idle (pscan_slot_wait()). + * Only ever read/written under pscan.mutex. */ + bool owned; }; struct pscan_global { @@ -90,9 +95,6 @@ void pscan_set_progress(uint64_t added_files, uint64_t added_bytes); void pscan_examined(void); uint64_t pscan_files_scanned(void); -/* Used by each scan threads to grab its own struct pscan_thread */ -struct pscan_thread *pscan_register_thread(pid_t tid); - /* * Setup the pty and start the progress thread * The thread will run until the scan is done, that is: @@ -123,9 +125,12 @@ void pscan_reset_thread(struct pscan_thread **progress); * For a persistently-held slot (one kept by a long-lived worker across many * files, rather than re-claimed per file): pscan_finish_file() does the per-file * byte/count accounting without going idle, so no "idle" flashes between files; - * pscan_slot_idle() parks the slot when the worker finally runs out of work. + * pscan_slot_wait() shows the slot as idle while its worker waits for more work + * but keeps it reserved (the worker comes back to the same line); + * pscan_slot_idle() releases it when the worker finally runs out of work. */ void pscan_finish_file(struct pscan_thread **progress); +void pscan_slot_wait(struct pscan_thread *slot); void pscan_slot_idle(struct pscan_thread *slot); bool is_progress_printer_running(void); diff --git a/src/tests.c b/src/tests.c index 72c6d53460a9..a088eaaf75db 100644 --- a/src/tests.c +++ b/src/tests.c @@ -337,19 +337,69 @@ MU_TEST(test_scan_workq_priority) { /* Biggest bucket first; within b4, FIFO keeps pos2 before pos4. */ struct file_to_scan *f; - f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 5); /* b7 */ - f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 2); /* b4 */ - f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 4); /* b4 */ - f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 3); /* b2 */ - f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 1); /* b0 */ + f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 5); /* b7 */ + f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 2); /* b4 */ + f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 4); /* b4 */ + f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 3); /* b2 */ + f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 1); /* b0 */ /* Empty + draining => pop returns NULL (worker would exit). */ scan_workq.draining = true; - mu_check(scan_workq_pop(&scan_workq) == NULL); + mu_check(scan_workq_pop(&scan_workq, NULL) == NULL); memset(&scan_workq, 0, sizeof(scan_workq)); } +struct park_probe { + struct pscan_thread *slot; + struct file_to_scan *got; +}; + +static gpointer park_probe_pop(gpointer arg) +{ + struct park_probe *probe = arg; + + probe->got = scan_workq_pop(&scan_workq, probe->slot); + return NULL; +} + +MU_TEST(test_scan_slot_parks_idle_when_starved) { + /* + * A csum worker holds its display slot across files, so whatever status + * the last file left behind ("commit") is what a starved queue would + * keep showing - for the whole rest of a walk-bound run. After + * SCAN_IDLE_PARK_MS with nothing to hash the slot must read "idle" + * instead, while staying owned so no sibling worker takes over its line. + */ + memset(&scan_workq, 0, sizeof(scan_workq)); + + struct file_to_scan file = { .filesize = 4096, .file_position = 1 }; + struct pscan_thread *slot = pscan_claim_slot(4242, thread_committing); + struct park_probe probe = { .slot = slot }; + GThread *popper = g_thread_new("park", park_probe_pop, &probe); + + g_usleep((SCAN_IDLE_PARK_MS + 250) * 1000); + mu_check(slot->status == thread_idle); + mu_check(slot->owned); + + /* Owned: a worker claiming now gets a line of its own, not this one. */ + struct pscan_thread *other = pscan_claim_slot(4243, thread_scanning); + mu_check(other != slot); + + /* Work again: the parked worker wakes up on its own slot. */ + scan_workq_push(&file); + g_thread_join(popper); + mu_check(probe.got == &file); + + /* Released for good (drain): now it is up for grabs. */ + pscan_slot_idle(other); + pscan_slot_idle(slot); + mu_check(pscan_claim_slot(4244, thread_scanning) == slot); + + pscan_free_threads(); + memset(&scan_workq, 0, sizeof(scan_workq)); +} + /* Within eps of expected. */ static bool near(double got, double want, double eps) { @@ -634,6 +684,7 @@ MU_TEST_SUITE(test_suite) { MU_RUN_TEST(test_storage_recommend_io_threads); MU_RUN_TEST(test_scan_bucket); MU_RUN_TEST(test_scan_workq_priority); + MU_RUN_TEST(test_scan_slot_parks_idle_when_starved); MU_RUN_TEST(test_scan_eta); MU_RUN_TEST(test_group_u64); MU_RUN_TEST(test_longpath); From 3df35455352202ee9982167d94424acfa01f6cec Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 25 Jul 2026 21:53:27 +0200 Subject: [PATCH 2/2] progress: decide the idle line in the renderer, not in the work queue Follow-up cleanup of the previous commit's mechanism (a /simplify pass). It parked the line from inside scan_workq_pop(): a bounded first wait, then a call into the progress module to overwrite the slot's status. That put display timing in a data-structure primitive, needed a display slot threaded through a queue pop, and needed an `owned` flag so that a parked-but-still-busy slot wasn't handed to a sibling worker - two fields encoding one allocation state. Publish the fact instead of acting on it: the worker records when it started waiting (pscan_slot_waiting, one atomic store bracketing the pop), and the renderer draws any line whose wait outlasts a couple of redraws as idle. Same behaviour, and it deletes the timed wait, the park constant, the `owned` field, pscan_slot_wait() - a near-duplicate of pscan_slot_idle() - and the pscan_thread parameter on scan_workq_pop(), whose signature and unit test go back to what they were. The display threshold now lives next to the redraw interval it is derived from, both named rather than bare literals. The unit test loses its helper thread's 500 ms sleep (the whole C suite was 0.50 s wall for 0.6 ms of CPU): with the rule a pure function of a timestamp, backdating waiting_since tests it exactly, in microseconds. Co-Authored-By: Claude --- src/file_scan.c | 48 +++++++------------------ src/progress.c | 93 +++++++++++++++++++++++++++++-------------------- src/progress.h | 25 ++++++++----- src/tests.c | 67 +++++++++++++++++------------------ 4 files changed, 115 insertions(+), 118 deletions(-) diff --git a/src/file_scan.c b/src/file_scan.c index 835f33b9f033..49cce8f9d67f 100644 --- a/src/file_scan.c +++ b/src/file_scan.c @@ -1828,47 +1828,17 @@ static void scan_workq_push(struct file_to_scan *file) g_mutex_unlock(&q->lock); } -/* - * How long a worker may sit empty-handed before its progress line is parked as - * "idle". Longer than a redraw (100ms on a tty), so the sub-millisecond gaps - * between two small files never flicker; short enough that a genuinely starved - * pool - the walk is the bottleneck, or almost everything is up to date - stops - * showing the last file's "commit" within a blink. - */ -#define SCAN_IDLE_PARK_MS 250 - /* Pop from the largest non-empty bucket, or NULL once drained. Blocks. O(1). */ -static struct file_to_scan *scan_workq_pop(struct scan_workq *q, - struct pscan_thread *slot) +static struct file_to_scan *scan_workq_pop(struct scan_workq *q) { struct file_to_scan *file; unsigned b; bool waited = false; - bool parked = false; g_mutex_lock(&q->lock); while (q->occupied == 0 && !q->draining) { waited = true; /* starved: no work, blocking on the producer */ - if (parked) { - g_cond_wait(&q->cond, &q->lock); - continue; - } - /* - * First wait of this starvation: bounded, so that a wait which - * turns out to be a long one can be reflected in the display. - * The park itself takes the progress mutex, so drop q->lock for - * it rather than nesting the two locks - the loop re-checks the - * queue anyway. - */ - if (g_cond_wait_until(&q->cond, &q->lock, - g_get_monotonic_time() + - SCAN_IDLE_PARK_MS * G_TIME_SPAN_MILLISECOND)) - continue; /* signalled (or spurious): re-check */ - - g_mutex_unlock(&q->lock); - pscan_slot_wait(slot); /* still nothing to do: show idle */ - parked = true; - g_mutex_lock(&q->lock); + g_cond_wait(&q->cond, &q->lock); } if (q->occupied == 0) { g_mutex_unlock(&q->lock); @@ -1906,13 +1876,19 @@ static gpointer scan_worker(gpointer arg) * actually empty (see pscan_finish_file). Claimed lazily on the first file * with a non-idle status so a still-unclaimed slot is never reused by a * sibling worker; a worker that gets no files never shows a phantom line. - * While the queue starves, scan_workq_pop() shows the slot as idle - * (without giving it up), so a walk-bound run doesn't freeze the line on - * the last file it hashed. + * The flip side is that between files the line keeps showing the last one's + * status, so bracket the wait for the next file: a wait long enough to + * outlast a couple of redraws is what makes the line read "idle" instead of + * freezing on the last file's "commit" for the rest of a walk-bound run. */ struct pscan_thread *slot = NULL; - while ((file = scan_workq_pop(q, slot))) { + for (;;) { + pscan_slot_waiting(slot, true); + file = scan_workq_pop(q); + pscan_slot_waiting(slot, false); + if (!file) + break; if (!slot) slot = pscan_claim_slot(gettid(), thread_scanning); csum_whole_file(file, &buffer, slot); diff --git a/src/progress.c b/src/progress.c index 519e01da9068..f989a013189c 100644 --- a/src/progress.c +++ b/src/progress.c @@ -383,6 +383,18 @@ static const char *const BAR_SUB[] = { /* Worker-line left column: number(3) + gap(2) + status + gap(2) before the path. */ #define WORKER_LEFT_W (3 + 2 + STATUS_COL_W + 2) +/* Redraw cadence of the live block: smooth on a tty, sparse into a log. */ +#define REDRAW_MS 100 +#define REDRAW_LOG_MS 1000 + +/* + * How long a worker may wait for its next unit of work before its line reads + * "idle" rather than the status the last one left behind. Longer than a redraw, + * so the sub-millisecond gaps between two small files never flicker; short + * enough that a starved pool stops lying within a blink. + */ +#define IDLE_AFTER_US (2 * REDRAW_MS * G_TIME_SPAN_MILLISECOND) + /* The dim " ยท " separator between fields on the bar and detail lines. */ static void detail_sep(void) { @@ -416,6 +428,20 @@ static const char *status_color(enum pscan_thread_status s) return col_reset; } +/* + * Does this line read as idle? Either the slot is unclaimed, or its worker has + * been waiting for work long enough that drawing the last file's status would + * be a lie (a persistent csum worker keeps its line across files, so that + * status outlives the file - see pscan_slot_waiting). + */ +static bool slot_is_idle(struct pscan_thread *t) +{ + gint64 since = t->waiting_since; + + return t->status == thread_idle || + (since && g_get_monotonic_time() - since > IDLE_AFTER_US); +} + static void print_thread_progress(struct pscan_thread *t, unsigned int slot) { char buf[BUF_LEN]; @@ -426,9 +452,10 @@ static void print_thread_progress(struct pscan_thread *t, unsigned int slot) int avail, termw; /* Idle slots are just the dim number and word - nothing on the right. */ - if (t->status == thread_idle) { - s_printf("%s%3u%s %s%s%s\n", - col_dim, slot, col_reset, wcol, word, col_reset); + if (slot_is_idle(t)) { + s_printf("%s%3u%s %s%s%s\n", col_dim, slot, col_reset, + status_color(thread_idle), status_word(thread_idle), + col_reset); return; } @@ -927,7 +954,7 @@ static void *pscan_progress_thread(void * p) json_last = now; json_last_phase = phase; } - usleep(100 * 1000); + usleep(REDRAW_MS * 1000); continue; } @@ -947,7 +974,7 @@ static void *pscan_progress_thread(void * p) spin_frame++; /* advance every spinner + the indeterminate bar */ /* Do not waste too much cpu */ - usleep(1000 * (tty ? 100 : 1000)); + usleep(1000 * (tty ? REDRAW_MS : REDRAW_LOG_MS)); } while (printer_running); return NULL; @@ -963,10 +990,10 @@ static void pscan_free_threads(void) } /* - * Allocate a fresh slot, already claimed by `tid`, and publish it. status and - * owned are set before publication: the moment the slot is in pscan.threads a - * concurrent pscan_claim_slot() may look at it, and a slot that is still - * idle/unowned there would be handed to a second worker. + * Allocate a fresh slot, already claimed by `tid`, and publish it. The status + * is set before publication: the moment the slot is in pscan.threads a + * concurrent pscan_claim_slot() may look at it, and a still-idle slot there + * would be handed to a second worker. */ static struct pscan_thread *pscan_register_thread(pid_t tid, enum pscan_thread_status status) @@ -974,7 +1001,6 @@ static struct pscan_thread *pscan_register_thread(pid_t tid, struct pscan_thread *tprogress = calloc(1, sizeof(struct pscan_thread)); tprogress->tid = tid; tprogress->status = status; - tprogress->owned = true; g_mutex_lock(&pscan.mutex); pscan.threads = realloc(pscan.threads, (pscan.thread_count + 1) * @@ -1114,42 +1140,31 @@ void pscan_set_file(struct pscan_thread *slot, const char *path, } /* - * Show a persistently-held slot as idle while its worker waits for more work, - * without releasing it: the worker keeps the slot and rolls it into its next - * file. Used when the hashing queue starves - the walk is the bottleneck, or - * nearly everything is up to date - so the line reads "idle" instead of - * freezing on the last file's "commit" for the rest of the run. - * No per-file accounting: pscan_finish_file() already ran for the last file. + * Park a persistently-held slot as idle once its worker has no more work (drain). + * No per-file accounting - pscan_finish_file() already ran for the last file. */ -void pscan_slot_wait(struct pscan_thread *slot) +void pscan_slot_idle(struct pscan_thread *slot) { if (!slot) return; - /* file_path is only written under this mutex (the renderer reads it). */ + /* + * Park the slot under the same mutex pscan_claim_slot() scans with, so + * that lock is the handoff edge to whichever worker picks it up next. + * (Every other slot field is either _Atomic or written under this mutex.) + */ g_mutex_lock(&pscan.mutex); slot->file_path[0] = '\0'; slot->status = thread_idle; + slot->waiting_since = 0; g_mutex_unlock(&pscan.mutex); } -/* - * Release a slot: its worker is done with it for good (drain, or the churning - * dedupe pool finishing one work item), so another worker may claim it. - */ -void pscan_slot_idle(struct pscan_thread *slot) +/* See progress.h: the worker publishes the wait, slot_is_idle() judges it. */ +void pscan_slot_waiting(struct pscan_thread *slot, bool waiting) { if (!slot) return; - /* - * Release the slot under the same mutex pscan_claim_slot() scans with, - * so that lock is the handoff edge to whichever worker picks it up next. - * (Every other slot field is either _Atomic or written under this mutex.) - */ - g_mutex_lock(&pscan.mutex); - slot->file_path[0] = '\0'; - slot->status = thread_idle; - slot->owned = false; - g_mutex_unlock(&pscan.mutex); + slot->waiting_since = waiting ? g_get_monotonic_time() : 0; } bool is_progress_printer_running(void) @@ -1200,14 +1215,16 @@ struct pscan_thread *pscan_claim_slot(pid_t tid, g_mutex_lock(&pscan.mutex); for (unsigned int i = 0; i < pscan.thread_count; i++) { - /* Idle but still owned = a persistent worker waiting for its - * next file; it will be back, so leave that line alone. */ - if (pscan.threads[i]->status == thread_idle && - !pscan.threads[i]->owned) { + /* + * Only a released slot is free. A persistent worker waiting for + * its next file keeps a non-idle status (that is what + * waiting_since exists to soften, in the renderer only), so its + * line is never handed to a sibling. + */ + if (pscan.threads[i]->status == thread_idle) { slot = pscan.threads[i]; slot->tid = tid; slot->status = status; - slot->owned = true; break; } } diff --git a/src/progress.h b/src/progress.h index 487f86d07dc6..8a695338e208 100644 --- a/src/progress.h +++ b/src/progress.h @@ -41,10 +41,10 @@ struct pscan_thread { * handoff itself is ordered by the mutex, not by this field. */ _Atomic enum pscan_thread_status status; - /* A worker still owns this slot and will come back to it, so no other - * worker may claim it - even while it sits idle (pscan_slot_wait()). - * Only ever read/written under pscan.mutex. */ - bool owned; + /* Monotonic us at which this slot's worker started waiting for its next + * unit of work, or 0 while it has work. Set by the worker, read by the + * renderer every redraw - hence atomic. See pscan_slot_waiting(). */ + _Atomic gint64 waiting_since; }; struct pscan_global { @@ -125,14 +125,23 @@ void pscan_reset_thread(struct pscan_thread **progress); * For a persistently-held slot (one kept by a long-lived worker across many * files, rather than re-claimed per file): pscan_finish_file() does the per-file * byte/count accounting without going idle, so no "idle" flashes between files; - * pscan_slot_wait() shows the slot as idle while its worker waits for more work - * but keeps it reserved (the worker comes back to the same line); - * pscan_slot_idle() releases it when the worker finally runs out of work. + * pscan_slot_idle() parks the slot when the worker finally runs out of work. */ void pscan_finish_file(struct pscan_thread **progress); -void pscan_slot_wait(struct pscan_thread *slot); void pscan_slot_idle(struct pscan_thread *slot); +/* + * Bracket a persistently-held slot's wait for its next unit of work. Such a + * slot keeps showing the last file's status between files, which is right for + * the microsecond gap between two small files and a lie once the worker is + * starved (the walk is the bottleneck, or nearly everything is up to date). + * The worker just publishes when the wait started; the renderer decides when + * that has gone on long enough to draw the line as idle instead. One atomic + * store (plus a clock read on the `true` side), so it is cheap enough for the + * per-file path. + */ +void pscan_slot_waiting(struct pscan_thread *slot, bool waiting); + bool is_progress_printer_running(void); /* diff --git a/src/tests.c b/src/tests.c index a088eaaf75db..a758852db786 100644 --- a/src/tests.c +++ b/src/tests.c @@ -337,64 +337,59 @@ MU_TEST(test_scan_workq_priority) { /* Biggest bucket first; within b4, FIFO keeps pos2 before pos4. */ struct file_to_scan *f; - f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 5); /* b7 */ - f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 2); /* b4 */ - f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 4); /* b4 */ - f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 3); /* b2 */ - f = scan_workq_pop(&scan_workq, NULL); mu_check(f->file_position == 1); /* b0 */ + f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 5); /* b7 */ + f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 2); /* b4 */ + f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 4); /* b4 */ + f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 3); /* b2 */ + f = scan_workq_pop(&scan_workq); mu_check(f->file_position == 1); /* b0 */ /* Empty + draining => pop returns NULL (worker would exit). */ scan_workq.draining = true; - mu_check(scan_workq_pop(&scan_workq, NULL) == NULL); + mu_check(scan_workq_pop(&scan_workq) == NULL); memset(&scan_workq, 0, sizeof(scan_workq)); } -struct park_probe { - struct pscan_thread *slot; - struct file_to_scan *got; -}; - -static gpointer park_probe_pop(gpointer arg) +static gpointer pop_one(gpointer arg) { - struct park_probe *probe = arg; + struct file_to_scan **got = arg; - probe->got = scan_workq_pop(&scan_workq, probe->slot); + *got = scan_workq_pop(&scan_workq); return NULL; } -MU_TEST(test_scan_slot_parks_idle_when_starved) { +MU_TEST(test_starved_worker_line_reads_idle) { /* - * A csum worker holds its display slot across files, so whatever status - * the last file left behind ("commit") is what a starved queue would - * keep showing - for the whole rest of a walk-bound run. After - * SCAN_IDLE_PARK_MS with nothing to hash the slot must read "idle" - * instead, while staying owned so no sibling worker takes over its line. + * A csum worker holds its display line across files, so the status the + * last file left behind ("commit") is what a starved queue would keep + * showing - for the whole rest of a walk-bound run. The worker publishes + * how long it has been waiting for its next file and the renderer draws + * a long enough wait as idle; a short one (the gap between two small + * files) must still show the file's real status, or the line flickers. */ memset(&scan_workq, 0, sizeof(scan_workq)); struct file_to_scan file = { .filesize = 4096, .file_position = 1 }; + struct file_to_scan *got = NULL; struct pscan_thread *slot = pscan_claim_slot(4242, thread_committing); - struct park_probe probe = { .slot = slot }; - GThread *popper = g_thread_new("park", park_probe_pop, &probe); - g_usleep((SCAN_IDLE_PARK_MS + 250) * 1000); - mu_check(slot->status == thread_idle); - mu_check(slot->owned); + /* Waiting on an empty queue: the worker blocks in the pop. */ + GThread *popper = g_thread_new("pop", pop_one, &got); + + pscan_slot_waiting(slot, true); + mu_check(!slot_is_idle(slot)); /* just started waiting */ + slot->waiting_since -= IDLE_AFTER_US + 1; /* ... a while ago */ + mu_check(slot_is_idle(slot)); - /* Owned: a worker claiming now gets a line of its own, not this one. */ - struct pscan_thread *other = pscan_claim_slot(4243, thread_scanning); - mu_check(other != slot); + /* Still claimed while it waits, so no sibling takes over its line. */ + mu_check(pscan_claim_slot(4243, thread_scanning) != slot); - /* Work again: the parked worker wakes up on its own slot. */ + /* Back to work: the line shows the file again, not idle. */ scan_workq_push(&file); g_thread_join(popper); - mu_check(probe.got == &file); - - /* Released for good (drain): now it is up for grabs. */ - pscan_slot_idle(other); - pscan_slot_idle(slot); - mu_check(pscan_claim_slot(4244, thread_scanning) == slot); + pscan_slot_waiting(slot, false); + mu_check(got == &file); + mu_check(!slot_is_idle(slot)); pscan_free_threads(); memset(&scan_workq, 0, sizeof(scan_workq)); @@ -684,7 +679,7 @@ MU_TEST_SUITE(test_suite) { MU_RUN_TEST(test_storage_recommend_io_threads); MU_RUN_TEST(test_scan_bucket); MU_RUN_TEST(test_scan_workq_priority); - MU_RUN_TEST(test_scan_slot_parks_idle_when_starved); + MU_RUN_TEST(test_starved_worker_line_reads_idle); MU_RUN_TEST(test_scan_eta); MU_RUN_TEST(test_group_u64); MU_RUN_TEST(test_longpath);