Messages in this thread | | | Date | Wed, 24 Nov 2021 22:41:51 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v0.9.1 3/6] sched/umcg: implement UMCG syscalls |
| |
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; > + }
| |