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

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Nov 22, 2021 at 01:13:24PM -0800, Peter Oskolkov wrote:
> +	while (true) {

(you have 2 inf. loops in umcg and you chose a different expression for each)

> +		u64 umcg_state;
> +
> +		/*
> +		 * We need to read from userspace _after_ the task is marked
> +		 * TASK_INTERRUPTIBLE, to properly handle concurrent wakeups;
> +		 * but faulting is not allowed; so we try a fast no-fault read,
> +		 * and if it fails, pin the page temporarily.
> +		 */

That comment is misleading! Faulting *is* allowed, but it can scribble
__state. If faulting would not be allowed, you wouldn't be able to call
pin_user_pages_fast().

> +retry_once:
> +		set_current_state(TASK_INTERRUPTIBLE);
> +
> +		/* Order set_current_state above with get_user below. */
> +		smp_mb();

And just in case you hadn't yet seen, that smp_mb() is implied by
set_current_state().

> +		ret = -EFAULT;
> +		if (get_user_nofault(umcg_state, &self->state_ts)) {
> +			set_current_state(TASK_RUNNING);
> +
> +			if (pinned_page)
> +				goto out;
> +			else if (1 != pin_user_pages_fast((unsigned long)self,
> +						1, 0, &pinned_page))

That else is pointless, and that '1 != foo' coding style is evil.

> +					goto out;
> +
> +			goto retry_once;
> +		}

And, as you could've seen from the big patch, all that goto isn't
actually needed here, break / continue seem to be sufficient.

> +
> +		if (pinned_page) {
> +			unpin_user_page(pinned_page);
> +			pinned_page = NULL;
> +		}