From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 94B80C433F5 for ; Sun, 7 Nov 2021 18:26:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id DF99B61465 for ; Sun, 7 Nov 2021 18:26:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org DF99B61465 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=posk.io Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 2D3A46B00C3; Sun, 7 Nov 2021 13:26:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 282D26B00C4; Sun, 7 Nov 2021 13:26:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 123FB6B00C5; Sun, 7 Nov 2021 13:26:47 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0169.hostedemail.com [216.40.44.169]) by kanga.kvack.org (Postfix) with ESMTP id 029D46B00C3 for ; Sun, 7 Nov 2021 13:26:46 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 8EA05180397B4 for ; Sun, 7 Nov 2021 18:26:46 +0000 (UTC) X-FDA: 78782965212.13.B7CD7DC Received: from mail-vk1-f177.google.com (mail-vk1-f177.google.com [209.85.221.177]) by imf16.hostedemail.com (Postfix) with ESMTP id 5142AF0000AD for ; Sun, 7 Nov 2021 18:26:37 +0000 (UTC) Received: by mail-vk1-f177.google.com with SMTP id a129so7158044vkb.8 for ; Sun, 07 Nov 2021 10:26:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=posk.io; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nVWxq2SX8iKkf01bcvZv4ACcZmlcmz41QXMt8ATwkIA=; b=MR4TRo5yutL/LK5jM/F5nvSqUsTI6QWE3ELz3BlUGJmkKB/aI5u5YV6v5HZxZrL/+6 ydoyPmk/FlW+jAN5fVWg3W9BXdqU+OPKkTMUJ9gBi5PXLwyfIhqx76SP0tx3coW4bHt/ 7GtsceaGk5j5NMuxwuAtyr99ZW+gMLELFKxHDg5dV/RYTs5y/cbLMUFFnVzXghybtssG O3CqaVB3yIyA4jk32qvmy9kLeArIt8AbieoVWq6hA6eQQPCivUcT+jypgnSdQIQKHN4N KajzhlVnMH9ufywqmRosqeZwSIBACaQTNuYOS2tU4ZzupyogJRDVjP9wDjmG3P1nCl0i CNYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nVWxq2SX8iKkf01bcvZv4ACcZmlcmz41QXMt8ATwkIA=; b=SYXe6988BvO3go97yCwBoNhE69acOnJcP6PBQO1+PP/zSp0B1PZErxutHMRoWVBA04 U977o0XaCiI5DD2qgFlzYv9+eXTsloosdjj0Y+jjvNo3wTWBtcX1u/NIW98XtR1iksoo 3coZFzk3jkQOxsZvTV/9raX1xt8+zIWtpdXcHvDVerOSokoowatEyyXRI6UEYh0Z+qe3 HzYoLt+91Qg/dp7o/I9EKUG3TEf4buBpSvEhTZDoxljswWGXMaPJ+Oo5Ns0iN46oCufm eUr6HYMxVv3vpIWHXGlyiJr6JavTHTlS6fxy6TbOVOQmwmExi040zeaM/3N9eVseM0+F htMA== X-Gm-Message-State: AOAM533ljhcb/tjieA0ducR6cdEaP4vLUAz5BiaQ6aHhCJ+2fDaLUC4H iPHaBVVY2w5dD4IwDdT3apbXkmlajQ5AmeLsVBqOeA== X-Google-Smtp-Source: ABdhPJyHxcwX8wYYEQn3Uxm0Zb82uJ1McE3J66N5U3GROqqJt507URRpQvPyJBjOL1lbrmfMGOwejAXwoa+lfPMV1IY= X-Received: by 2002:a1f:b64e:: with SMTP id g75mr91129659vkf.13.1636309605427; Sun, 07 Nov 2021 10:26:45 -0800 (PST) MIME-Version: 1.0 References: <20211104195804.83240-1-posk@google.com> <20211104195804.83240-4-posk@google.com> In-Reply-To: From: Peter Oskolkov Date: Sun, 7 Nov 2021 10:26:34 -0800 Message-ID: Subject: Re: [PATCH v0.8 3/6] sched/umcg: implement UMCG syscalls To: Tao Zhou Cc: Peter Zijlstra , Ingo Molnar , Thomas Gleixner , Andrew Morton , Dave Hansen , Andy Lutomirski , linux-mm@kvack.org, Linux Kernel Mailing List , linux-api@vger.kernel.org, Paul Turner , Ben Segall , Peter Oskolkov , Andrei Vagin , Jann Horn , Thierry Delisle Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 5142AF0000AD X-Stat-Signature: s38o5nt11fef4yhhhifspsbzqzhe6onn Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=posk.io header.s=google header.b=MR4TRo5y; dmarc=none; spf=pass (imf16.hostedemail.com: domain of posk@posk.io designates 209.85.221.177 as permitted sender) smtp.mailfrom=posk@posk.io X-HE-Tag: 1636309597-94033 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sat, Nov 6, 2021 at 10:19 AM Tao Zhou wrote: > > Hi Peter, > > On Thu, Nov 04, 2021 at 12:58:01PM -0700, Peter Oskolkov wrote: > > > +/** > > + * umcg_update_state: atomically update umcg_task.state_ts, set new timestamp. > > + * @state_ts - points to the state_ts member of struct umcg_task to update; > > + * @expected - the expected value of state_ts, including the timestamp; > > + * @desired - the desired value of state_ts, state part only; > > + * @may_fault - whether to use normal or _nofault cmpxchg. > > + * > > + * The function is basically cmpxchg(state_ts, expected, desired), with extra > > + * code to set the timestamp in @desired. > > + */ > > +static int umcg_update_state(u64 __user *state_ts, u64 *expected, u64 desired, > > + bool may_fault) > > +{ > > + u64 curr_ts = (*expected) >> (64 - UMCG_STATE_TIMESTAMP_BITS); > > + u64 next_ts = ktime_get_ns() >> UMCG_STATE_TIMESTAMP_GRANULARITY; > > + > > + /* Cut higher order bits. */ > > + next_ts &= UMCG_TASK_STATE_MASK_FULL; > > next_ts &= (1 << UMCG_STATE_TIMESTAMP_BITS) - 1; or am I wrong. Right, thanks. I'll fix it in the next patchset version, if any. But at the moment I don't think this is bad enough to prevent merging, if the maintainers feel like it - basically, the condition below will always be false, so if the state is updated within 16 nanoseconds, the timestamps may sometimes match. For this to be an issue, this should result in ABA updates, so two state changes should happen in 16 nanoseconds, which is extremely unlikely (impossible?), as most state changes are accompanied by other atomic ops. > > > + if (next_ts == curr_ts) > > + ++next_ts; > > + > > + /* Remove an old timestamp, if any. */ > > + desired &= UMCG_TASK_STATE_MASK_FULL; > > + > > + /* Set the new timestamp. */ > > + desired |= (next_ts << (64 - UMCG_STATE_TIMESTAMP_BITS)); > > + > > + if (may_fault) > > + return cmpxchg_user_64(state_ts, expected, desired); > > + > > + return cmpxchg_user_64_nofault(state_ts, expected, desired); > > +} > > + > > +/** > > + * sys_umcg_ctl: (un)register the current task as a UMCG task. > > + * @flags: ORed values from enum umcg_ctl_flag; see below; > > + * @self: a pointer to struct umcg_task that describes this > > + * task and governs the behavior of sys_umcg_wait if > > + * registering; must be NULL if unregistering. > > + * > > + * @flags & UMCG_CTL_REGISTER: register a UMCG task: > > + * UMCG workers: > > + * - @flags & UMCG_CTL_WORKER > > + * - self->state must be UMCG_TASK_BLOCKED > > + * UMCG servers: > > + * - !(@flags & UMCG_CTL_WORKER) > > + * - self->state must be UMCG_TASK_RUNNING > > + * > > + * All tasks: > > + * - self->next_tid must be zero > > + * > > + * If the conditions above are met, sys_umcg_ctl() immediately returns > > + * if the registered task is a server; a worker will be added to > > + * idle_workers_ptr, and the worker put to sleep; an idle server > > + * from idle_server_tid_ptr will be woken, if present. > > + * > > + * @flags == UMCG_CTL_UNREGISTER: unregister a UMCG task. If the current task > > + * is a UMCG worker, the userspace is responsible for waking its > > + * server (before or after calling sys_umcg_ctl). > > + * > > + * Return: > > + * 0 - success > > + * -EFAULT - failed to read @self > > + * -EINVAL - some other error occurred > > + */ > > +SYSCALL_DEFINE2(umcg_ctl, u32, flags, struct umcg_task __user *, self) > > +{ > > + struct umcg_task ut; > > + > > + if (flags == UMCG_CTL_UNREGISTER) { > > + if (self || !current->umcg_task) > > + return -EINVAL; > > + > > + if (current->flags & PF_UMCG_WORKER) > > + umcg_handle_exiting_worker(); > > + else > > + umcg_clear_task(current); > > + > > + return 0; > > + } > > + > > + if (!(flags & UMCG_CTL_REGISTER)) > > + return -EINVAL; > > + > > + flags &= ~UMCG_CTL_REGISTER; > > + if (flags && flags != UMCG_CTL_WORKER) > > + return -EINVAL; > > + > > + if (current->umcg_task || !self) > > + return -EINVAL; > > + > > + if (copy_from_user(&ut, self, sizeof(ut))) > > + return -EFAULT; > > + > > + if (ut.next_tid) > > + return -EINVAL; > > + > > + if (flags == UMCG_CTL_WORKER) { > > + if ((ut.state_ts & UMCG_TASK_STATE_MASK_FULL) != UMCG_TASK_BLOCKED) > > Or use UMCG_TASK_STATE_MASK that is enough. Do you have a use case for this (i.e. when state flags can be legitimately set here)? At the moment I can't think of it, and I'd rather keep things more strict to avoid dealing with unexpected use cases in the future. > > > + return -EINVAL; > > + > > + WRITE_ONCE(current->umcg_task, self); > > + current->flags |= PF_UMCG_WORKER; > > + > > + /* Trigger umcg_handle_resuming_worker() */ > > + set_tsk_thread_flag(current, TIF_NOTIFY_RESUME); > > + } else { > > + if ((ut.state_ts & UMCG_TASK_STATE_MASK_FULL) != UMCG_TASK_RUNNING) > > The same here. Yes, the same here - why do you think task state flags should be allowed here? > > > + return -EINVAL; > > + > > + WRITE_ONCE(current->umcg_task, self); > > + } > > + > > + return 0; > > +} > > + > > > Thanks, > Tao