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;
> + }
