DF-5169 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 | diff --git a/sys/dev/drm/include/linux/workqueue.h b/sys/dev/drm/include/linux/workqueue.h --- a/sys/dev/drm/include/linux/workqueue.h +++ b/sys/dev/drm/include/linux/workqueue.h @@ -54,6 +54,7 @@ struct workqueue_worker { STAILQ_HEAD(ws_list, work_struct) ws_list_head; struct thread *worker_thread; + struct work_struct *running_work; /* owner of in-flight callback */ struct lock worker_lock; }; diff --git a/sys/dev/drm/linux_workqueue.c b/sys/dev/drm/linux_workqueue.c --- a/sys/dev/drm/linux_workqueue.c +++ b/sys/dev/drm/linux_workqueue.c @@ -62,6 +62,34 @@ * - unbound high priority */ +/* + * work_is_executing() - authoritative "queued or mid-callback" test. + * + * The worker-owned worker->running_work pointer is authoritative because + * work->running is no longer cleared after the callback returns: a work + * item may legally free itself inside func() (self-freeing works are + * standard Linux semantics, e.g. i915_clflush_work()), and + * process_all_work() must not touch *work once func() has returned. + */ +static bool +work_is_executing(struct work_struct *work) +{ + struct workqueue_worker *worker; + bool executing; + + if (work->on_queue) + return (true); + if (work->running == false) + return (false); + worker = work->worker; + if (worker == NULL) + return (true); /* conservatively: about to start */ + lockmgr(&worker->worker_lock, LK_EXCLUSIVE); + executing = (worker->running_work == work); + lockmgr(&worker->worker_lock, LK_RELEASE); + return (executing); +} + static inline void process_all_work(struct workqueue_worker *worker) { @@ -74,7 +102,7 @@ work->on_queue = false; /* A work shouldn't be executed concurrently on a single cpu */ - if (work->running) + if (work_is_executing(work)) continue; /* Do not run canceled works */ @@ -84,16 +112,32 @@ continue; } + /* + * Snapshot cancel state before the callback: after func() + * returns we may no longer touch *work at all (it may have + * freed itself). running_work is set before running so any + * locked reader that observes running=true also observes + * running_work==work. + */ + didcan = work->canceled; + worker->running_work = work; work->running = true; lockmgr(&worker->worker_lock, LK_RELEASE); work->func(work); lwkt_yield(); lockmgr(&worker->worker_lock, LK_EXCLUSIVE); - if (work->on_queue == false) - work->worker = NULL; - didcan = work->canceled; + + /* + * Completion bookkeeping is worker-owned; never touch *work + * past this point. wakeup() only uses the address as a + * wait-channel key, and the free-after-cancel API contract + * keeps the item alive for any cancel_work_sync() waiter + * until woken (this covers cancels requested before func(); + * cancels during func() are picked up by the waiter's own + * 1-tick retest loop). + */ + worker->running_work = NULL; cpu_sfence(); - work->running = false; if (didcan == true) wakeup(work); } @@ -295,7 +339,7 @@ unsigned int work_busy(struct work_struct *work) { - return (work->on_queue || work->running); + return (work_is_executing(work)); } static inline void @@ -313,7 +357,7 @@ INIT_WORK(&__flush_work, __flush_work_func); queue_work(wq, &__flush_work); - while (__flush_work.on_queue || __flush_work.running) { + while (work_is_executing(&__flush_work)) { tsleep(&__flush_work, 0, "flshwq", 0); } } @@ -328,7 +372,7 @@ int ret = false; /* XXX: probably unreliable */ - while (work->on_queue || work->running) { + while (work_is_executing(work)) { ret = true; /* XXX: use something more intelligent than tsleep() */ tsleep(&flush_work, 0, "flshwrk", 1); @@ -361,14 +405,14 @@ ret = true; lockmgr(&worker->worker_lock, LK_RELEASE); } - if (work->running == false) + if (work_is_executing(work) == false) break; worker = work->worker; if (worker == NULL) continue; lockmgr(&worker->worker_lock, LK_EXCLUSIVE); - if (worker != work->worker || work->running == false) { + if (worker != work->worker || worker->running_work != work) { lockmgr(&worker->worker_lock, LK_RELEASE); continue; } |