Re: [RFC][PATCH 0/3] sched: User Managed Concurrency Groups

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Dec 14, 2021 at 07:46:25PM -0800, Peter Oskolkov wrote:
> On Tue, Dec 14, 2021 at 12:55 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Hi,
> >
> > This is actually tested code; but still missing the SMP wake-to-idle machinery.
> > I still need to think about that.
> 
> Thanks, Peter!
> 
> At a first glance, your main patch does not look much smaller than
> mine, and I thought the whole point of re-doing it was to throw away
> extra features and make things smaller/simpler...

Well, simpler was the goal. I didn't really focus on size much. It isn't
really big to begin with.

But yes, it has 5 hooks now, 3 syscalls and lots of comments and all
that under 900 lines, not bad I'd say.

Also I think you wanted something like this? I'm not sure of the LAZY
name, but I can't seem to come up with anything saner atm.


---
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1297,6 +1297,7 @@ struct task_struct {
 
 #ifdef CONFIG_UMCG
 	/* setup by sys_umcg_ctrl() */
+	u32			umcg_flags;
 	clockid_t		umcg_clock;
 	struct umcg_task __user	*umcg_task;
 
--- a/include/uapi/linux/umcg.h
+++ b/include/uapi/linux/umcg.h
@@ -133,11 +133,13 @@ struct umcg_task {
  * @UMCG_CTL_REGISTER:   register the current task as a UMCG task
  * @UMCG_CTL_UNREGISTER: unregister the current task as a UMCG task
  * @UMCG_CTL_WORKER:     register the current task as a UMCG worker
+ * @UMCG_CTL_LAZY:	 don't wake server on runnable enqueue
  */
 enum umcg_ctl_flag {
 	UMCG_CTL_REGISTER	= 0x00001,
 	UMCG_CTL_UNREGISTER	= 0x00002,
 	UMCG_CTL_WORKER		= 0x10000,
+	UMCG_CTL_LAZY		= 0x20000,
 };
 
 #endif /* _UAPI_LINUX_UMCG_H */
--- a/kernel/sched/umcg.c
+++ b/kernel/sched/umcg.c
@@ -416,6 +416,27 @@ static int umcg_enqueue_runnable(struct
 }
 
 /*
+ * Enqueue tsk to it's server's runnable list and wake the server for pickup if
+ * so desired. Notable LAZY workers will not wake the server and rely on the
+ * server to do pickup whenever it naturally runs next.
+ *
+ * Returns:
+ * 0:	success
+ * -EFAULT
+ */
+static int umcg_enqueue_and_wake(struct task_struct *tsk, bool force)
+{
+	int ret = umcg_enqueue_runnable(tsk);
+	if (ret)
+		return ret;
+
+	if (force || !(tsk->umcg_flags & UMCG_CTL_LAZY))
+		ret = umcg_wake_server(tsk);
+
+	return ret;
+}
+
+/*
  * umcg_wait: Wait for ->state to become RUNNING
  *
  * Returns:
@@ -522,12 +543,8 @@ void umcg_sys_exit(struct pt_regs *regs)
 	if (umcg_update_state(tsk, self, UMCG_TASK_BLOCKED, UMCG_TASK_RUNNABLE))
 		UMCG_DIE_UNPIN("state");
 
-	if (umcg_enqueue_runnable(tsk))
-		UMCG_DIE_UNPIN("enqueue");
-
-	/* Server might not be RUNNABLE, means it's already running */
-	if (umcg_wake_server(tsk))
-		UMCG_DIE_UNPIN("wake-server");
+	if (umcg_enqueue_and_wake(tsk, false))
+		UMCG_DIE_UNPIN("enqueue-and-wake");
 
 	umcg_unpin_pages();
 
@@ -582,15 +599,11 @@ void umcg_notify_resume(struct pt_regs *
 				      UMCG_TASK_RUNNABLE))
 			UMCG_DIE_UNPIN("state");
 
-		if (umcg_enqueue_runnable(tsk))
-			UMCG_DIE_UNPIN("enqueue");
-
 		/*
-		 * XXX do we want a preemption consuming ::next_tid ?
-		 * I'm currently leaning towards no.
+		 * Preemption relies on waking the server on enqueue.
 		 */
-		if (umcg_wake_server(tsk))
-			UMCG_DIE_UNPIN("wake-server");
+		if (umcg_enqueue_and_wake(tsk, true))
+			UMCG_DIE_UNPIN("enqueue-and-wake");
 
 		umcg_unpin_pages();
 	}
@@ -686,23 +699,15 @@ SYSCALL_DEFINE2(umcg_wait, u32, flags, u
 		goto unpin;
 
 	if (worker) {
-		ret = umcg_enqueue_runnable(tsk);
+		ret = umcg_enqueue_and_wake(tsk, !tsk->umcg_next);
 		if (ret)
 			goto unpin;
 	}
 
-	if (worker)
-		ret = umcg_wake(tsk);
-	else if (tsk->umcg_next)
+	if (tsk->umcg_next) {
 		ret = umcg_wake_next(tsk);
-
-	if (ret) {
-		/*
-		 * XXX already enqueued ourself on ::server_tid; failing now
-		 * leaves the lot in an inconsistent state since it'll also
-		 * unblock self in order to return the error. !?!?
-		 */
-		goto unpin;
+		if (ret)
+			goto unpin;
 	}
 
 	umcg_unpin_pages();
@@ -783,7 +788,8 @@ SYSCALL_DEFINE3(umcg_ctl, u32, flags, st
 
 	if (flags & ~(UMCG_CTL_REGISTER |
 		      UMCG_CTL_UNREGISTER |
-		      UMCG_CTL_WORKER))
+		      UMCG_CTL_WORKER |
+		      UMCG_CTL_LAZY))
 		return -EINVAL;
 
 	if (flags == UMCG_CTL_UNREGISTER) {
@@ -827,7 +833,7 @@ SYSCALL_DEFINE3(umcg_ctl, u32, flags, st
 	rcu_read_lock();
 	server = find_task_by_vpid(ut.server_tid);
 	if (server && server->mm == current->mm) {
-		if (flags == UMCG_CTL_WORKER) {
+		if (flags & UMCG_CTL_WORKER) {
 			if (!server->umcg_task ||
 			    (server->flags & PF_UMCG_WORKER))
 				server = NULL;
@@ -843,10 +849,11 @@ SYSCALL_DEFINE3(umcg_ctl, u32, flags, st
 	if (!server)
 		return -ESRCH;
 
-	if (flags == UMCG_CTL_WORKER) {
+	if (flags & UMCG_CTL_WORKER) {
 		if ((ut.state & (UMCG_TASK_MASK | UMCG_TF_MASK)) != UMCG_TASK_BLOCKED)
 			return -EINVAL;
 
+		current->umcg_flags = flags & UMCG_CTL_LAZY;
 		WRITE_ONCE(current->umcg_task, self);
 		current->flags |= PF_UMCG_WORKER;	/* hook schedule() */
 		set_syscall_work(SYSCALL_UMCG);		/* hook syscall */
@@ -858,6 +865,7 @@ SYSCALL_DEFINE3(umcg_ctl, u32, flags, st
 		if ((ut.state & (UMCG_TASK_MASK | UMCG_TF_MASK)) != UMCG_TASK_RUNNING)
 			return -EINVAL;
 
+		current->umcg_flags = 0;
 		WRITE_ONCE(current->umcg_task, self);
 		set_thread_flag(TIF_UMCG);		/* hook return-to-user */