lkml.org 
[lkml]   [2021]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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;
> + }

\
 
 \ /
  Last update: 2021-11-24 22:42    [W:1.584 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site