Re: [PATCH v0.9.1 3/6] sched/umcg: implement UMCG syscalls

[Peter Zijlstra <peterz@infradead.org>]

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)