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

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Nov 29, 2021 at 10:08:41PM +0100, Peter Zijlstra wrote:
> I'm not sure I'm following. For this to be true A and C must be running
> on a different server right?
> 
> So we have something like:
> 
> 	S0 running A			S1 running B
> 
> Therefore:
> 
> 	S0::state == RUNNABLE		S1::state == RUNNABLE
> 	A::server_tid == S0.tid		B::server_tid == S1.tid
> 	A::state == RUNNING		B::state == RUNNING
> 
> Now, you want A to switch to C, therefore C had better be with S0, eg we
> have:
> 
> 	C::server_tid == S0.tid
> 	C::state == RUNNABLE
> 
> So then A does:
> 
> 	A::next_tid = C.tid;
> 	sys_umcg_wait();
> 
> Which will:
> 
> 	pin(A);
> 	pin(S0);
> 
> 	cmpxchg(A::state, RUNNING, RUNNABLE);
> 
> 	next_tid = A::next_tid; // C
> 
> 	enqueue(S0::runnable, A);
> 
> At which point B steals S0's runnable queue, and tries to make A go.
> 
> 					runnable = xchg(S0::runnable_list_ptr, NULL); // == A
> 					A::server_tid = S1.tid;
> 					B::next_tid = A.tid;
> 					sys_umcg_wait();
> 
> 	wake(C)
> 	  cmpxchg(C::state, RUNNABLE, RUNNING); <-- *fault*
> 
> 
> Something like that, right?

And note that there's an XXX in the code about exactly this case; it has
a question whether we want to add pin(next) to umcg_pin_pages().

That would not in fact help here, because sys_umcg_wait() is faultable
and the only reason it'll return -EFAULT is because, as stated below, C
is garbage. But it does make a difference for when we do something like:

	self->next_tid = someone;
	sys_something_we_expect_to_block();
	// handle not blocking

Because in that case userspace must have taken 'someone' from the
runnable queue and made it 'next', but then we'll not wake next but the
server, which then needs to figure out something went sideways.

So I'm tempted to add that optional 3rd pin, simply to reduce the
failure cases.

> What currently happens is that S0 goes back to S0 and S1 ends up in A.
> That is, if, for any reason we fail to wake next_tid, we'll wake
> server_tid.
> 
> So then S0 wakes up and gets to re-evaluate life. If it has another
> worker it can go run that, otherwise it can try and steal a worker
> somewhere or just idle out.
> 
> Now arguably, the only reason A->C can fault is because C is garbage, at
> which point your program is malformed and it doesn't matter what
> happens one way or the other.