Re: [PATCH v0.9.1 3/6] sched/umcg: implement UMCG syscalls
flat view
From: Peter Zijlstra <peterz@infradead.org>
Date: 2021-12-13 13:56:00
Also in:
linux-mm, lkml
On Mon, Dec 06, 2021 at 12:32:22PM +0100, Peter Zijlstra wrote:
Sorry, I haven't been feeling too well and as such procastinated on this because thinking is required :/ Trying to pick up the bits.
*sigh* and yet another week gone... someone was unhappy about refcount_t.
No, the failure case is different; umcg_notify_resume() will simply
block A until someone sets A::state == RUNNING and kicks it, which will
be no-one.
Now, the above situation is actually simple to fix, but it gets more
interesting when we're using sys_umcg_wait() to build wait primitives.
Because in that case we get stuff like:
for (;;) {
self->state = RUNNABLE;
smp_mb();
if (cond)
break;
sys_umcg_wait();
}
self->state = RUNNING;
And we really need to not block and also not do sys_umcg_wait() early.
So yes, I agree that we need a special case here that ensures
umcg_notify_resume() doesn't block. Let me ponder naming and comments.
Either a TF_COND_WAIT or a whole new state. I can't decide yet.
Now, obviously if you do a random syscall anywhere around here, you get
to keep the pieces :-)Something like so I suppose..
--- a/include/uapi/linux/umcg.h
+++ b/include/uapi/linux/umcg.h@@ -42,6 +42,32 @@ * */ #define UMCG_TF_PREEMPT 0x0100U +/* + * UMCG_TF_COND_WAIT: indicate the task *will* call sys_umcg_wait() + * + * Enables server loops like (vs umcg_sys_exit()): + * + * for(;;) { + * self->status = UMCG_TASK_RUNNABLE | UMCG_TF_COND_WAIT; + * // smp_mb() implied by xchg() + * + * runnable_ptr = xchg(self->runnable_workers_ptr, NULL); + * while (runnable_ptr) { + * next = runnable_ptr->runnable_workers_ptr; + * + * umcg_server_add_runnable(self, runnable_ptr); + * + * runnable_ptr = next; + * } + * + * self->next = umcg_server_pick_next(self); + * sys_umcg_wait(0, 0); + * } + * + * without a signal or interrupt in between setting umcg_task::state and + * sys_umcg_wait() resulting in an infinite wait in umcg_notify_resume(). + */ +#define UMCG_TF_COND_WAIT 0x0200U #define UMCG_TF_MASK 0xff00U --- a/kernel/sched/umcg.c +++ b/kernel/sched/umcg.c
@@ -180,7 +180,7 @@ void umcg_worker_exit(void) /* * Do a state transition, @from -> @to, and possible read @next after that. * - * Will clear UMCG_TF_PREEMPT. + * Will clear UMCG_TF_PREEMPT, UMCG_TF_COND_WAIT. * * When @to == {BLOCKED,RUNNABLE}, update timestamps. *
@@ -216,7 +216,8 @@ static int umcg_update_state(struct task if ((old & UMCG_TASK_MASK) != from) goto fail; - new = old & ~(UMCG_TASK_MASK | UMCG_TF_PREEMPT); + new = old & ~(UMCG_TASK_MASK | + UMCG_TF_PREEMPT | UMCG_TF_COND_WAIT); new |= to & UMCG_TASK_MASK; } while (!unsafe_try_cmpxchg_user(&self->state, &old, new, Efault));
@@ -567,11 +568,13 @@ void umcg_notify_resume(struct pt_regs * if (state == UMCG_TASK_RUNNING) goto done; - // XXX can get here when: - // - // self->state = RUNNABLE - // <signal> - // sys_umcg_wait(); + /* + * See comment at UMCG_TF_COND_WAIT; TL;DR: user *will* call + * sys_umcg_wait() and signals/interrupts shouldn't block + * return-to-user. + */ + if (state == UMCG_TASK_RUNNABLE | UMCG_TF_COND_WAIT) + goto done; if (state & UMCG_TF_PREEMPT) { if (umcg_pin_pages())
@@ -658,6 +661,13 @@ SYSCALL_DEFINE2(umcg_wait, u32, flags, u if (ret) goto unblock; + /* + * Clear UMCG_TF_COND_WAIT *and* check state == RUNNABLE. + */ + ret = umcg_update_state(self, tsk, UMCG_TASK_RUNNABLE, UMCG_TASK_RUNNABLE); + if (ret) + goto unpin; + if (worker) { ret = umcg_enqueue_runnable(tsk); if (ret)