From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:23
As indicated earlier in the FUTEX_SWAP patchset:
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/
"Google Fibers" is a userspace scheduling framework
used widely and successfully at Google to improve in-process workload
isolation and response latencies. We are working on open-sourcing
this framework, and UMCG (User-Managed Concurrency Groups) kernel
patches are intended as the foundation of this.
This patchset is "early preview/RFC" - an earlier version of the
"core UMCG API" was discussed offlist, and I was asked to post
"what I have" to LKML before I consider the work ready for a full
review.
Notes:
- the first six patches cover "core UMCG API" and are more "ready"
than the last three, in the sense that I expect to see few, if any,
material changes to them, unless the whole approach is NACKed;
- the last three patches cover "server/worker UMCG API" and need
more work and testing:
- while I'm not aware of any specific issues with them, I have not
implemented and/or tested, yet, many important use cases, such as:
- tracing
- signals/interrupts
- explicit preemption of workers other than cooperative wait/swap
- comments/documentation is missing in many important places, or
maybe even wrong/outdated.
As such, please pay more attention to the high-level intended behavior
and design than to things like patch organization, contents of commit
messages, comments or indentation, especially in the last three patches.
Unless the feedback here points to a different approach, my next step
is to add timeout handling to sys_umcg_wait/sys_umcg_swap, as this
will open up a lot of Google-internal tests that cover most of
use/corner cases other than explicit preemption of workers (Google
Fibers use cooperative scheduling features only). Then I'll
work on issues uncovered by those tests. Then I'll address preemption
and tracing.
This work is loosely based on Google-internal SwitchTo and SwitchTo
Groups kernel patches developed by Paul Turner and Ben Segall.
Peter Oskolkov (9):
sched/umcg: add UMCG syscall stubs and CONFIG_UMCG
sched/umcg: add uapi/linux/umcg.h and sched/umcg.c
sched: add WF_CURRENT_CPU and externise ttwu
sched/umcg: implement core UMCG API
lib/umcg: implement UMCG core API for userspace
selftests/umcg: add UMCG core API selftest
sched/umcg: add UMCG server/worker API (early RFC)
lib/umcg: add UMCG server/worker API (early RFC)
selftests/umcg: add UMCG server/worker API selftest
arch/x86/entry/syscalls/syscall_64.tbl | 11 +
include/linux/mm_types.h | 5 +
include/linux/sched.h | 7 +-
include/linux/syscalls.h | 14 +
include/uapi/asm-generic/unistd.h | 25 +-
include/uapi/linux/umcg.h | 70 ++
init/Kconfig | 10 +
kernel/fork.c | 11 +
kernel/sched/Makefile | 1 +
kernel/sched/core.c | 17 +-
kernel/sched/fair.c | 4 +
kernel/sched/sched.h | 15 +-
kernel/sched/umcg.c | 1114 +++++++++++++++++
kernel/sched/umcg.h | 96 ++
kernel/sys_ni.c | 13 +
mm/init-mm.c | 4 +
tools/lib/umcg/.gitignore | 4 +
tools/lib/umcg/Makefile | 11 +
tools/lib/umcg/libumcg.c | 572 +++++++++
tools/lib/umcg/libumcg.h | 262 ++++
tools/testing/selftests/umcg/.gitignore | 3 +
tools/testing/selftests/umcg/Makefile | 15 +
tools/testing/selftests/umcg/umcg_core_test.c | 347 +++++
tools/testing/selftests/umcg/umcg_test.c | 475 +++++++
24 files changed, 3096 insertions(+), 10 deletions(-)
create mode 100644 include/uapi/linux/umcg.h
create mode 100644 kernel/sched/umcg.c
create mode 100644 kernel/sched/umcg.h
create mode 100644 tools/lib/umcg/.gitignore
create mode 100644 tools/lib/umcg/Makefile
create mode 100644 tools/lib/umcg/libumcg.c
create mode 100644 tools/lib/umcg/libumcg.h
create mode 100644 tools/testing/selftests/umcg/.gitignore
create mode 100644 tools/testing/selftests/umcg/Makefile
create mode 100644 tools/testing/selftests/umcg/umcg_core_test.c
create mode 100644 tools/testing/selftests/umcg/umcg_test.c
--
2.31.1.818.g46aad6cb9e-goog
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:28
User Managed Concurrency Groups is a fast context switching and
in-process userspace scheduling framework.
Two main use cases are security sandboxes and userspace scheduling.
Security sandboxes: fast X-process context switching will open up a
bunch of light-weight security tools, e.g. gVisor, or Tor Project's
Shadow simulator, to more use cases.
In-process userspace scheduling is used extensively at Google to provide
latency control and isolation guarantees for diverse workloads while
maintaining high CPU utilization.
Signed-off-by: Peter Oskolkov <redacted>
---
arch/x86/entry/syscalls/syscall_64.tbl | 11 +++++++++++
include/uapi/asm-generic/unistd.h | 25 ++++++++++++++++++++++++-
init/Kconfig | 10 ++++++++++
kernel/sys_ni.c | 13 +++++++++++++
4 files changed, 58 insertions(+), 1 deletion(-)
@@ -368,6 +368,17 @@ 444 common landlock_create_ruleset sys_landlock_create_ruleset 445 common landlock_add_rule sys_landlock_add_rule 446 common landlock_restrict_self sys_landlock_restrict_self+447 common umcg_api_version sys_umcg_api_version+448 common umcg_register_task sys_umcg_register_task+449 common umcg_unregister_task sys_umcg_unregister_task+450 common umcg_wait sys_umcg_wait+451 common umcg_wake sys_umcg_wake+452 common umcg_swap sys_umcg_swap+453 common umcg_create_group sys_umcg_create_group+454 common umcg_destroy_group sys_umcg_destroy_group+455 common umcg_poll_worker sys_umcg_poll_worker+456 common umcg_run_worker sys_umcg_run_worker+457 common umcg_preempt_worker sys_umcg_preempt_worker # # Due to a historical design error, certain syscalls are numbered differently
@@ -1661,6 +1661,16 @@ config MEMBARRIERIfunsure,sayY.+configUMCG+bool"Enable User Managed Concurrency Groups API"+defaultn+help+EnableUMCGcorewait/wake/swapoperationsaswellasUMCG+group/server/workerAPI.ThecoreAPIisusefulforfastIPC+andcontextswitching,whilethegroup/server/workerAPI,together+withthecoreAPI,formthebasisforanin-processM:Nuserspace+schedulingframeworkimplementedinlib/umcg.+configKALLSYMSbool"Load all symbols for debugging/ksymoops"ifEXPERTdefaulty
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:34
Introduce the uapi UMCG header file and document core UMCG API syscalls.
It is sometimes useful to separate the discussion of API from
the implementation details, and it seems to be the case here.
Signed-off-by: Peter Oskolkov <redacted>
---
include/linux/syscalls.h | 9 +++
include/uapi/linux/umcg.h | 70 +++++++++++++++++++
kernel/sched/Makefile | 1 +
kernel/sched/umcg.c | 143 ++++++++++++++++++++++++++++++++++++++
4 files changed, 223 insertions(+)
create mode 100644 include/uapi/linux/umcg.h
create mode 100644 kernel/sched/umcg.c
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:38
Add WF_CURRENT_CPU wake flag that advices the scheduler to
move the wakee to the current CPU. This is useful for fast on-CPU
context switching use cases such as UMCG.
In addition, make ttwu external rather than static so that
the flag could be passed to it from outside of sched/core.c.
Signed-off-by: Peter Oskolkov <redacted>
---
kernel/sched/core.c | 3 +--
kernel/sched/fair.c | 4 ++++
kernel/sched/sched.h | 15 +++++++++------
3 files changed, 14 insertions(+), 8 deletions(-)
@@ -2027,13 +2027,14 @@ static inline int task_on_rq_migrating(struct task_struct *p)}/* Wake flags. The first three directly map to some SD flag value */-#define WF_EXEC 0x02 /* Wakeup after exec; maps to SD_BALANCE_EXEC */-#define WF_FORK 0x04 /* Wakeup after fork; maps to SD_BALANCE_FORK */-#define WF_TTWU 0x08 /* Wakeup; maps to SD_BALANCE_WAKE */+#define WF_EXEC 0x02 /* Wakeup after exec; maps to SD_BALANCE_EXEC */+#define WF_FORK 0x04 /* Wakeup after fork; maps to SD_BALANCE_FORK */+#define WF_TTWU 0x08 /* Wakeup; maps to SD_BALANCE_WAKE */-#define WF_SYNC 0x10 /* Waker goes to sleep after wakeup */-#define WF_MIGRATED 0x20 /* Internal use, task got migrated */-#define WF_ON_CPU 0x40 /* Wakee is on_cpu */+#define WF_SYNC 0x10 /* Waker goes to sleep after wakeup */+#define WF_MIGRATED 0x20 /* Internal use, task got migrated */+#define WF_ON_CPU 0x40 /* Wakee is on_cpu */+#define WF_CURRENT_CPU 0x80 /* Prefer to move the wakee to the current CPU. */#ifdef CONFIG_SMPstatic_assert(WF_EXEC==SD_BALANCE_EXEC);
@@ -0,0 +1,350 @@+// SPDX-License-Identifier: GPL-2.0+#define _GNU_SOURCE+#include"libumcg.h"++#include<errno.h>+#include<pthread.h>+#include<signal.h>+#include<stdatomic.h>+#include<stdbool.h>+#include<stdio.h>+#include<stdlib.h>+#include<threads.h>++/* UMCG API version supported by this library. */+staticconstuint32_tumcg_api_version=1;++structumcg_group{+uint32_tgroup_id;+};++/**+*structumcg_task_tls-perthreadstructusedtoidentify/manageUMCGtasks+*+*EachUMCGtaskrequiresaninstanceofstructumcg_taskpassedto+*sys_umcg_register.Thisstructcontainsit,aswellasseveraladditional+*fields.+*/+structumcg_task_tls{+structumcg_taskumcg_task;+umcg_tidself;+intptr_ttag;+pid_ttid;++}__attribute((aligned(4*sizeof(uint64_t))));++staticthread_localstructumcg_task_tls*umcg_task_tls;++umcg_tidumcg_get_utid(void)+{+return(umcg_tid)&umcg_task_tls;+}++staticumcg_tidumcg_task_to_utid(structumcg_task*ut)+{+if(!ut)+returnUMCG_NONE;++return((structumcg_task_tls*)ut)->self;+}++staticstructumcg_task_tls*utid_to_utls(umcg_tidutid)+{+if(!utid||!*(structumcg_task_tls**)utid){+fprintf(stderr,"utid_to_utls: NULL\n");+/* Kill the process rather than corrupt memory. */+raise(SIGKILL);+returnNULL;+}+return*(structumcg_task_tls**)utid;+}++voidumcg_set_task_tag(umcg_tidutid,intptr_ttag)+{+utid_to_utls(utid)->tag=tag;+}++intptr_tumcg_get_task_tag(umcg_tidutid)+{+returnutid_to_utls(utid)->tag;+}++umcg_tidumcg_register_core_task(intptr_ttag)+{+intret;++if(umcg_task_tls!=NULL){+errno=EINVAL;+returnUMCG_NONE;+}++umcg_task_tls=malloc(sizeof(structumcg_task_tls));+if(!umcg_task_tls){+errno=ENOMEM;+returnUMCG_NONE;+}++umcg_task_tls->umcg_task.state=UMCG_TASK_NONE;+umcg_task_tls->self=(umcg_tid)&umcg_task_tls;+umcg_task_tls->tag=tag;+umcg_task_tls->tid=gettid();++ret=sys_umcg_register_task(umcg_api_version,UMCG_REGISTER_CORE_TASK,+UMCG_NOID,&umcg_task_tls->umcg_task);+if(ret){+free(umcg_task_tls);+umcg_task_tls=NULL;+errno=ret;+returnUMCG_NONE;+}++returnumcg_task_tls->self;+}++intumcg_unregister_task(void)+{+intret;++if(!umcg_task_tls){+errno=EINVAL;+return-1;+}++ret=sys_umcg_unregister_task(0);+if(ret){+errno=ret;+return-1;+}++free(umcg_task_tls);+atomic_store_explicit(&umcg_task_tls,NULL,memory_order_seq_cst);+return0;+}++/* Helper return codes. */+enumumcg_prepare_op_result{+UMCG_OP_DONE,+UMCG_OP_SYS,+UMCG_OP_AGAIN,+UMCG_OP_ERROR+};++staticenumumcg_prepare_op_resultumcg_prepare_wait(void)+{+structumcg_task*ut;+uint32_tumcg_state;+intret;++if(!umcg_task_tls){+errno=EINVAL;+returnUMCG_OP_ERROR;+}++ut=&umcg_task_tls->umcg_task;++umcg_state=UMCG_TASK_RUNNING;+if(atomic_compare_exchange_strong_explicit(&ut->state,+&umcg_state,UMCG_TASK_RUNNABLE,+memory_order_seq_cst,memory_order_seq_cst))+returnUMCG_OP_SYS;++if(umcg_state!=(UMCG_TASK_RUNNING|UMCG_TF_WAKEUP_QUEUED)){+fprintf(stderr,"libumcg: unexpected state before wait: %u\n",+umcg_state);+errno=EINVAL;+returnUMCG_OP_ERROR;+}++if(atomic_compare_exchange_strong_explicit(&ut->state,+&umcg_state,UMCG_TASK_RUNNING,+memory_order_seq_cst,memory_order_seq_cst)){+returnUMCG_OP_DONE;+}++/* Raced with another wait/wake? This is not supported. */+fprintf(stderr,"libumcg: failed to remove the wakeup flag: %u\n",+umcg_state);+errno=EINVAL;+returnUMCG_OP_ERROR;+}++staticintumcg_do_wait(conststructtimespec*timeout)+{+uint32_tumcg_state;+intret;++do{+ret=sys_umcg_wait(0,timeout);+if(ret!=0&&errno!=EAGAIN)+returnret;++umcg_state=atomic_load_explicit(+&umcg_task_tls->umcg_task.state,+memory_order_acquire);+}while(umcg_state==UMCG_TASK_RUNNABLE);++return0;+}++intumcg_wait(conststructtimespec*timeout)+{+switch(umcg_prepare_wait()){+caseUMCG_OP_DONE:+return0;+caseUMCG_OP_SYS:+break;+caseUMCG_OP_ERROR:+return-1;+default:+fprintf(stderr,"Unknown pre_op result.\n");+exit(1);+return-1;+}++returnumcg_do_wait(timeout);+}++staticenumumcg_prepare_op_resultumcg_prepare_wake(structumcg_task_tls*utls)+{+structumcg_task*ut=&utls->umcg_task;+uint32_tumcg_state,next_state;++next_state=UMCG_TASK_RUNNING;+umcg_state=UMCG_TASK_RUNNABLE;+if(atomic_compare_exchange_strong_explicit(&ut->state,+&umcg_state,next_state,+memory_order_seq_cst,memory_order_seq_cst))+returnUMCG_OP_SYS;++if(umcg_state!=UMCG_TASK_RUNNING){+if(umcg_state==(UMCG_TASK_RUNNING|UMCG_TF_WAKEUP_QUEUED)){+/*+*Withping-pongmutualswappingusingwake/wait+*withoutsynchronizationthiscanhappen.+*/+returnUMCG_OP_AGAIN;+}+fprintf(stderr,"libumcg: unexpected state in umcg_wake(): %u\n",+umcg_state);+errno=EINVAL;+returnUMCG_OP_ERROR;+}++if(atomic_compare_exchange_strong_explicit(&ut->state,+&umcg_state,UMCG_TASK_RUNNING|UMCG_TF_WAKEUP_QUEUED,+memory_order_seq_cst,memory_order_seq_cst)){+returnUMCG_OP_DONE;+}++if(umcg_state!=UMCG_TASK_RUNNABLE){+fprintf(stderr,"libumcg: unexpected state in umcg_wake (1): %u\n",+umcg_state);+errno=EINVAL;+returnUMCG_OP_ERROR;+}++returnUMCG_OP_AGAIN;+}++staticintumcg_do_wake_or_swap(structumcg_task_tls*next_utls,+uint64_tprev_wait_counter,boolshould_wait,+conststructtimespec*timeout)+{+intret;++again:++if(should_wait)+ret=sys_umcg_swap(0,next_utls->tid,0,timeout);+else+ret=sys_umcg_wake(0,next_utls->tid);++if(ret&&errno==EAGAIN)+gotoagain;++returnret;+}++intumcg_wake(umcg_tidnext)+{+structumcg_task_tls*utls=*(structumcg_task_tls**)next;+uint64_tprev_wait_counter;++if(!utls){+errno=EINVAL;+return-1;+}++again:+switch(umcg_prepare_wake(utls)){+caseUMCG_OP_DONE:+return0;+caseUMCG_OP_SYS:+break;+caseUMCG_OP_ERROR:+return-1;+caseUMCG_OP_AGAIN:+gotoagain;+default:+fprintf(stderr,"libumcg: unknown pre_op result.\n");+exit(1);+return-1;+}++returnumcg_do_wake_or_swap(utls,prev_wait_counter,false,NULL);+}++intumcg_swap(umcg_tidnext,conststructtimespec*timeout)+{+structumcg_task_tls*utls=*(structumcg_task_tls**)next;+boolshould_wake,should_wait;+uint64_tprev_wait_counter;+intret;++if(!utls){+errno=EINVAL;+return-1;+}++again:+switch(umcg_prepare_wake(utls)){+caseUMCG_OP_DONE:+should_wake=false;+break;+caseUMCG_OP_SYS:+should_wake=true;+break;+caseUMCG_OP_ERROR:+return-1;+caseUMCG_OP_AGAIN:+gotoagain;+default:+fprintf(stderr,"lubumcg: unknown pre_op result.\n");+exit(1);+return-1;+}++switch(umcg_prepare_wait()){+caseUMCG_OP_DONE:+should_wait=false;+break;+caseUMCG_OP_SYS:+should_wait=true;+break;+caseUMCG_OP_ERROR:+return-1;+default:+fprintf(stderr,"lubumcg: unknown pre_op result.\n");+exit(1);+return-1;+}++if(should_wake)+returnumcg_do_wake_or_swap(utls,prev_wait_counter,+should_wait,timeout);++if(should_wait)+returnumcg_do_wait(timeout);++return0;+}
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:49
Implement version 1 of core UMCG API (wait/wake/swap).
As has been outlined in
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/,
efficient and synchronous on-CPU context switching is key
to enabling two broad use cases: in-process M:N userspace scheduling
and fast X-process RPCs for security wrappers.
High-level design considerations/approaches used:
- wait & wake can race with each other;
- offload as much work as possible to libumcg in tools/lib/umcg,
specifically:
- most state changes, e.g. RUNNABLE <=> RUNNING, are done in
the userspace (libumcg);
- retries are offloaded to the userspace.
This implementation misses timeout handling in sys_umcg_wait
and sys_umcg_swap, which will be added in version 2.
Signed-off-by: Peter Oskolkov <redacted>
---
include/linux/sched.h | 7 +-
kernel/sched/core.c | 3 +
kernel/sched/umcg.c | 237 ++++++++++++++++++++++++++++++++++++++++--
kernel/sched/umcg.h | 42 ++++++++
4 files changed, 282 insertions(+), 7 deletions(-)
create mode 100644 kernel/sched/umcg.h
@@ -1022,7 +1027,7 @@ struct task_struct {u64parent_exec_id;u64self_exec_id;-/* Protection against (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed, mempolicy: */+/* Protection against (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed, mempolicy, umcg: */spinlock_talloc_lock;/* Protection of the PI data structures: */
@@ -0,0 +1,347 @@+// SPDX-License-Identifier: GPL-2.0+#define _GNU_SOURCE+#include"libumcg.h"++#include<pthread.h>+#include<stdatomic.h>++#include"../kselftest_harness.h"++#define CHECK_CONFIG() \+{\+intret=sys_umcg_api_version(1,0);\+if(ret==-1&&errno==ENOSYS)\+SKIP(return,"CONFIG_UMCG not set");\+}++TEST(umcg_api_version){+CHECK_CONFIG();+ASSERT_EQ(0,sys_umcg_api_version(1,0));+ASSERT_EQ(1,sys_umcg_api_version(1234,0));+}++/* Test that forked children of UMCG enabled tasks are not UMCG enabled. */+TEST(register_and_fork){+CHECK_CONFIG();+pid_tpid;+intwstatus;+umcg_tidutid;++/* umcg_unregister should fail without registering earlier. */+ASSERT_NE(0,umcg_unregister_task());++utid=umcg_register_core_task(0);+ASSERT_TRUE(utid!=UMCG_NONE);++pid=fork();+if(pid==0){+/* This is child. umcg_unregister_task() should fail. */+if(!umcg_unregister_task()){+fprintf(stderr,"umcg_unregister_task() succeeded in "+"the forked child.\n");+exit(1);+}+exit(0);+}++ASSERT_EQ(pid,waitpid(pid,&wstatus,0));+ASSERT_TRUE(WIFEXITED(wstatus));+ASSERT_EQ(0,WEXITSTATUS(wstatus));+ASSERT_EQ(0,umcg_unregister_task());+}++structtest_waiter_args{+umcg_tidutid;+boolstop;+boolwaiting;+};++/* Thread FN for the test waiter: calls umcg_wait() in a loop until stopped. */+staticvoid*test_waiter_threadfn(void*arg)+{+structtest_waiter_args*args=(structtest_waiter_args*)arg;+uint64_tcounter=0;++atomic_store_explicit(&args->utid,umcg_register_core_task(0),+memory_order_relaxed);+if(!args->utid){+fprintf(stderr,"umcg_register_core_task failed: %d.\n",errno);+exit(1);+}++while(!atomic_load_explicit(&args->stop,memory_order_seq_cst)){+boolexpected=false;++if(!atomic_compare_exchange_strong_explicit(&args->waiting,+&expected,true,+memory_order_seq_cst,+memory_order_seq_cst)){+fprintf(stderr,"Failed to set waiting flag.\n");+exit(1);+}++++counter;+if(counter%5==0)+usleep(1);/* Trigger a race with ucmg_wake(). */++if(umcg_wait(NULL)){+fprintf(stderr,"umcg_wait failed: %d.\n",errno);+exit(1);+}+}++if(umcg_unregister_task()){+fprintf(stderr,"umcg_register_core_task failed: %d.\n",errno);+exit(1);+}++return(void*)counter;+}++/* Test wake/wait pair racing with each other. */+TEST(umcg_wake_wait){+CHECK_CONFIG();+structtest_waiter_argsargs;+constintsteps=10000;+boolexpected=true;+void*result;+pthread_tt;+intret;++args.utid=UMCG_NONE;+args.stop=false;+args.waiting=false;++ASSERT_EQ(0,pthread_create(&t,NULL,&test_waiter_threadfn,&args));++while(!atomic_load_explicit(&args.utid,memory_order_relaxed))+;++for(intstep=0;step<steps;++step){+/* Spin until the waiter indicates it is going to wait. */+while(!atomic_compare_exchange_weak_explicit(&args.waiting,+&expected,false,+memory_order_seq_cst,+memory_order_seq_cst)){+expected=true;+}++ASSERT_EQ(0,umcg_wake(args.utid));+}++/* Carefully shut down. */+expected=true;+while(!atomic_compare_exchange_weak_explicit(&args.waiting,&expected,+false,memory_order_seq_cst,memory_order_seq_cst)){+expected=true;+}+atomic_store_explicit(&args.stop,true,memory_order_seq_cst);+ret=umcg_wake(args.utid);++/* If the worker immediately exits upon wake, we may get ESRCH. */+ASSERT_TRUE((ret==0)||(errno==ESRCH));++ASSERT_EQ(0,pthread_join(t,&result));+ASSERT_EQ(steps+1,(uint64_t)result);+}++structtest_ping_pong_args{+boolping;/* Is this worker doing pings or pongs? */+umcg_tidutid_self;+umcg_tidutid_peer;+intsteps;+booluse_swap;/* Use umcg_swap or wake/wait. */+boolpayload;/* call gettid() if true at each iteration. */++/*+*Itisnotallowedtowakeataskthathasawakeupqueued,so+*normallythetest"softly"synchronizespingandpongtasksso+*thatpongcallsumcg_wait()towaitforthefirstping.+*+*However,itisallowedtodomutualumcg_swap(),sointhe+*testflavorwhenbothpingandpongtasksuseswapswealso+*runthetestwithoutpongwaitingfortheinitialping.+*/+boolpong_waits;+};++/* Thread FN for ping-pong workers. */+staticvoid*test_ping_pong_threadfn(void*arg)+{+structtest_ping_pong_args*args=(structtest_ping_pong_args*)arg;+structtimespecstart,stop;+intcounter;++atomic_store_explicit(&args->utid_self,umcg_register_core_task(0),+memory_order_relaxed);+if(!args->utid_self){+fprintf(stderr,"umcg_register_core_task failed: %d.\n",errno);+exit(1);+}++while(!atomic_load_explicit(&args->utid_peer,memory_order_acquire))+;++if(args->pong_waits&&!args->ping){+/* This is pong: we sleep first. */+if(umcg_wait(NULL)){+fprintf(stderr,"umcg_wait failed: %d.\n",errno);+exit(1);+}+}++if(args->ping){/* The "ping" measures the running time. */+if(clock_gettime(CLOCK_MONOTONIC,&start)){+fprintf(stderr,"clock_gettime() failed.\n");+exit(1);+}+}++for(counter=0;counter<args->steps;++counter){+intret;++if(args->payload)+gettid();++if(args->use_swap){+ret=umcg_swap(args->utid_peer,NULL);+}else{+ret=umcg_wake(args->utid_peer);+if(!ret)+ret=umcg_wait(NULL);+}++if(ret){+if(args->use_swap)+fprintf(stderr,"umcg_swap failed: %d.\n",errno);+else+fprintf(stderr,"umcg_wake/wait failed: %d.\n",errno);+exit(1);+}+}++if(args->ping){+uint64_tduration;++if(clock_gettime(CLOCK_MONOTONIC,&stop)){+fprintf(stderr,"clock_gettime() failed.\n");+exit(1);+}++duration=(stop.tv_sec-start.tv_sec)*1000000000LL++stop.tv_nsec-start.tv_nsec;+printf("completed %d ping-pong iterations in %lu ns: "+"%lu ns per context switch\n",+args->steps,duration,duration/(args->steps*2));+}++if(args->pong_waits&&args->ping){+/* This is ping: we wake pong at the end. */+if(umcg_wake(args->utid_peer)){+fprintf(stderr,"umcg_wake failed: %d.\n",errno);+exit(1);+}+}++if(umcg_unregister_task()){+fprintf(stderr,"umcg_unregister_task failed: %d.\n",errno);+exit(1);+}++returnNULL;+}++enumping_pong_flavor{+NO_SWAPS,/* Use wake/wait pairs on both sides. */+ONE_SWAP,/* Use wake/wait on one side and swap on the other. */+ALL_SWAPS/* Use swaps on both sides. */+};++staticvoidtest_ping_pong_flavored(enumping_pong_flavorflavor,+boolpong_waits,boolpayload)+{+structtest_ping_pong_argsping,pong;+pthread_tping_t,pong_t;+constintSTEPS=100000;++ping.ping=true;+ping.utid_self=UMCG_NONE;+ping.utid_peer=UMCG_NONE;+ping.steps=STEPS;+ping.pong_waits=pong_waits;+ping.payload=payload;++pong.ping=false;+pong.utid_self=UMCG_NONE;+pong.utid_peer=UMCG_NONE;+pong.steps=STEPS;+pong.pong_waits=pong_waits;+pong.payload=payload;++switch(flavor){+caseNO_SWAPS:+ping.use_swap=false;+pong.use_swap=false;+break;+caseONE_SWAP:+ping.use_swap=true;+pong.use_swap=false;+break;+caseALL_SWAPS:+ping.use_swap=true;+pong.use_swap=true;+break;+default:+fprintf(stderr,"Unknown ping/pong flavor.\n");+exit(1);+}++if(pthread_create(&ping_t,NULL,&test_ping_pong_threadfn,&ping)){+fprintf(stderr,"pthread_create(ping) failed.\n");+exit(1);+}++while(!atomic_load_explicit(&ping.utid_self,memory_order_relaxed))+;+pong.utid_peer=ping.utid_self;++if(pthread_create(&pong_t,NULL,&test_ping_pong_threadfn,&pong)){+fprintf(stderr,"pthread_create(pong) failed.\n");+exit(1);+}++while(!atomic_load_explicit(&pong.utid_self,memory_order_relaxed))+;+atomic_store_explicit(&ping.utid_peer,pong.utid_self,+memory_order_relaxed);++pthread_join(ping_t,NULL);+pthread_join(pong_t,NULL);+}++TEST(umcg_ping_pong_no_swaps_nop){+CHECK_CONFIG();+test_ping_pong_flavored(NO_SWAPS,true,false);+}+TEST(umcg_ping_pong_one_swap_nop){+CHECK_CONFIG();+test_ping_pong_flavored(ONE_SWAP,true,false);+}+TEST(umcg_ping_pong_all_swaps_nop){+CHECK_CONFIG();+test_ping_pong_flavored(ALL_SWAPS,true,false);+}+TEST(umcg_ping_pong_all_swaps_loose_nop){+CHECK_CONFIG();+test_ping_pong_flavored(ALL_SWAPS,false,false);+}+TEST(umcg_ping_pong_no_swaps_payload){+CHECK_CONFIG();+test_ping_pong_flavored(NO_SWAPS,true,true);+}+TEST(umcg_ping_pong_all_swaps_payload){+CHECK_CONFIG();+test_ping_pong_flavored(ALL_SWAPS,true,true);+}++TEST_HARNESS_MAIN
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:53
Implement UMCG server/worker API.
This is an early RFC patch - the code seems working, but
more testing is needed. Gaps I plan to address before this
is ready for a detailed review:
- preemption/interrupt handling;
- better documentation/comments;
- tracing;
- additional testing;
- corner cases like abnormal process/task termination;
- in some cases where I kill the task (umcg_segv), returning
an error may be more appropriate.
All in all, please focus more on the high-level approach
and less on things like variable names, (doc) comments, or indentation.
Signed-off-by: Peter Oskolkov <redacted>
---
include/linux/mm_types.h | 5 +
include/linux/syscalls.h | 5 +
kernel/fork.c | 11 +
kernel/sched/core.c | 11 +
kernel/sched/umcg.c | 764 ++++++++++++++++++++++++++++++++++++++-
kernel/sched/umcg.h | 54 +++
mm/init-mm.c | 4 +
7 files changed, 845 insertions(+), 9 deletions(-)
@@ -86,6 +165,105 @@ static int register_core_task(u32 api_version, struct umcg_task __user *umcg_tasreturn0;}+staticintadd_task_to_group(u32api_version,u32group_id,+structumcg_task__user*umcg_task,+enumumcg_task_typetask_type,u32new_state)+{+structmm_struct*mm=current->mm;+structumcg_task_data*utd=NULL;+structumcg_group*group=NULL;+structumcg_group*list_entry;+intret=-EINVAL;+u32state;++if(get_state(umcg_task,&state))+return-EFAULT;++if(state!=UMCG_TASK_NONE)+return-EINVAL;++if(put_state(umcg_task,new_state))+return-EFAULT;++retry_once:+rcu_read_lock();+list_for_each_entry_rcu(list_entry,&mm->umcg_groups,list){+if(list_entry->group_id==group_id){+group=list_entry;+break;+}+}++if(!group||group->api_version!=api_version)+gotoout_rcu;++spin_lock(&group->lock);+if(group->nr_tasks<0)/* The groups is being destroyed. */+gotoout_group;++if(!utd){+utd=kzalloc(sizeof(structumcg_task_data),GFP_NOWAIT);+if(!utd){+spin_unlock(&group->lock);+rcu_read_unlock();++utd=kzalloc(sizeof(structumcg_task_data),GFP_KERNEL);+if(!utd){+ret=-ENOMEM;+gotoout;+}++gotoretry_once;+}+}++utd->self=current;+utd->group=group;+utd->umcg_task=umcg_task;+utd->task_type=task_type;+utd->api_version=api_version;+RCU_INIT_POINTER(utd->peer,NULL);++INIT_LIST_HEAD(&utd->list);+group->nr_tasks++;++task_lock(current);+rcu_assign_pointer(current->umcg_task_data,utd);+task_unlock(current);++ret=0;++out_group:+spin_unlock(&group->lock);++out_rcu:+rcu_read_unlock();+if(ret&&utd)+kfree(utd);++out:+if(ret)+put_state(umcg_task,UMCG_TASK_NONE);+else+schedule();/* Trigger umcg_on_wake(). */++returnret;+}++staticintregister_worker(u32api_version,u32group_id,+structumcg_task__user*umcg_task)+{+returnadd_task_to_group(api_version,group_id,umcg_task,+UMCG_TT_WORKER,UMCG_TASK_UNBLOCKED);+}++staticintregister_server(u32api_version,u32group_id,+structumcg_task__user*umcg_task)+{+returnadd_task_to_group(api_version,group_id,umcg_task,+UMCG_TT_SERVER,UMCG_TASK_PROCESSING);+}+/***sys_umcg_register_task-registerthecurrenttaskasaUMCGtask.*@api_version:Theexpected/desiredAPIversionofthesyscall.
@@ -358,6 +613,25 @@ SYSCALL_DEFINE4(umcg_swap, u32, wake_flags, u32, next_tid, u32, wait_flags,gotoout;}+/* Move the server from curr to next, if appropriate. */+if(curr_utd->task_type==UMCG_TT_WORKER){+structtask_struct*server=rcu_dereference(curr_utd->peer);+if(server){+structumcg_task_data*server_utd=+rcu_dereference(server->umcg_task_data);++if(rcu_access_pointer(next_utd->peer)){+ret=-EAGAIN;+gotoout;+}+umcg_detach_peer();+umcg_lock_pair(server,next);+rcu_assign_pointer(server_utd->peer,next);+rcu_assign_pointer(next_utd->peer,server);+umcg_unlock_pair(server,next);+}+}+rcu_read_unlock();returndo_context_switch(next);
@@ -366,3 +640,475 @@ SYSCALL_DEFINE4(umcg_swap, u32, wake_flags, u32, next_tid, u32, wait_flags,rcu_read_unlock();returnret;}++/**+*sys_umcg_create_group-createaUMCGgroup+*@api_version:RequestedAPIversion.+*@flags:Reserved.+*+*Return:+*>=0-thegroupID+*-EOPNOTSUPP-@api_versionisnotsupported+*-EINVAL-@flagsisnotvalid+*-ENOMEM-notenoughmemory+*/+SYSCALL_DEFINE2(umcg_create_group,u32,api_version,u64,flags)+{+intret;+structumcg_group*group;+structumcg_group*list_entry;+structmm_struct*mm=current->mm;++if(flags)+return-EINVAL;++if(__api_version(api_version))+return-EOPNOTSUPP;++group=kzalloc(sizeof(structumcg_group),GFP_KERNEL);+if(!group)+return-ENOMEM;++spin_lock_init(&group->lock);+INIT_LIST_HEAD(&group->list);+INIT_LIST_HEAD(&group->waiters);+group->flags=flags;+group->api_version=api_version;++spin_lock(&mm->umcg_lock);++list_for_each_entry_rcu(list_entry,&mm->umcg_groups,list){+if(list_entry->group_id>=group->group_id)+group->group_id=list_entry->group_id+1;+}++list_add_rcu(&mm->umcg_groups,&group->list);++ret=group->group_id;+spin_unlock(&mm->umcg_lock);++returnret;+}++/**+*sys_umcg_destroy_group-destroyaUMCGgroup+*@group_id:TheIDofthegrouptodestroy.+*+*Thegroupmustbeempty,i.e.havenoregisteredserversorworkers.+*+*Return:+*0-success;+*-ESRCH-groupnotfound;+*-EBUSY-thegrouphasregisteredworkersorservers.+*/+SYSCALL_DEFINE1(umcg_destroy_group,u32,group_id)+{+intret=0;+structumcg_group*group=NULL;+structumcg_group*list_entry;+structmm_struct*mm=current->mm;++spin_lock(&mm->umcg_lock);+list_for_each_entry_rcu(list_entry,&mm->umcg_groups,list){+if(list_entry->group_id==group_id){+group=list_entry;+break;+}+}++if(group==NULL){+ret=-ESRCH;+gotoout;+}++spin_lock(&group->lock);++if(group->nr_tasks>0){+ret=-EBUSY;+spin_unlock(&group->lock);+gotoout;+}++/* Tell group rcu readers that the group is going to be deleted. */+group->nr_tasks=-1;++spin_unlock(&group->lock);++list_del_rcu(&group->list);+kfree_rcu(group,rcu);++out:+spin_unlock(&mm->umcg_lock);+returnret;+}++/**+*sys_umcg_poll_worker-pollanUNBLOCKEDworker+*@flags:reserved;+*@ut:thecontrolstructumcg_taskofthepolledworker.+*+*ThecurrenttaskmustbeaUMCGserverinPOLLINGstate;ifthereare+*UNBLOCKEDworkersintheserver'sgroup,taketheearliestqueued,+*marktheworkerasRUNNABLE.andreturn.+*+*Iftherearenounblockedworkers,thesyscallwaitsforonetobecome+*available.+*+*Return:+*0-Ok;+*-EINTR-asignalwasreceived;+*-EINVAL-oneoftheparametersiswrong,orapreconditionwasnotmet.+*/+SYSCALL_DEFINE2(umcg_poll_worker,u32,flags,structumcg_task__user**,ut)+{+structumcg_group*group;+structtask_struct*worker;+structtask_struct*server=current;+structumcg_task__user*result;+structumcg_task_data*worker_utd,*server_utd;++if(flags)+return-EINVAL;++rcu_read_lock();++server_utd=rcu_dereference(server->umcg_task_data);++if(!server_utd||server_utd->task_type!=UMCG_TT_SERVER){+rcu_read_unlock();+return-EINVAL;+}++umcg_detach_peer();++group=server_utd->group;++spin_lock(&group->lock);++if(group->nr_waiting_workers==0){/* Queue the server. */+++group->nr_waiting_pollers;+list_add_tail(&server_utd->list,&group->waiters);+set_current_state(TASK_INTERRUPTIBLE);+spin_unlock(&group->lock);+rcu_read_unlock();++freezable_schedule();++rcu_read_lock();+server_utd=rcu_dereference(server->umcg_task_data);++if(!list_empty(&server_utd->list)){+spin_lock(&group->lock);+list_del_init(&server_utd->list);+--group->nr_waiting_pollers;+spin_unlock(&group->lock);+}++if(signal_pending(current)){+rcu_read_unlock();+return-EINTR;+}++worker=rcu_dereference(server_utd->peer);+if(worker){+worker_utd=rcu_dereference(worker->umcg_task_data);+result=worker_utd->umcg_task;+}else+result=NULL;++rcu_read_unlock();++if(put_user(result,ut))+returnumcg_segv(-EFAULT);+return0;+}++/* Pick up the first worker. */+worker_utd=list_first_entry(&group->waiters,structumcg_task_data,+list);+list_del_init(&worker_utd->list);+worker=worker_utd->self;+--group->nr_waiting_workers;++umcg_lock_pair(server,worker);+spin_unlock(&group->lock);++if(WARN_ON(rcu_access_pointer(server_utd->peer)||+rcu_access_pointer(worker_utd->peer))){+/* This is unexpected. */+rcu_read_unlock();+returnumcg_segv(-EINVAL);+}+rcu_assign_pointer(server_utd->peer,worker);+rcu_assign_pointer(worker_utd->peer,current);++umcg_unlock_pair(server,worker);++result=worker_utd->umcg_task;+rcu_read_unlock();++if(put_state(result,UMCG_TASK_RUNNABLE))+returnumcg_segv(-EFAULT);++if(put_user(result,ut))+returnumcg_segv(-EFAULT);++return0;+}++/**+*sys_umcg_run_worker-"run"aRUNNABLEworkerasaserver+*@flags:reserved;+*@worker_tid:tidoftheworkertorun;+*@ut:thecontrolstructumcg_taskoftheworkerthatblocked+*duringthis"run".+*+*TheworkermustbeinRUNNABLEstate.Theserver(=currenttask)+*wakestheworkerandblocks;whentheworker,oroneoftheworkers+*inumcg_swapchain,blocks,theserveriswokenandthesyscallreturns+*withutindicatingtheblockedworker.+*+*Iftheworkerexitsorunregistersitself,thesyscallsucceedswith+*ut==NULL.+*+*Return:+*0-Ok;+*-EINTR-asignalwasreceived;+*-EINVAL-oneoftheparametersiswrong,orapreconditionwasnotmet.+*/+SYSCALL_DEFINE3(umcg_run_worker,u32,flags,u32,worker_tid,+structumcg_task__user**,ut)+{+intret=-EINVAL;+structtask_struct*worker;+structtask_struct*server=current;+structumcg_task__user*result=NULL;+structumcg_task_data*worker_utd;+structumcg_task_data*server_utd;+structumcg_task__user*server_ut;+structumcg_task__user*worker_ut;++if(!ut)+return-EINVAL;++rcu_read_lock();+server_utd=rcu_dereference(server->umcg_task_data);++if(!server_utd||server_utd->task_type!=UMCG_TT_SERVER)+gotoout_rcu;++if(flags)+gotoout_rcu;++worker=find_get_task_by_vpid(worker_tid);+if(!worker){+ret=-ESRCH;+gotoout_rcu;+}++worker_utd=rcu_dereference(worker->umcg_task_data);+if(!worker_utd)+gotoout_rcu;++if(!READ_ONCE(worker_utd->in_wait)){+ret=-EAGAIN;+gotoout_rcu;+}++if(server_utd->group!=worker_utd->group)+gotoout_rcu;++if(rcu_access_pointer(server_utd->peer)!=worker)+umcg_detach_peer();++if(!rcu_access_pointer(server_utd->peer)){+umcg_lock_pair(server,worker);+WARN_ON(worker_utd->peer);+rcu_assign_pointer(server_utd->peer,worker);+rcu_assign_pointer(worker_utd->peer,server);+umcg_unlock_pair(server,worker);+}++server_ut=server_utd->umcg_task;+worker_ut=server_utd->umcg_task;++rcu_read_unlock();++ret=do_context_switch(worker);+if(ret)+returnret;++rcu_read_lock();+worker=rcu_dereference(server_utd->peer);+if(worker){+worker_utd=rcu_dereference(worker->umcg_task_data);+if(worker_utd)+result=worker_utd->umcg_task;+}+rcu_read_unlock();++if(put_user(result,ut))+return-EFAULT;+return0;++out_rcu:+rcu_read_unlock();+returnret;+}++voidumcg_on_block(void)+{+structumcg_task_data*utd=rcu_access_pointer(current->umcg_task_data);+structumcg_task__user*ut;+structtask_struct*server;+u32state;++if(utd->task_type!=UMCG_TT_WORKER||utd->in_workqueue)+return;++ut=utd->umcg_task;++if(get_user(state,(u32__user*)ut)){+if(signal_pending(current))+return;+umcg_segv(0);+return;+}++if(state!=UMCG_TASK_RUNNING)+return;++state=UMCG_TASK_BLOCKED;+if(put_user(state,(u32__user*)ut)){+umcg_segv(0);+return;+}++rcu_read_lock();+server=rcu_dereference(utd->peer);+rcu_read_unlock();++if(server)+WARN_ON(!try_to_wake_up(server,TASK_NORMAL,WF_CURRENT_CPU));+}++/* Return true to return to the user, false to keep waiting. */+staticboolprocess_unblocked_worker(void)+{+structumcg_task_data*utd;+structumcg_group*group;++rcu_read_lock();++utd=rcu_dereference(current->umcg_task_data);+group=utd->group;++spin_lock(&group->lock);+if(!list_empty(&utd->list)){+/* This was a spurious wakeup or an interrupt, do nothing. */+spin_unlock(&group->lock);+rcu_read_unlock();+do_wait();+returnfalse;+}++if(group->nr_waiting_pollers>0){/* Wake a server. */+structtask_struct*server;+structumcg_task_data*server_utd=list_first_entry(+&group->waiters,structumcg_task_data,list);++list_del_init(&server_utd->list);+server=server_utd->self;+--group->nr_waiting_pollers;++umcg_lock_pair(server,current);+spin_unlock(&group->lock);++if(WARN_ON(server_utd->peer||utd->peer)){+umcg_segv(0);+returntrue;+}+rcu_assign_pointer(server_utd->peer,current);+rcu_assign_pointer(utd->peer,server);++umcg_unlock_pair(server,current);+rcu_read_unlock();++if(put_state(utd->umcg_task,UMCG_TASK_RUNNABLE)){+umcg_segv(0);+returntrue;+}++do_context_switch(server);+returnfalse;+}++/* Add to the queue. */+++group->nr_waiting_workers;+list_add_tail(&utd->list,&group->waiters);+spin_unlock(&group->lock);+rcu_read_unlock();++do_wait();++smp_rmb();+if(!list_empty(&utd->list)){+spin_lock(&group->lock);+list_del_init(&utd->list);+--group->nr_waiting_workers;+spin_unlock(&group->lock);+}++returnfalse;+}++voidumcg_on_wake(void)+{+structumcg_task_data*utd;+structumcg_task__user*ut;+boolshould_break=false;++/* current->umcg_task_data is modified only from current. */+utd=rcu_access_pointer(current->umcg_task_data);+if(utd->task_type!=UMCG_TT_WORKER||utd->in_workqueue)+return;++do{+u32state;++if(fatal_signal_pending(current))+return;++if(signal_pending(current))+return;++ut=utd->umcg_task;++if(get_state(ut,&state)){+if(signal_pending(current))+return;+gotosegv;+}++if(state==UMCG_TASK_RUNNING&&rcu_access_pointer(utd->peer))+return;++if(state==UMCG_TASK_BLOCKED||state==UMCG_TASK_RUNNING){+state=UMCG_TASK_UNBLOCKED;+if(put_state(ut,state))+gotosegv;+}elseif(state!=UMCG_TASK_UNBLOCKED){+gotosegv;+}++utd->in_workqueue=true;+should_break=process_unblocked_worker();+utd->in_workqueue=false;+if(should_break)+return;++}while(!should_break);++segv:+umcg_segv(0);+}
@@ -8,6 +8,34 @@#include<linux/sched.h>#include<linux/umcg.h>+structumcg_group{+structlist_headlist;+u32group_id;/* Never changes. */+u32api_version;/* Never changes. */+u64flags;/* Never changes. */++spinlock_tlock;++/*+*Oneofthecountersbelowisalwayszero.Thenon-zerocounter+*indicatesthenumberofelementsin@waitersbelow.+*/+intnr_waiting_workers;+intnr_waiting_pollers;++/*+*ThelistbeloweithercontainsUNBLOCKEDworkerswaiting+*fortheuserspacetopollorrunthemifnr_waiting_workers>0,+*orpollingserverswaitingforunblockedworkersif+*nr_waiting_pollers>0.+*/+structlist_headwaiters;++intnr_tasks;/* The total number of tasks registered. */++structrcu_headrcu;+};+enumumcg_task_type{UMCG_TT_CORE=1,UMCG_TT_SERVER=2,
@@ -32,11 +60,37 @@ struct umcg_task_data {*/u32api_version;+/* NULL for core API tasks. Never changes. */+structumcg_group*group;++/*+*Ifthisisaservertask,pointstoitsassignedworker,ifany;+*ifthisisaworkertask,pointstoitsassignedserver,ifany.+*+*Protectedbyalloc_lockofthetaskowningthisstruct.+*+*AlwayseitherNULL,ortheserverandtheworkerpointtoeachother.+*Lockingorder:firstlocktheserver,thentheworker.+*+*Eithertheworkerortheservershouldbethecurrenttaskwhen+*thisfieldischanged,withtheexceptionofsys_umcg_swap.+*/+structtask_struct__rcu*peer;++/* Used in umcg_group.waiters. */+structlist_headlist;++/* Used by curr in umcg_on_block/wake to prevent nesting/recursion. */+boolin_workqueue;+/**Usedbywait/wakeroutinestohandleraces.Writtenonlybycurrent.*/boolin_wait;};+voidumcg_on_block(void);+voidumcg_on_wake(void);+#endif /* CONFIG_UMCG */#endif /* _KERNEL_SCHED_UMCG_H */
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:36:55
Add userspace UMCG server/worker API.
This is an early RFC patch, with a lot of changes expected on the way.
Signed-off-by: Peter Oskolkov <redacted>
---
tools/lib/umcg/libumcg.c | 222 +++++++++++++++++++++++++++++++++++++++
tools/lib/umcg/libumcg.h | 108 +++++++++++++++++++
2 files changed, 330 insertions(+)
@@ -348,3 +428,145 @@ int umcg_swap(umcg_tid next, const struct timespec *timeout)return0;}++umcg_tumcg_create_group(uint32_tflags)+{+intres=sys_umcg_create_group(umcg_api_version,flags);+structumcg_group*group;++if(res<0){+errno=-res;+return-1;+}++group=malloc(sizeof(structumcg_group));+if(!group){+errno=ENOMEM;+returnUMCG_NONE;+}++group->group_id=res;+return(intptr_t)group;+}++intumcg_destroy_group(umcg_tumcg)+{+intres;+structumcg_group*group=(structumcg_group*)umcg;++res=sys_umcg_destroy_group(group->group_id);+if(res){+errno=-res;+return-1;+}++free(group);+return0;+}++umcg_tidumcg_poll_worker(void)+{+structumcg_task*server_ut=&umcg_task_tls->umcg_task;+structumcg_task*worker_ut;+uint32_texpected_state;+intret;++expected_state=UMCG_TASK_PROCESSING;+if(!atomic_compare_exchange_strong_explicit(&server_ut->state,+&expected_state,UMCG_TASK_POLLING,+memory_order_seq_cst,memory_order_seq_cst)){+fprintf(stderr,"umcg_poll_worker: wrong server state before: %u\n",+expected_state);+exit(1);+returnUMCG_NONE;+}+ret=sys_umcg_poll_worker(0,&worker_ut);++expected_state=UMCG_TASK_POLLING;+if(!atomic_compare_exchange_strong_explicit(&server_ut->state,+&expected_state,UMCG_TASK_PROCESSING,+memory_order_seq_cst,memory_order_seq_cst)){+fprintf(stderr,"umcg_poll_worker: wrong server state after: %u\n",+expected_state);+exit(1);+returnUMCG_NONE;+}++if(ret){+fprintf(stderr,"sys_umcg_poll_worker: unexpected result %d\n",+errno);+exit(1);+returnUMCG_NONE;+}++returnumcg_task_to_utid(worker_ut);+}++umcg_tidumcg_run_worker(umcg_tidworker)+{+structumcg_task_tls*worker_utls;+structumcg_task*server_ut=&umcg_task_tls->umcg_task;+structumcg_task*worker_ut;+uint32_texpected_state;+intret;++worker_utls=atomic_load_explicit((structumcg_task_tls**)worker,+memory_order_seq_cst);+if(!worker_utls)+returnUMCG_NONE;++worker_ut=&worker_utls->umcg_task;++expected_state=UMCG_TASK_RUNNABLE;+if(!atomic_compare_exchange_strong_explicit(&worker_ut->state,+&expected_state,UMCG_TASK_RUNNING,+memory_order_seq_cst,memory_order_seq_cst)){+fprintf(stderr,"umcg_run_worker: wrong worker state: %u\n",+expected_state);+exit(1);+returnUMCG_NONE;+}++expected_state=UMCG_TASK_PROCESSING;+if(!atomic_compare_exchange_strong_explicit(&server_ut->state,+&expected_state,UMCG_TASK_SERVING,+memory_order_seq_cst,memory_order_seq_cst)){+fprintf(stderr,"umcg_run_worker: wrong server state: %u\n",+expected_state);+exit(1);+returnUMCG_NONE;+}++again:+ret=sys_umcg_run_worker(0,worker_utls->tid,&worker_ut);+if(ret&&errno==EAGAIN)+gotoagain;++if(ret){+fprintf(stderr,"umcg_run_worker failed: %d %d\n",ret,errno);+returnUMCG_NONE;+}++expected_state=UMCG_TASK_SERVING;+if(!atomic_compare_exchange_strong_explicit(&server_ut->state,+&expected_state,UMCG_TASK_PROCESSING,+memory_order_seq_cst,memory_order_seq_cst)){+fprintf(stderr,"umcg_run_worker: wrong server state: %u\n",+expected_state);+exit(1);+returnUMCG_NONE;+}++returnumcg_task_to_utid(worker_ut);+}++uint32_tumcg_get_task_state(umcg_tidtask)+{+structumcg_task_tls*utls=atomic_load_explicit(+(structumcg_task_tls**)task,memory_order_seq_cst);++if(!utls)+returnUMCG_TASK_NONE;++returnatomic_load_explicit(&utls->umcg_task.state,memory_order_relaxed);+}
From: Peter Oskolkov <hidden> Date: 2021-05-20 18:37:00
Add UMCG server/worker API selftests. These are only basic
tests, they do not cover many important use cases/conditions.
More to come.
Signed-off-by: Peter Oskolkov <redacted>
---
tools/testing/selftests/umcg/.gitignore | 1 +
tools/testing/selftests/umcg/Makefile | 4 +-
tools/testing/selftests/umcg/umcg_test.c | 475 +++++++++++++++++++++++
3 files changed, 479 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/umcg/umcg_test.c
@@ -0,0 +1,475 @@+// SPDX-License-Identifier: GPL-2.0+#define _GNU_SOURCE+#include"libumcg.h"++#include<pthread.h>+#include<stdatomic.h>++#include"../kselftest_harness.h"++#define CHECK_CONFIG() \+{\+intret=sys_umcg_api_version(1,0);\+\+if(ret==-1&&errno==ENOSYS)\+SKIP(return,"CONFIG_UMCG not set");\+}++structworker_args{+umcg_tgroup;/* Which group the worker should join. */+umcg_tidutid;/* This worker's utid. */+void*(*thread_fn)(void*);/* Function to run. */+void*thread_arg;+intptr_ttag;+};++staticvoidvalidate_state(umcg_tidutid,u32expected,constchar*ctx)+{+u32state=umcg_get_task_state(utid);++if(state==expected)+return;++fprintf(stderr,"BAD state for %ld: expected: %u; got: %u; ctx :%s\n",+utid,expected,state,ctx);+exit(1);+}++staticvoid*worker_fn(void*arg)+{+void*result;+umcg_tidutid;+structworker_args*args=(structworker_args*)arg;++validate_state(umcg_get_utid(),UMCG_TASK_NONE,"worker_fn start");++atomic_thread_fence(memory_order_acquire);+atomic_store_explicit(&args->utid,umcg_get_utid(),+memory_order_seq_cst);++utid=umcg_register_worker(args->group,args->tag);+if(args->utid!=utid){+fprintf(stderr,"umcg_register_worker failed.\n");+exit(1);+}+validate_state(umcg_get_utid(),UMCG_TASK_RUNNING,"worker_fn in");++/* Fence args->thread_arg */+atomic_thread_fence(memory_order_acquire);++result=args->thread_fn(args->thread_arg);+validate_state(umcg_get_utid(),UMCG_TASK_RUNNING,"worker_fn out");++if(umcg_unregister_task()){+fprintf(stderr,"umcg_unregister_task failed.\n");+exit(1);+}+validate_state(umcg_get_utid(),UMCG_TASK_NONE,"worker_fn finish");++returnresult;+}++staticvoid*simple_running_worker(void*arg)+{+bool*checkpoint=(bool*)arg;++atomic_store_explicit(checkpoint,true,memory_order_relaxed);+returnNULL;+}++TEST(umcg_poll_run_test){+pthread_tworker;+boolcheckpoint=false;+structworker_argsworker_args;++CHECK_CONFIG();++worker_args.utid=UMCG_NONE;+worker_args.group=umcg_create_group(0);+ASSERT_NE(UMCG_NONE,worker_args.group);++worker_args.thread_fn=&simple_running_worker;+worker_args.thread_arg=&checkpoint;+worker_args.tag=0;++ASSERT_EQ(0,pthread_create(&worker,NULL,&worker_fn,&worker_args));++/* Wait for the worker to start. */+while(UMCG_NONE==atomic_load_explicit(&worker_args.utid,+memory_order_relaxed))+;++/*+*Makesurethattheworkerdoesnotcheckpointuntiltheserver+*runsit.+*/+usleep(1000);+ASSERT_FALSE(atomic_load_explicit(&checkpoint,memory_order_relaxed));++ASSERT_NE(0,umcg_register_server(worker_args.group,0));++/*+*Runtheworkeruntilitexits.Needtoloopbecausetheworker+*maypagefaultandwaketheserver.+*/+do{+u32state;++/* Poll the worker. */+ASSERT_EQ(worker_args.utid,umcg_poll_worker());+validate_state(worker_args.utid,UMCG_TASK_RUNNABLE,"wns poll");++umcg_tidutid=umcg_run_worker(worker_args.utid);+if(utid==UMCG_NONE){+ASSERT_EQ(0,errno);+break;+}++ASSERT_EQ(utid,worker_args.utid);++state=umcg_get_task_state(utid);+ASSERT_TRUE(state==UMCG_TASK_BLOCKED||UMCG_TASK_UNBLOCKED);+}while(true);++ASSERT_TRUE(atomic_load_explicit(&checkpoint,memory_order_relaxed));++/* Can't destroy group while this thread still belongs to it. */+ASSERT_NE(0,umcg_destroy_group(worker_args.group));+ASSERT_EQ(0,umcg_unregister_task());+ASSERT_EQ(0,umcg_destroy_group(worker_args.group));+ASSERT_EQ(0,pthread_join(worker,NULL));+}++staticvoid*sleeping_worker(void*arg)+{+int*checkpoint=(int*)arg;++atomic_store_explicit(checkpoint,1,memory_order_relaxed);+usleep(2000);+atomic_store_explicit(checkpoint,2,memory_order_relaxed);++returnNULL;+}++TEST(umcg_sleep_test){+pthread_tworker;+u32state;+intcheckpoint=0;+structworker_argsworker_args;++CHECK_CONFIG();++worker_args.utid=UMCG_NONE;+worker_args.group=umcg_create_group(0);+ASSERT_NE(UMCG_NONE,worker_args.group);++worker_args.thread_fn=&sleeping_worker;+worker_args.thread_arg=&checkpoint;+worker_args.tag=0;++ASSERT_EQ(0,pthread_create(&worker,NULL,&worker_fn,&worker_args));++/* Wait for the worker to start. */+while(UMCG_NONE==atomic_load_explicit(&worker_args.utid,+memory_order_relaxed))+;++/*+*Makesurethattheworkerdoesnotcheckpointuntiltheserver+*runsit.+*/+usleep(1000);+ASSERT_EQ(0,atomic_load_explicit(&checkpoint,memory_order_relaxed));++validate_state(umcg_get_utid(),UMCG_TASK_NONE,"sws prereg");++ASSERT_NE(0,umcg_register_server(worker_args.group,0));++validate_state(umcg_get_utid(),UMCG_TASK_PROCESSING,"sws postreg");++/*+*Runtheworkeruntilitcheckpoints1.Needtoloopbecause+*theworkermaypagefaultandwaketheserver.+*/+do{+ASSERT_EQ(worker_args.utid,umcg_poll_worker());+validate_state(worker_args.utid,UMCG_TASK_RUNNABLE,+"sws poll");++umcg_tidutid=umcg_run_worker(worker_args.utid);+ASSERT_EQ(utid,worker_args.utid);+}while(1!=atomic_load_explicit(&checkpoint,memory_order_relaxed));++state=umcg_get_task_state(worker_args.utid);+ASSERT_TRUE(state==UMCG_TASK_BLOCKED||UMCG_TASK_UNBLOCKED);+validate_state(umcg_get_utid(),UMCG_TASK_PROCESSING,"sws mid");++/* The worker cannot reach checkpoint 2 without the server running it. */+usleep(2000);+ASSERT_EQ(1,atomic_load_explicit(&checkpoint,memory_order_relaxed));++state=umcg_get_task_state(worker_args.utid);+ASSERT_TRUE(state==UMCG_TASK_BLOCKED||UMCG_TASK_UNBLOCKED);++/* Run the worker until it exits. */+do{+ASSERT_EQ(worker_args.utid,umcg_poll_worker());+umcg_tidutid=umcg_run_worker(worker_args.utid);+if(utid==UMCG_NONE){+ASSERT_EQ(0,errno);+break;+}++ASSERT_EQ(utid,worker_args.utid);+}while(true);++/* The final check and cleanup. */+ASSERT_EQ(2,atomic_load_explicit(&checkpoint,memory_order_relaxed));+validate_state(umcg_get_utid(),UMCG_TASK_PROCESSING,"sws preunreg");+ASSERT_EQ(0,pthread_join(worker,NULL));+ASSERT_EQ(0,umcg_unregister_task());+validate_state(umcg_get_utid(),UMCG_TASK_NONE,"sws postunreg");+ASSERT_EQ(0,umcg_destroy_group(worker_args.group));+}++staticvoid*waiting_worker(void*arg)+{+int*checkpoint=(int*)arg;++atomic_store_explicit(checkpoint,1,memory_order_relaxed);+if(umcg_wait(NULL)){+fprintf(stderr,"umcg_wait() failed.\n");+exit(1);+}+atomic_store_explicit(checkpoint,2,memory_order_relaxed);++returnNULL;+}++TEST(umcg_wait_wake_test){+pthread_tworker;+intcheckpoint=0;+structworker_argsworker_args;++CHECK_CONFIG();++worker_args.utid=UMCG_NONE;+worker_args.group=umcg_create_group(0);+ASSERT_NE(UMCG_NONE,worker_args.group);++worker_args.thread_fn=&waiting_worker;+worker_args.thread_arg=&checkpoint;+worker_args.tag=0;++ASSERT_EQ(0,pthread_create(&worker,NULL,&worker_fn,&worker_args));++/* Wait for the worker to start. */+while(UMCG_NONE==atomic_load_explicit(&worker_args.utid,+memory_order_relaxed))+;++/*+*Makesurethattheworkerdoesnotcheckpointuntiltheserver+*runsit.+*/+usleep(1000);+ASSERT_EQ(0,atomic_load_explicit(&checkpoint,memory_order_relaxed));++ASSERT_NE(0,umcg_register_server(worker_args.group,0));++/*+*Runtheworkeruntilitcheckpoints1.Needtoloopbecause+*theworkermaypagefaultandwaketheserver.+*/+do{+ASSERT_EQ(worker_args.utid,umcg_poll_worker());+ASSERT_EQ(worker_args.utid,umcg_run_worker(worker_args.utid));+}while(1!=atomic_load_explicit(&checkpoint,memory_order_relaxed));++validate_state(worker_args.utid,UMCG_TASK_RUNNABLE,"wait_wake wait");++/* The worker cannot reach checkpoint 2 without the server waking it. */+usleep(2000);+ASSERT_EQ(1,atomic_load_explicit(&checkpoint,memory_order_relaxed));+validate_state(worker_args.utid,UMCG_TASK_RUNNABLE,"wait_wake wait");+++ASSERT_EQ(0,umcg_wake(worker_args.utid));++/*+*umcg_wake()abovemarkstheworkerasRUNNING;itwillbecome+*UNBLOCKEDuponwakeupasitdoesnothaveaserver.Butthismay+*bedelayed.+*/+while(umcg_get_task_state(worker_args.utid)!=UMCG_TASK_UNBLOCKED)+;++/* The worker cannot reach checkpoint 2 without the server running it. */+usleep(2000);+ASSERT_EQ(1,atomic_load_explicit(&checkpoint,memory_order_relaxed));++/* Run the worker until it exits. */+do{+ASSERT_EQ(worker_args.utid,umcg_poll_worker());+umcg_tidutid=umcg_run_worker(worker_args.utid);+if(utid==UMCG_NONE){+ASSERT_EQ(0,errno);+break;+}++ASSERT_EQ(utid,worker_args.utid);+}while(true);++/* The final check and cleanup. */+ASSERT_EQ(2,atomic_load_explicit(&checkpoint,memory_order_relaxed));+ASSERT_EQ(0,pthread_join(worker,NULL));+ASSERT_EQ(0,umcg_unregister_task());+ASSERT_EQ(0,umcg_destroy_group(worker_args.group));+}++staticvoid*swapping_worker(void*arg)+{+umcg_tidnext;++atomic_thread_fence(memory_order_acquire);+next=(umcg_tid)arg;++if(next==UMCG_NONE){+if(0!=umcg_wait(NULL)){+fprintf(stderr,"swapping_worker: umcg_wait failed\n");+exit(1);+}+}else{+if(0!=umcg_swap(next,NULL)){+fprintf(stderr,"swapping_worker: umcg_swap failed\n");+exit(1);+}+}++returnNULL;+}++TEST(umcg_swap_test){+constintn_workers=10;+structworker_args*worker_args;+intswap_chain_wakeups=0;+umcg_tidutid=UMCG_NONE;+bool*workers_polled;+pthread_t*workers;+umcg_tgroup_id;+intidx;++CHECK_CONFIG();++group_id=umcg_create_group(0);+ASSERT_NE(UMCG_NONE,group_id);++workers=malloc(n_workers*sizeof(pthread_t));+worker_args=malloc(n_workers*sizeof(structworker_args));+workers_polled=malloc(n_workers*sizeof(bool));+if(!workers||!worker_args||!workers_polled){+fprintf(stderr,"malloc failed\n");+exit(1);+}++memset(worker_args,0,n_workers*sizeof(structworker_args));++/* Start workers. All will block in umcg_register_worker(). */+for(idx=0;idx<n_workers;++idx){+workers_polled[idx]=false;++worker_args[idx].group=group_id;+worker_args[idx].thread_fn=&swapping_worker;+worker_args[idx].tag=idx;+atomic_thread_fence(memory_order_release);++ASSERT_EQ(0,pthread_create(&workers[idx],NULL,&worker_fn,+&worker_args[idx]));+}++/* Wait for all workers to update their utids. */+for(idx=0;idx<n_workers;++idx){+uint64_tcounter=0;+while(UMCG_NONE==atomic_load_explicit(&worker_args[idx].utid,+memory_order_seq_cst)){+++counter;+if(!(counter%1000000))+fprintf(stderr,"looping for utid: %d %lu\n",+idx,counter);+}+}++/* Update worker args. */+for(idx=0;idx<(n_workers-1);++idx){+worker_args[idx].thread_arg=(void*)worker_args[idx+1].utid;+}+atomic_thread_fence(memory_order_release);++ASSERT_NE(0,umcg_register_server(group_id,0));++/* Poll workers. */+for(idx=0;idx<n_workers;++idx){+utid=umcg_poll_worker();++ASSERT_NE(UMCG_NONE,utid);+workers_polled[umcg_get_task_tag(utid)]=true;++validate_state(utid,UMCG_TASK_RUNNABLE,"swap poll");+}++/* Check that all workers have been polled. */+for(idx=0;idx<n_workers;++idx){+ASSERT_TRUE(workers_polled[idx]);+}++/* Run the first worker; the swap chain will lead to the last worker. */+utid=worker_args[0].utid;+idx=0;+do{+uint32_tstate;++utid=umcg_run_worker(utid);+if(utid==worker_args[n_workers-1].utid&&+umcg_get_task_state(utid)==UMCG_TASK_RUNNABLE)+break;++/* There can be an occasional mid-swap wakeup due to pagefault. */+++swap_chain_wakeups;++/* Validate progression. */+ASSERT_GE(umcg_get_task_tag(utid),idx);+idx=umcg_get_task_tag(utid);++/* Validate state. */+state=umcg_get_task_state(utid);+ASSERT_TRUE(state==UMCG_TASK_BLOCKED||+state==UMCG_TASK_UNBLOCKED);++ASSERT_EQ(utid,umcg_poll_worker());+}while(true);++ASSERT_LT(swap_chain_wakeups,4);+if(swap_chain_wakeups)+fprintf(stderr,"WARNING: %d swap chain wakeups\n",+swap_chain_wakeups);++/* Finally run/release all workers. */+for(idx=0;idx<n_workers;++idx){+utid=worker_args[idx].utid;+do{+utid=umcg_run_worker(utid);+if(utid){+ASSERT_EQ(utid,worker_args[idx].utid);+ASSERT_EQ(utid,umcg_poll_worker());+}+}while(utid!=UMCG_NONE);+}++/* Cleanup. */+for(idx=0;idx<n_workers;++idx)+ASSERT_EQ(0,pthread_join(workers[idx],NULL));+ASSERT_EQ(0,umcg_unregister_task());+ASSERT_EQ(0,umcg_destroy_group(group_id));+}++TEST_HARNESS_MAIN
From: Jonathan Corbet <corbet@lwn.net> Date: 2021-05-20 21:17:34
Peter Oskolkov [off-list ref] writes:
As indicated earlier in the FUTEX_SWAP patchset:
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/
"Google Fibers" is a userspace scheduling framework
used widely and successfully at Google to improve in-process workload
isolation and response latencies. We are working on open-sourcing
this framework, and UMCG (User-Managed Concurrency Groups) kernel
patches are intended as the foundation of this.
So I have to ask...is there *any* documentation out there on what this
is and how people are supposed to use it? Shockingly, typing "Google
fibers" into Google leads to a less than fully joyful outcome... This
won't be easy for anybody to review if they have to start by
reverse-engineering what it's supposed to do.
Thanks,
jon
From: Peter Oskolkov <hidden> Date: 2021-05-20 21:38:53
On Thu, May 20, 2021 at 2:17 PM Jonathan Corbet [off-list ref] wrote:
Peter Oskolkov [off-list ref] writes:
quoted
As indicated earlier in the FUTEX_SWAP patchset:
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/
"Google Fibers" is a userspace scheduling framework
used widely and successfully at Google to improve in-process workload
isolation and response latencies. We are working on open-sourcing
this framework, and UMCG (User-Managed Concurrency Groups) kernel
patches are intended as the foundation of this.
So I have to ask...is there *any* documentation out there on what this
is and how people are supposed to use it? Shockingly, typing "Google
fibers" into Google leads to a less than fully joyful outcome... This
won't be easy for anybody to review if they have to start by
reverse-engineering what it's supposed to do.
From: Randy Dunlap <hidden> Date: 2021-05-21 00:16:02
On 5/20/21 2:38 PM, Peter Oskolkov wrote:
On Thu, May 20, 2021 at 2:17 PM Jonathan Corbet [off-list ref] wrote:
quoted
Peter Oskolkov [off-list ref] writes:
quoted
As indicated earlier in the FUTEX_SWAP patchset:
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/
"Google Fibers" is a userspace scheduling framework
used widely and successfully at Google to improve in-process workload
isolation and response latencies. We are working on open-sourcing
this framework, and UMCG (User-Managed Concurrency Groups) kernel
patches are intended as the foundation of this.
So I have to ask...is there *any* documentation out there on what this
is and how people are supposed to use it? Shockingly, typing "Google
fibers" into Google leads to a less than fully joyful outcome... This
won't be easy for anybody to review if they have to start by
reverse-engineering what it's supposed to do.
Certainly for links to email, we prefer to use lore.kernel.org archives.
Are links to other sites discouraged? If so, that's news to me.
Feel free to reach out to me directly or through this LKML thread if
you have any questions.
Do you think a documentation patch would be useful at this point, as
opposed to a free-form email discussion?
Certainly for links to email, we prefer to use lore.kernel.org archives.
Are links to other sites discouraged? If so, that's news to me.
Discouraged in so far as that when an email solely references external
resources and doesn't bother to summarize or otherwise recap the
contents in the email proper; I'll ignore the whole thing.
Basically, if I have to click a link to figure out basic information of
a patch series, the whole thing is a fail and goes into the bit bucket.
That said; I have no objection against having links, as long as they're
not used to convey the primary information that _should_ be in the
cover letter and/or changelogs.
From: Jonathan Corbet <corbet@lwn.net> Date: 2021-05-21 15:08:24
Peter Oskolkov [off-list ref] writes:
On Thu, May 20, 2021 at 2:17 PM Jonathan Corbet [off-list ref] wrote:
quoted
Peter Oskolkov [off-list ref] writes:
quoted
As indicated earlier in the FUTEX_SWAP patchset:
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/
"Google Fibers" is a userspace scheduling framework
used widely and successfully at Google to improve in-process workload
isolation and response latencies. We are working on open-sourcing
this framework, and UMCG (User-Managed Concurrency Groups) kernel
patches are intended as the foundation of this.
So I have to ask...is there *any* documentation out there on what this
is and how people are supposed to use it? Shockingly, typing "Google
fibers" into Google leads to a less than fully joyful outcome... This
won't be easy for anybody to review if they have to start by
reverse-engineering what it's supposed to do.
I did look at those - but a presentation from 2013 is going to be of
limited relevance for a 2021 patch set. In particular, the syscall API
appears to have evolved considerably since then.
Feel free to reach out to me directly or through this LKML thread if
you have any questions.
Do you think a documentation patch would be useful at this point, as
opposed to a free-form email discussion?
Documentation patches can help to guide that discussion; they also need
to be reviewed as well. So yes, I think they should be present from the
beginning. But then, that's the position I'm supposed to take :) This
is a big change to the kernel's system-call API, I don't think that
there can be a proper discussion of that without a description of what
you're trying to do.
Thanks,
jon
From: Peter Oskolkov <hidden> Date: 2021-05-21 16:03:28
On Fri, May 21, 2021 at 8:08 AM Jonathan Corbet [off-list ref] wrote:
[...]
Documentation patches can help to guide that discussion; they also need
to be reviewed as well. So yes, I think they should be present from the
beginning. But then, that's the position I'm supposed to take :) This
is a big change to the kernel's system-call API, I don't think that
there can be a proper discussion of that without a description of what
you're trying to do.
Hi Jon,
There are doc comments in patches 2 and 7 in umcg.c documenting the
new syscalls. That said, I'll prepare a separate doc patch - I guess
I'll add Documentation/scheduler/umcg.rst, unless you tell me there is
a better place to do that. ETA mid-to-late next week.
Thanks,
Peter
From: Jonathan Corbet <corbet@lwn.net> Date: 2021-05-21 19:17:58
Peter Oskolkov [off-list ref] writes:
On Fri, May 21, 2021 at 8:08 AM Jonathan Corbet [off-list ref] wrote:
[...]
quoted
Documentation patches can help to guide that discussion; they also need
to be reviewed as well. So yes, I think they should be present from the
beginning. But then, that's the position I'm supposed to take :) This
is a big change to the kernel's system-call API, I don't think that
there can be a proper discussion of that without a description of what
you're trying to do.
Hi Jon,
There are doc comments in patches 2 and 7 in umcg.c documenting the
new syscalls. That said, I'll prepare a separate doc patch - I guess
I'll add Documentation/scheduler/umcg.rst, unless you tell me there is
a better place to do that. ETA mid-to-late next week.
Yes, I saw those; they are a bit terse at best. What are the "worker
states"? What's a "UMCG group"? Yes, all this can be worked out by
pounding one's head against the code for long enough, but you're asking
a fair amount of your reviewers.
A good overall description would be nice, perhaps for the userspace-api
book. But *somebody* is also going to have to write real man pages for
all these system calls; if you provided those, the result should be a
good description of how you expect this subsystem to work.
Thanks,
jon
From: Andy Lutomirski <luto@kernel.org> Date: 2021-05-21 19:32:22
On Thu, May 20, 2021 at 11:36 AM Peter Oskolkov [off-list ref] wrote:
Implement version 1 of core UMCG API (wait/wake/swap).
As has been outlined in
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/,
efficient and synchronous on-CPU context switching is key
to enabling two broad use cases: in-process M:N userspace scheduling
and fast X-process RPCs for security wrappers.
High-level design considerations/approaches used:
- wait & wake can race with each other;
- offload as much work as possible to libumcg in tools/lib/umcg,
specifically:
- most state changes, e.g. RUNNABLE <=> RUNNING, are done in
the userspace (libumcg);
- retries are offloaded to the userspace.
Do you have some perf numbers as to how long a UMCG context switch
takes compared to a normal one?
--Andy
From: Andrei Vagin <hidden> Date: 2021-05-21 20:20:54
On Thu, May 20, 2021 at 11:36:12AM -0700, Peter Oskolkov wrote:
quoted hunk
Implement UMCG server/worker API.
This is an early RFC patch - the code seems working, but
more testing is needed. Gaps I plan to address before this
is ready for a detailed review:
- preemption/interrupt handling;
- better documentation/comments;
- tracing;
- additional testing;
- corner cases like abnormal process/task termination;
- in some cases where I kill the task (umcg_segv), returning
an error may be more appropriate.
All in all, please focus more on the high-level approach
and less on things like variable names, (doc) comments, or indentation.
Signed-off-by: Peter Oskolkov <redacted>
---
include/linux/mm_types.h | 5 +
include/linux/syscalls.h | 5 +
kernel/fork.c | 11 +
kernel/sched/core.c | 11 +
kernel/sched/umcg.c | 764 ++++++++++++++++++++++++++++++++++++++-
kernel/sched/umcg.h | 54 +++
mm/init-mm.c | 4 +
7 files changed, 845 insertions(+), 9 deletions(-)
I am not sure that I understand what is going on here. umsg_groups is
the head of a group list. list_del is usually called on list entries.
Should we enumirate all groups here and destroy them?
+ spin_unlock(&mm->umcg_lock);
+ }
+#endif
if (mm->binfmt)
module_put(mm->binfmt->module);
mmdrop(mm);
...
+/**
+ * sys_umcg_create_group - create a UMCG group
+ * @api_version: Requested API version.
+ * @flags: Reserved.
+ *
+ * Return:
+ * >= 0 - the group ID
+ * -EOPNOTSUPP - @api_version is not supported
+ * -EINVAL - @flags is not valid
+ * -ENOMEM - not enough memory
+ */
+SYSCALL_DEFINE2(umcg_create_group, u32, api_version, u64, flags)
+{
+ int ret;
+ struct umcg_group *group;
+ struct umcg_group *list_entry;
+ struct mm_struct *mm = current->mm;
+
+ if (flags)
+ return -EINVAL;
+
+ if (__api_version(api_version))
+ return -EOPNOTSUPP;
+
+ group = kzalloc(sizeof(struct umcg_group), GFP_KERNEL);
+ if (!group)
+ return -ENOMEM;
+
+ spin_lock_init(&group->lock);
+ INIT_LIST_HEAD(&group->list);
+ INIT_LIST_HEAD(&group->waiters);
+ group->flags = flags;
+ group->api_version = api_version;
+
+ spin_lock(&mm->umcg_lock);
+
+ list_for_each_entry_rcu(list_entry, &mm->umcg_groups, list) {
+ if (list_entry->group_id >= group->group_id)
+ group->group_id = list_entry->group_id + 1;
+ }
pls take into account that we need to be able to save and restore umcg
groups from user-space. There is the CRIU project that allows to
checkpoint/restore processes.
+
+ list_add_rcu(&mm->umcg_groups, &group->list);
I think it should be:
list_add_rcu(&group->list, &mm->umcg_groups);
On Thu, May 20, 2021 at 8:36 PM Peter Oskolkov [off-list ref] wrote:
Implement version 1 of core UMCG API (wait/wake/swap).
As has been outlined in
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/,
efficient and synchronous on-CPU context switching is key
to enabling two broad use cases: in-process M:N userspace scheduling
and fast X-process RPCs for security wrappers.
High-level design considerations/approaches used:
- wait & wake can race with each other;
- offload as much work as possible to libumcg in tools/lib/umcg,
specifically:
- most state changes, e.g. RUNNABLE <=> RUNNING, are done in
the userspace (libumcg);
- retries are offloaded to the userspace.
+static int do_context_switch(struct task_struct *next)
+{
+ struct umcg_task_data *utd = rcu_access_pointer(current->umcg_task_data);
+
+ /*
+ * It is important to set_current_state(TASK_INTERRUPTIBLE) before
+ * waking @next, as @next may immediately try to wake current back
+ * (e.g. current is a server, @next is a worker that immediately
+ * blocks or waits), and this next wakeup must not be lost.
+ */
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ WRITE_ONCE(utd->in_wait, true);
+
+ if (!try_to_wake_up(next, TASK_NORMAL, WF_CURRENT_CPU))
+ return -EAGAIN;
+
+ freezable_schedule();
+
+ WRITE_ONCE(utd->in_wait, false);
+
+ if (signal_pending(current))
+ return -EINTR;
What is this -EINTR supposed to tell userspace? We can't tell whether
we were woken up by a signal or by do_context_switch() or the
umcg_wake syscall, right? If we're woken by another thread calling
do_context_switch() and then get a signal immediately afterwards,
can't that lead to a lost wakeup?
I don't know whether trying to track the origin of the wakeup is a
workable approach here; you might have to instead do cmpxchg() on the
->in_wait field and give it three states (default, waiting-for-wake
and successfully-woken)?
Or you give up on trying to figure out who woke you, just always
return zero, and let userspace deal with figuring out whether the
wakeup was real or not. I don't know whether that'd be acceptable.
rcu_access_pointer() instead of the locking and unlocking?
quoted hunk
+ return do_wait();
}
/**
@@ -110,7 +264,39 @@ SYSCALL_DEFINE2(umcg_wait, u32, flags, */ SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid) {- return -ENOSYS;+ struct umcg_task_data *next_utd;+ struct task_struct *next;+ int ret = -EINVAL;++ if (!next_tid)+ return -EINVAL;+ if (flags)+ return -EINVAL;++ next = find_get_task_by_vpid(next_tid);+ if (!next)+ return -ESRCH;+ rcu_read_lock();
Wouldn't it be more efficient to replace the last 4 lines with the following?
rcu_read_lock();
next = find_task_by_vpid(next_tid);
if (!next) {
err = -ESRCH;
goto out;
}
Then you don't need to use refcounting here...
+ next_utd = rcu_dereference(next->umcg_task_data);
+ if (!next_utd)
+ goto out;
+
+ if (!READ_ONCE(next_utd->in_wait)) {
+ ret = -EAGAIN;
+ goto out;
+ }
+
+ ret = wake_up_process(next);
+ put_task_struct(next);
... and you'd be able to drop this put_task_struct(), too.
quoted hunk
+ if (ret)
+ ret = 0;
+ else
+ ret = -EAGAIN;
+
+out:
+ rcu_read_unlock();
+ return ret;
}
/**
@@ -139,5 +325,44 @@ SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid) SYSCALL_DEFINE4(umcg_swap, u32, wake_flags, u32, next_tid, u32, wait_flags, const struct __kernel_timespec __user *, timeout) {- return -ENOSYS;+ struct umcg_task_data *curr_utd;+ struct umcg_task_data *next_utd;+ struct task_struct *next;+ int ret = -EINVAL;++ rcu_read_lock();+ curr_utd = rcu_dereference(current->umcg_task_data);++ if (!next_tid || wake_flags || wait_flags || !curr_utd)+ goto out;++ if (timeout) {+ ret = -EOPNOTSUPP;+ goto out;+ }++ next = find_get_task_by_vpid(next_tid);+ if (!next) {+ ret = -ESRCH;+ goto out;+ }
There isn't any type of access check here, right? Any task can wake up
any other task? That feels a bit weird to me - and if you want to keep
it as-is, it should probably at least be documented that any task on
the system can send you spurious wakeups if you opt in to umcg.
In contrast, shared futexes can avoid this because they get their
access control implicitly from the VMA.
+ next_utd = rcu_dereference(next->umcg_task_data);
+ if (!next_utd) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (!READ_ONCE(next_utd->in_wait)) {
+ ret = -EAGAIN;
+ goto out;
+ }
+
+ rcu_read_unlock();
+
+ return do_context_switch(next);
It looks like the refcount of the target task is incremented but never
decremented, so this probably currently leaks references?
I'd maybe try to split do_context_switch() into two parts, one that
does the non-blocking waking of another task and one that does the
sleeping. Then you can avoid taking a reference on the task as above -
this is supposed to be a really hot fastpath, so it's a good idea to
avoid atomic instructions if possible, right?
From: Peter Oskolkov <hidden> Date: 2021-05-21 22:01:41
On Fri, May 21, 2021 at 12:32 PM Andy Lutomirski [off-list ref] wrote:
On Thu, May 20, 2021 at 11:36 AM Peter Oskolkov [off-list ref] wrote:
quoted
Implement version 1 of core UMCG API (wait/wake/swap).
As has been outlined in
https://lore.kernel.org/lkml/20200722234538.166697-1-posk@posk.io/,
efficient and synchronous on-CPU context switching is key
to enabling two broad use cases: in-process M:N userspace scheduling
and fast X-process RPCs for security wrappers.
High-level design considerations/approaches used:
- wait & wake can race with each other;
- offload as much work as possible to libumcg in tools/lib/umcg,
specifically:
- most state changes, e.g. RUNNABLE <=> RUNNING, are done in
the userspace (libumcg);
- retries are offloaded to the userspace.
Do you have some perf numbers as to how long a UMCG context switch
takes compared to a normal one?
I'm not sure what is a "normal context switch" in this context. Futex
wakeup on a remote idle CPU takes 5-10usec; an on-CPU UMCG context
switch takes less than 1usec; futex wake + futex wait on the same CPU
(taskset ***) takes about 1-1.5usec in my benchmarks.
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-06-09 12:55:38
Quoting random parts of the first few patches folded.
You present an API without explaining, *at*all*, how it's supposed to be
used and I can't seem to figure it out from the implementation either :/
@@ -368,6 +368,17 @@ 444 common landlock_create_ruleset sys_landlock_create_ruleset 445 common landlock_add_rule sys_landlock_add_rule 446 common landlock_restrict_self sys_landlock_restrict_self
+447 common umcg_api_version sys_umcg_api_version
+448 common umcg_register_task sys_umcg_register_task
+449 common umcg_unregister_task sys_umcg_unregister_task
I think we can do away with the api_version thing and frob that in
register. Also, do we really need unregister over just letting a task
exit? Is there a sane use-case where task goes in and out of service?
+450 common umcg_wait sys_umcg_wait
+451 common umcg_wake sys_umcg_wake
Right, except I'm confused by the proposed implementation. I thought the
whole point was to let UMCG tasks block in kernel, at which point we'd
change their state to BLOCKED and have userspace select another task to
run. Such BLOCKED tasks would then also be captured before they return
to userspace, i.e. the whole admission scheduler thing.
I don't see any of that in these patches. So what are they actually
implementing? I can't find enough clues to tell :-(
+452 common umcg_swap sys_umcg_swap
You're presenting it like a pure optimization, but IIRC this is what
enables us to frob the scheduler state to ensure the whole thing is seen
(to the rest of the system) as the M server tasks, instead of the
constellation of N+M worker and server tasks.
Also, you're not doing any of the frobbing required.
+453 common umcg_create_group sys_umcg_create_group
+454 common umcg_destroy_group sys_umcg_destroy_group
This is basically needed for cross-server things, right? What we in the
kernel would call SMP. Some thoughts on that below.
+455 common umcg_poll_worker sys_umcg_poll_worker
Shouldn't this be called idle or something, instead of poll, the whole
point of having this syscall is to that you can indeed go idle.
Userspace can implement polling just fine without help:
for (;;) {
struct umcg_task *runnable = xchg(me->umcg_runnable_ptr, NULL);
if (runnable) {
// put them on a list and run one
}
cpu_relax();
}
comes to mind (see below).
+456 common umcg_run_worker sys_umcg_run_worker
This I'm confused about again.. there is no fundamental difference
between a worker or server, they're all the same.
+457 common umcg_preempt_worker sys_umcg_preempt_worker
All that needs a state transition diagram included
+ */
+#define UMCG_TASK_NONE 0
+/* UMCG server states. */
+#define UMCG_TASK_POLLING 1
+#define UMCG_TASK_SERVING 2
+#define UMCG_TASK_PROCESSING 3
I get POLLING, although per the above, this probably wants to be IDLE.
What are the other two again? That is, along with the diagram, each
state wants a description.
Weird order, also I can't remember why we need the UNBLOCKED, isn't that
the same as the RUNNABLE, or did we want to distinguish the state were
we're no longer BLOCKED but the user scheduler hasn't yet put us on it's
ready queue (IOW, we're on the runnable_ptr list, see below).
+
+/* UMCG task state flags, bits 8-15 */
+#define UMCG_TF_WAKEUP_QUEUED (1 << 8)
+
+/*
+ * Unused at the moment flags reserved for features to be introduced
+ * in the near future.
+ */
+#define UMCG_TF_PREEMPT_DISABLED (1 << 9)
+#define UMCG_TF_PREEMPTED (1 << 10)
+
+#define UMCG_NOID UINT_MAX
+
+/**
+ * struct umcg_task - controls the state of UMCG-enabled tasks.
+ *
+ * While at the moment only one field is present (@state), in future
+ * versions additional fields will be added, e.g. for the userspace to
+ * provide performance-improving hints and for the kernel to export sched
+ * stats.
+ *
+ * The struct is aligned at 32 bytes to ensure that even with future additions
+ * it fits into a single cache line.
+ */
+struct umcg_task {
+ /**
+ * @state: the current state of the UMCG task described by this struct.
+ *
+ * UMCG task state:
+ * bits 0 - 7: task state;
+ * bits 8 - 15: state flags;
+ * bits 16 - 23: reserved; must be zeroes;
+ * bits 24 - 31: for userspace use.
+ */
+ uint32_t state;
+} __attribute((packed, aligned(4 * sizeof(uint64_t))));
So last time I really looked at this it looked something like this:
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_server_tid; /* r */
u32 umcg_next_tid; /* r */
u32 __hole__;
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
(where r/w is from the kernel's pov)
(also see uapi/linux/rseq.h's ptr magic)
So a PF_UMCG_WORKER would be added to sched_submit_work()'s PF_*_WORKER
path to capture these tasks blocking. The umcg_sleeping() hook added
there would:
put_user(BLOCKED, umcg_task->umcg_status);
tid = get_user(umcg_task->next_tid);
if (!tid)
tid = get_user(umcg_task->umcg_server_tid);
umcg_server = find_task(tid);
/* append to blocked list */
umcg_task->umcg_blocked_ptr = umcg_server->umcg_blocked_ptr;
umcg_server->umcg_blocked_ptr = umcg_task;
// with some user_cmpxchg() sprinkled on to make it an atomic single
// linked list, we can borrow from futex_atomic_cmpxchg_inatomic().
/* capture return to user */
add_task_work(current, ¤t->umcg->task_work, TWA_RESUME);
umcg_server->state = RUNNING;
wake_up_process(umcg_server);
That task_work would, as the comment says, capture the return to user,
and do something like:
put_user(RUNNABLE, umcg_task->umcg_status);
tid = get_user(umcg_task->umcg_server_tid);
umcg_server = find_task(tid);
/* append to runable list */
umcg_task->umcg_runnable_ptr = umcg_server->umcg_runnable_ptr;
umcg_server->umcg_runnable_ptr = umcg_task;
// same as above, this wants some user cmpxchg
umcg_wait();
And for that we had something like:
void umcg_wait(void)
{
u32 state;
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
if (get_user(state, current->umcg->state))
break;
if (state == UMCG_RUNNING)
break;
if (signal_pending(current))
break;
schedule();
}
__set_current_state(TASK_RUNNING);
}
Which would wait until the userspace admission logic lets us rip by
setting state to RUNNING and prodding us with a sharp stick.
This all ensures that when a UMCG task goes to sleep, we mark ourselves
BLOCKED, we add ourselves to a user visible blocked list and wake the
owner of that blocked list.
We can either pre-select some task to run after us (next_tid) or it'll
pick the dedicated server task we're assigned to (server_tid).
Any time a task wakes up, it needs to check the blocked list and update
userspace ready queues and the sort, after which it can either run
things if it's a worker or pick another task to run if that's its work
(a server isn't special in this regard).
This was the absolute bare minimum, and I'm not seeing any of that here.
Nor an explanation of what there actually is :/
On top of this there's 'fun' questions about signals, ptrace and
umcg_preemption to be answered.
I think we want to allow signals to happen to UMCG RUNNABLE tasks, but
have them resume umcg_wait() on sigreturn.
I've not re-read the discussion with tglx on ptrace, he had some cute
corner cases IIRC.
The whole preemption thing should be doable with a task_work. Basically
check if the victim is RUNNING, send it TWA_SIGNAL to handle the task
work, the task_work would attempt a RUNNING->RUNNABLE (cmpxchg)
transition, success thereof needs to be propagated back to the syscall
and returned.
Adding preemption also means you have to deal with appending to
runnable_ptr list when the server isn't reaily available (most times).
Now on to those group things; they would basically replace the above
server_tid with a group/list of related server tasks, right? So why not
do so, litearlly:
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_next_tid; /* r */
u64 umcg_server_ptr; /* r */
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
Then have the kernel iterate the umcg_server_ptr list, looking for an
available (RUNNING or IDLE) server, also see the preemption point above.
This does, however, require a umcg_task to pid translation, which we've
so far avoided :/ OTOH it makes that grouping crud a user problem and we
can make the syscalls go away (and I that CRUI would like this better
too).
+static int do_context_switch(struct task_struct *next)
+{
+ struct umcg_task_data *utd = rcu_access_pointer(current->umcg_task_data);
+
+ /*
+ * It is important to set_current_state(TASK_INTERRUPTIBLE) before
+ * waking @next, as @next may immediately try to wake current back
+ * (e.g. current is a server, @next is a worker that immediately
+ * blocks or waits), and this next wakeup must not be lost.
+ */
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ WRITE_ONCE(utd->in_wait, true);
+
+ if (!try_to_wake_up(next, TASK_NORMAL, WF_CURRENT_CPU))
+ return -EAGAIN;
+
+ freezable_schedule();
+
+ WRITE_ONCE(utd->in_wait, false);
+
+ if (signal_pending(current))
+ return -EINTR;
+
+ return 0;
+}
+
+static int do_wait(void)
+{
+ struct umcg_task_data *utd = rcu_access_pointer(current->umcg_task_data);
+
+ if (!utd)
+ return -EINVAL;
+
+ WRITE_ONCE(utd->in_wait, true);
+
+ set_current_state(TASK_INTERRUPTIBLE);
+ freezable_schedule();
+
+ WRITE_ONCE(utd->in_wait, false);
+
+ if (signal_pending(current))
+ return -EINTR;
+
+ return 0;
+}
Both these are fundamentally buggered for not having a loop.
+/**
+ * sys_umcg_wait - block the current task (if all condtions are met).
+ * @flags: Reserved.
+ * @timeout: The absolute timeout of the wait. Not supported yet.
+ * Must be NULL.
+ *
+ * Sleep until woken, interrupted, or @timeout expires.
+ *
+ * Return:
+ * 0 - Ok;
+ * -EFAULT - failed to read struct umcg_task assigned to this task
+ * via sys_umcg_register();
+ * -EAGAIN - try again;
+ * -EINTR - signal pending;
+ * -EOPNOTSUPP - @timeout != NULL (not supported yet).
+ * -EINVAL - a parameter or a member of struct umcg_task has a wrong value.
+ */
+SYSCALL_DEFINE2(umcg_wait, u32, flags,
+ const struct __kernel_timespec __user *, timeout)
+
+ return do_wait();
+}
+
+/**
+ * sys_umcg_wake - wake @next_tid task blocked in sys_umcg_wait.
+ * @flags: Reserved.
+ * @next_tid: The ID of the task to wake.
+ *
+ * Wake @next identified by @next_tid. @next must be either a UMCG core
+ * task or a UMCG worker task.
+ *
+ * Return:
+ * 0 - Ok;
+ * -EFAULT - failed to read struct umcg_task assigned to next;
+ * -ESRCH - @next_tid did not identify a task;
+ * -EAGAIN - try again;
+ * -EINVAL - a parameter or a member of next->umcg_task has a wrong value.
+ */
+SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid)
+{
+ struct umcg_task_data *next_utd;
+ struct task_struct *next;
+ int ret = -EINVAL;
+
+ if (!next_tid)
+ return -EINVAL;
+ if (flags)
+ return -EINVAL;
+
+ next = find_get_task_by_vpid(next_tid);
+ if (!next)
+ return -ESRCH;
+
+ rcu_read_lock();
+ next_utd = rcu_dereference(next->umcg_task_data);
+ if (!next_utd)
+ goto out;
+
+ if (!READ_ONCE(next_utd->in_wait)) {
+ ret = -EAGAIN;
+ goto out;
+ }
I'm thining this might want to be a user cmpxchg from RUNNABLE->RUNNING.
You need to deal with concurrent wakeups.
+
+ ret = wake_up_process(next);
+ put_task_struct(next);
+ if (ret)
+ ret = 0;
+ else
+ ret = -EAGAIN;
+
+out:
+ rcu_read_unlock();
+ return ret;
+}
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-06-09 13:02:34
On Fri, May 21, 2021 at 11:33:14PM +0200, Jann Horn wrote:
quoted
SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid)
{
- return -ENOSYS;
+ struct umcg_task_data *next_utd;
+ struct task_struct *next;
+ int ret = -EINVAL;
+
+ if (!next_tid)
+ return -EINVAL;
+ if (flags)
+ return -EINVAL;
+
+ next = find_get_task_by_vpid(next_tid);
+ if (!next)
+ return -ESRCH;
+ rcu_read_lock();
Wouldn't it be more efficient to replace the last 4 lines with the following?
rcu_read_lock();
next = find_task_by_vpid(next_tid);
if (!next) {
err = -ESRCH;
goto out;
}
This wakeup crud needs to modify the umcg->state, which is a user
variable. That can't be done under RCU. Weirdly the proposed code
doesn't actually do any of that for undocumented raisins :/
Then you don't need to use refcounting here...
quoted
+ next_utd = rcu_dereference(next->umcg_task_data);
+ if (!next_utd)
+ goto out;
+
+ if (!READ_ONCE(next_utd->in_wait)) {
+ ret = -EAGAIN;
+ goto out;
+ }
+
+ ret = wake_up_process(next);
+ put_task_struct(next);
... and you'd be able to drop this put_task_struct(), too.
quoted
+ if (ret)
+ ret = 0;
+ else
+ ret = -EAGAIN;
+
+out:
+ rcu_read_unlock();
+ return ret;
}
/**
@@ -139,5 +325,44 @@ SYSCALL_DEFINE2(umcg_wake, u32, flags, u32, next_tid) SYSCALL_DEFINE4(umcg_swap, u32, wake_flags, u32, next_tid, u32, wait_flags, const struct __kernel_timespec __user *, timeout) {- return -ENOSYS;+ struct umcg_task_data *curr_utd;+ struct umcg_task_data *next_utd;+ struct task_struct *next;+ int ret = -EINVAL;++ rcu_read_lock();+ curr_utd = rcu_dereference(current->umcg_task_data);++ if (!next_tid || wake_flags || wait_flags || !curr_utd)+ goto out;++ if (timeout) {+ ret = -EOPNOTSUPP;+ goto out;+ }++ next = find_get_task_by_vpid(next_tid);+ if (!next) {+ ret = -ESRCH;+ goto out;+ }
There isn't any type of access check here, right? Any task can wake up
any other task? That feels a bit weird to me - and if you want to keep
it as-is, it should probably at least be documented that any task on
the system can send you spurious wakeups if you opt in to umcg.
You can only send wakeups to other UMCG thingies, per the
next->umcg_task_data check below. That said..
In contrast, shared futexes can avoid this because they get their
access control implicitly from the VMA.
Every task must expect spurious wakups at all times, always (for
TASK_NORMAL wakeups that is). There's plenty ways to generate them.
quoted
+ next_utd = rcu_dereference(next->umcg_task_data);
+ if (!next_utd) {
+ ret = -EINVAL;
+ goto out;
+ }
From: Peter Oskolkov <hidden> Date: 2021-06-09 20:20:28
On Wed, Jun 9, 2021 at 5:55 AM Peter Zijlstra [off-list ref] wrote:
Finally, a high-level review - thanks a lot, Peter! My comments below,
and two high-level "important questions" at the end of my reply (with
some less important questions here and there).
[...]
You present an API without explaining, *at*all*, how it's supposed to be
used and I can't seem to figure it out from the implementation either :/
I tried to explain it in the doc patch that I followed up with:
https://lore.kernel.org/patchwork/cover/1433967/#1632328
Or do you mean it more narrowly, i.e. I do not explain syscalls in
detail? This assessment I agree with - my approach was/is to finalize
the userpace API (libumcg) first, and make the userspace vs kernel
decisions later.
For example, you wonder why there is no looping in umcg_wait
(do_wait). This is because the looping happens in the userspace in
libumcg. My overall approach was to make the syscalls as simple as
possible and push extra logic to the userspace.
It seems that this approach is not resonating with kernel
developers/maintainers - you are the third person asking why there is
no looping in sys_umcg_wait, despite the fact that I explicitly
mentioned pushing it out to the userspace.
Let me try to make my case once more here.
umcg_wait/umcg_wake: the RUNNABLE/RUNNING state changes, checks, and
looping happen in the userspace (libumcg - see umcg_wait/umcg_wake in
patch 5 here: https://lore.kernel.org/patchwork/patch/1433971/), while
the syscalls simply sleep/wake. I find doing it in the userspace is
much simpler and easier than in the kernel, as state reads and writes
are just atomic memory accesses; in the kernel it becomes much more
difficult - rcu locked sections, tasks locked, etc.
On the other hand I agree that having syscalls more logically
complete, in the sense that they do not require much hand-holding and
retries from the userspace, is probably better from the API design
perspective. My worry here is that state validation and retries in the
userspace are unavoidable, and so going the usual way we will end up
with retry loops both in the kernel and in the userspace.
So I pose this IMPORTANT QUESTION #1 to you that I hope to get a clear
answer to: it is strongly preferable to have syscalls be "logically
complete" in the sense that they retry things internally, and in
generally try to cover all possible corner cases; or, alternatively,
is it OK to make syscalls lightweight but "logically incomplete", and
have the accompanied userspace wrappers do all of the heavy lifting
re: state changes/validation, retries, etc.?
I see two additional benefits of thin/lightweight syscalls:
- reading userspace state is needed much less often (e.g. my umcg_wait
and umcg_wake syscalls do not access userspace data at all - also see
my "second important question" below)
- looping in the kernel, combined with reading/writing to userspace
memory, can easily lead to spinning in the kernel (e.g. trying to
atomically change a variable and looping until succeeding)
A clear answer one way or the other will help a lot!
[...]
quoted
+448 common umcg_register_task sys_umcg_register_task
+449 common umcg_unregister_task sys_umcg_unregister_task
I think we can do away with the api_version thing and frob that in
register.
Ok, will do.
Also, do we really need unregister over just letting a task
exit? Is there a sane use-case where task goes in and out of service?
I do not know of a specific use case here. On the other hand, I do not
know of a specific use case to unregister RSEQ, but the capability is
there. Maybe the assumption is that the userspace memory passed to the
kernel in register() may be freed before the task exits, and so there
should be a way to tell the kernel to no longer use it?
quoted
+450 common umcg_wait sys_umcg_wait
+451 common umcg_wake sys_umcg_wake
Right, except I'm confused by the proposed implementation. I thought the
whole point was to let UMCG tasks block in kernel, at which point we'd
change their state to BLOCKED and have userspace select another task to
run. Such BLOCKED tasks would then also be captured before they return
to userspace, i.e. the whole admission scheduler thing.
I don't see any of that in these patches. So what are they actually
implementing? I can't find enough clues to tell :-(
You're presenting it like a pure optimization, but IIRC this is what
enables us to frob the scheduler state to ensure the whole thing is seen
(to the rest of the system) as the M server tasks, instead of the
constellation of N+M worker and server tasks.
Yes, you recall it correctly.
Also, you're not doing any of the frobbing required.
This is because I consider the frobbing a (very) nice to have rather
than a required feature, and so I am hoping to argue about how to
properly do it in later patchsets. This whole thing (UMCG) will be
extremely useful even without runtime accounting hacking and whatnot,
and so I hope to have everything else settled and tested and merged
before we spend another several weeks/months trying to make the
frobbing perfect.
quoted
+453 common umcg_create_group sys_umcg_create_group
+454 common umcg_destroy_group sys_umcg_destroy_group
This is basically needed for cross-server things, right? What we in the
kernel would call SMP. Some thoughts on that below.
Yes, right.
quoted
+455 common umcg_poll_worker sys_umcg_poll_worker
Shouldn't this be called idle or something, instead of poll, the whole
point of having this syscall is to that you can indeed go idle.
That's another way of looking at it. Yes, this means the server idles
until a worker becomes available. How would you call it? umcg_idle()?
Userspace can implement polling just fine without help:
for (;;) {
struct umcg_task *runnable = xchg(me->umcg_runnable_ptr, NULL);
if (runnable) {
// put them on a list and run one
}
cpu_relax();
}
comes to mind (see below).
quoted
+456 common umcg_run_worker sys_umcg_run_worker
This I'm confused about again.. there is no fundamental difference
between a worker or server, they're all the same.
I don't see it this way. A server runs (on CPU) by itself and blocks
when there is a worker attached; a worker runs (on CPU) only when it
has a (blocked) server attached to it and, when the worker blocks, its
server detaches and runs another worker. So workers and servers are
the opposite of each other.
quoted
+457 common umcg_preempt_worker sys_umcg_preempt_worker
+ */
+#define UMCG_TASK_NONE 0
+/* UMCG server states. */
+#define UMCG_TASK_POLLING 1
+#define UMCG_TASK_SERVING 2
+#define UMCG_TASK_PROCESSING 3
I get POLLING, although per the above, this probably wants to be IDLE.
Ack.
What are the other two again? That is, along with the diagram, each
state wants a description.
SERVING: the server is blocked, its attached worker is running
PROCESSING: the server is running (= processing a block or wake
event), has no running worker attached
Both of these states are different from POLLING/IDLE and from each other.
Weird order, also I can't remember why we need the UNBLOCKED, isn't that
the same as the RUNNABLE, or did we want to distinguish the state were
we're no longer BLOCKED but the user scheduler hasn't yet put us on it's
ready queue (IOW, we're on the runnable_ptr list, see below).
Yes, UNBLOCKED it a transitory state meaning the worker's blocking
operation has completed, but the wake event hasn't been delivered to
the userspace yet (and so the worker it not yet RUNNABLE)
[...]
quoted
+struct umcg_task {
+ /**
+ * @state: the current state of the UMCG task described by this struct.
+ *
+ * UMCG task state:
+ * bits 0 - 7: task state;
+ * bits 8 - 15: state flags;
+ * bits 16 - 23: reserved; must be zeroes;
+ * bits 24 - 31: for userspace use.
+ */
+ uint32_t state;
+} __attribute((packed, aligned(4 * sizeof(uint64_t))));
So last time I really looked at this it looked something like this:
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_server_tid; /* r */
u32 umcg_next_tid; /* r */
u32 __hole__;
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
(where r/w is from the kernel's pov)
(also see uapi/linux/rseq.h's ptr magic)
I tried doing it this way, i.e. to only have only userspace struct
added (without kernel-only data), and I found it really cumbersome and
inconvenient and much slower than the proposed implementation. For
example, when a worker blocks, it seems working with "struct
task_struct *peer" to get to the worker's server is easy and
straightforward; reading server_tid from userspace, then looking up
the task and only then doing what is needed (change state and wakeup)
is ... unnecessary? Also validating things becomes really important
but difficult (what if the user put something weird in
umcg_server_tid? or the ptr fields?). In my proposed implementation
only the state is user-writable, and it does not really affect most of
the kernel-side work.
Why do you think everything should be in the userspace memory?
So a PF_UMCG_WORKER would be added to sched_submit_work()'s PF_*_WORKER
path to capture these tasks blocking. The umcg_sleeping() hook added
there would:
put_user(BLOCKED, umcg_task->umcg_status);
tid = get_user(umcg_task->next_tid);
if (!tid)
tid = get_user(umcg_task->umcg_server_tid);
umcg_server = find_task(tid);
/* append to blocked list */
umcg_task->umcg_blocked_ptr = umcg_server->umcg_blocked_ptr;
umcg_server->umcg_blocked_ptr = umcg_task;
// with some user_cmpxchg() sprinkled on to make it an atomic single
// linked list, we can borrow from futex_atomic_cmpxchg_inatomic().
/* capture return to user */
add_task_work(current, ¤t->umcg->task_work, TWA_RESUME);
umcg_server->state = RUNNING;
wake_up_process(umcg_server);
That task_work would, as the comment says, capture the return to user,
and do something like:
put_user(RUNNABLE, umcg_task->umcg_status);
tid = get_user(umcg_task->umcg_server_tid);
umcg_server = find_task(tid);
/* append to runable list */
umcg_task->umcg_runnable_ptr = umcg_server->umcg_runnable_ptr;
umcg_server->umcg_runnable_ptr = umcg_task;
// same as above, this wants some user cmpxchg
umcg_wait();
And for that we had something like:
void umcg_wait(void)
{
u32 state;
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);
if (get_user(state, current->umcg->state))
break;
if (state == UMCG_RUNNING)
break;
if (signal_pending(current))
break;
schedule();
}
__set_current_state(TASK_RUNNING);
}
Which would wait until the userspace admission logic lets us rip by
setting state to RUNNING and prodding us with a sharp stick.
This all ensures that when a UMCG task goes to sleep, we mark ourselves
BLOCKED, we add ourselves to a user visible blocked list and wake the
owner of that blocked list.
We can either pre-select some task to run after us (next_tid) or it'll
pick the dedicated server task we're assigned to (server_tid).
Any time a task wakes up, it needs to check the blocked list and update
userspace ready queues and the sort, after which it can either run
things if it's a worker or pick another task to run if that's its work
(a server isn't special in this regard).
This was the absolute bare minimum, and I'm not seeing any of that here.
Nor an explanation of what there actually is :/
On top of this there's 'fun' questions about signals, ptrace and
umcg_preemption to be answered.
I think we want to allow signals to happen to UMCG RUNNABLE tasks, but
have them resume umcg_wait() on sigreturn.
I've not re-read the discussion with tglx on ptrace, he had some cute
corner cases IIRC.
The whole preemption thing should be doable with a task_work. Basically
check if the victim is RUNNING, send it TWA_SIGNAL to handle the task
work, the task_work would attempt a RUNNING->RUNNABLE (cmpxchg)
transition, success thereof needs to be propagated back to the syscall
and returned.
Adding preemption also means you have to deal with appending to
runnable_ptr list when the server isn't reaily available (most times).
Now on to those group things; they would basically replace the above
server_tid with a group/list of related server tasks, right? So why not
do so, litearlly:
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_next_tid; /* r */
u64 umcg_server_ptr; /* r */
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
Then have the kernel iterate the umcg_server_ptr list, looking for an
available (RUNNING or IDLE) server, also see the preemption point above.
This does, however, require a umcg_task to pid translation, which we've
so far avoided :/ OTOH it makes that grouping crud a user problem and we
can make the syscalls go away (and I that CRUI would like this better
too).
All of the code above assumes userspace-only data. I did not look into
every detail of your suggestions because I want to make sure we first
agree on this: do we keep every bit of information in the userspace
(other than "struct umcg_task __user *" pointer in task_struct) or do
we have some kernel-only details as well?
So IMPORTANT QUESTION #2: why would we want to keep __everything__ in
the userspace memory? I understand that CRIU would like this, but
given that the implementation would at a minimum have to
1. read a umcg_server_ptr (points to the server's umcg_task)
2. get the server tid out of it (presumably by reading a field from
the server's umcg_task; what if the tid is wrong?)
3. do a tid lookup
to get a task_struct pointer, it will be slower; I am also not sure it
call be done safely at all: with kernel-side data and I can do rcu
locking, task locking, etc. to ensure that the value I got does not
change while I'm working with it; with userspace data, a lot of races
will have to be specially coded for that can be easily handled by
kernel-side rcu locks or spin locks... Maybe this is just my ignorance
showing, and indeed things can be done simply and easily with
userspace-only data, but I am not sure how.
A common example:
- worker W1 with server S1 calls umcg_wait()
- worker W2 with server S2 calls umcg_swap(W1)
If due to preemption and other concurrency weirdness the two syscalls
above race with each other, each trying to change the server assigned
to W1. I can easily handle the race by doing kernel-side locking;
without kernel-side locking (cannot do rcu locks and/or spin locks
while accessing userspace data) I am not sure how to handle the race.
Maybe it is possible with careful atomic writes to states and looping
to handle this specific race (what if the userspace antagonistically
writes to the same location? will it force the syscall to spin
indefinitely?); but with proper locking many potential races can be
handled; with atomic ops and looping it is more difficult... Will we
have to add a lock to struct umcg_task? And acquire it from the kernel
side? And worry about spinning forever?
quoted
+static int do_context_switch(struct task_struct *next)
+{
[...]
quoted
+}
+
+static int do_wait(void)
+{
[...]
quoted
+}
Both these are fundamentally buggered for not having a loop.
As I mentioned above, the loop is in the userpace.
[...]
I'm thinking this might want to be a user cmpxchg from RUNNABLE->RUNNING.
You need to deal with concurrent wakeups.
This is done in the userspace - much easier to do it there...
In summary, two IMPORTANT QUESTIONS:
1. thin vs fat syscalls: can we push some code/logic to the userspace
(state changes, looping/retries), or do we insist on syscalls handling
everything? Please have in mind that even if we choose the second
approach (fat syscalls), the userspace will most likely still have to
do everything it does under the first option just to handle
signals/interrupts (i.e. unscheduled wakeups);
2. kernel-side data vs userspace-only: can we avoid having kernel-side
data? More specifically, what alternatives to rcu_read_lock and/or
task_lock are available when working with userspace data?
When these two questions are answered to everybody's satisfaction, we
can discuss this patchset/library/API in more detail.
Thanks,
Peter
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-06-10 18:03:10
On Wed, Jun 09, 2021 at 01:18:59PM -0700, Peter Oskolkov wrote:
On Wed, Jun 9, 2021 at 5:55 AM Peter Zijlstra [off-list ref] wrote:
Finally, a high-level review - thanks a lot, Peter! My comments below,
and two high-level "important questions" at the end of my reply (with
some less important questions here and there).
[...]
quoted
You present an API without explaining, *at*all*, how it's supposed to be
used and I can't seem to figure it out from the implementation either :/
Urgh, you write RST :-( That sorta helps, but I'm still unclear on a
number of things, more below.
Or do you mean it more narrowly, i.e. I do not explain syscalls in
detail? This assessment I agree with - my approach was/is to finalize
the userpace API (libumcg) first, and make the userspace vs kernel
decisions later.
Yeah, I couldn't figure out how to use the syscalls and thus how to
interpret their implementation. A little more in the way of comments
would've been helpful.
For example, you wonder why there is no looping in umcg_wait
(do_wait). This is because the looping happens in the userspace in
libumcg. My overall approach was to make the syscalls as simple as
possible and push extra logic to the userspace.
So a simple comment on the syscall that says:
Userspace is expected to do:
do {
sys_umcg_wait();
} while (smp_load_acquire(&umcg_task->state) != RUNNING);
would've made all the difference. It provides context.
It seems that this approach is not resonating with kernel
developers/maintainers - you are the third person asking why there is
no looping in sys_umcg_wait, despite the fact that I explicitly
mentioned pushing it out to the userspace.
We've been trained, through years of 'funny' bugs, to go 'BUG BUG BUG'
when schedule() is not in a loop. And pushing the loop to userspace has
me all on edge for being 'weird'.
Let me try to make my case once more here.
umcg_wait/umcg_wake: the RUNNABLE/RUNNING state changes, checks, and
looping happen in the userspace (libumcg - see umcg_wait/umcg_wake in
patch 5 here: https://lore.kernel.org/patchwork/patch/1433971/), while
the syscalls simply sleep/wake. I find doing it in the userspace is
much simpler and easier than in the kernel, as state reads and writes
are just atomic memory accesses; in the kernel it becomes much more
difficult - rcu locked sections, tasks locked, etc.
Small difficulties as far as things go I think. The worst part is having
to do arch asm for the userspace cmpxchg. Luckily we can crib/share with
futex there.
On the other hand I agree that having syscalls more logically
complete, in the sense that they do not require much hand-holding and
retries from the userspace, is probably better from the API design
perspective. My worry here is that state validation and retries in the
userspace are unavoidable, and so going the usual way we will end up
with retry loops both in the kernel and in the userspace.
Can you expand on where you'd see the need for userspace to retry?
The canonical case in my mind is where a task, that's been BLOCKED in
kernelspace transitions to UNBLOCK/RUNNABLE in return-to-user and waits
for RUNNING.
Once it gets RUNNING, userspace can assume it can just go. It will never
have to re-check, because there's no way RUNNING can go away again. The
only way for RUNNING to become anything else, is setting it yourself
and/or doing a syscall.
Also, by having the BLOCKED thing block properly in return-to-user, you
don't have to wrap *all* the userspace syscall invocations. If you let
it return early, you get to wrap syscalls, which is both fragile and
bad for performance.
So I pose this IMPORTANT QUESTION #1 to you that I hope to get a clear
answer to: it is strongly preferable to have syscalls be "logically
complete" in the sense that they retry things internally, and in
generally try to cover all possible corner cases; or, alternatively,
is it OK to make syscalls lightweight but "logically incomplete", and
have the accompanied userspace wrappers do all of the heavy lifting
re: state changes/validation, retries, etc.?
Intuitively I'd go with complete. I'd have never even considered the
incomplete option. But let me try and get my head around the incomplete
cases.
Oooh, I found the BLOCKED stuff, you hid it inside the grouping patch,
that makes no sense :-( Reason I'm looking is that I don't see how you
get around the blocked and runnable lists. You have to tell userspace
about them.
FWIW: I think you placed umcg_on_block() wrong, it needs to be before
the terrible PI thing. Also, like said, please avoid yet another branch
here by using PF_UMCG_WORKER.
I see two additional benefits of thin/lightweight syscalls:
- reading userspace state is needed much less often (e.g. my umcg_wait
and umcg_wake syscalls do not access userspace data at all - also see
my "second important question" below)
It is also broken I think, best I can make of it is somsething like
this:
WAIT WAKE
if (smp_load_acquire(&state) == RUNNING)
return;
state = RUNNING;
do {
sys_umcg_wait()
{
in_wait = true;
sys_umcg_wake()
{
if (in_wait)
wake_up_process()
}
set_current_state(INTERRUPTIBLE);
schedule();
in_wait = false;
}
} while (smp_load_acquire(&state) != RUNNING);
missed wakeup, 'forever' stuck. You have to check your blocking
condition between setting state and scheduling. And if you do that, you
have a 'fat' syscall again.
- looping in the kernel, combined with reading/writing to userspace
memory, can easily lead to spinning in the kernel (e.g. trying to
atomically change a variable and looping until succeeding)
I don't imagine spinning in kernel or userspace matters.
quoted
Also, do we really need unregister over just letting a task
exit? Is there a sane use-case where task goes in and out of service?
I do not know of a specific use case here. On the other hand, I do not
know of a specific use case to unregister RSEQ, but the capability is
there. Maybe the assumption is that the userspace memory passed to the
kernel in register() may be freed before the task exits, and so there
should be a way to tell the kernel to no longer use it?
Fair enough I suppose.
quoted
quoted
+450 common umcg_wait sys_umcg_wait
+451 common umcg_wake sys_umcg_wake
Right, except I'm confused by the proposed implementation. I thought the
whole point was to let UMCG tasks block in kernel, at which point we'd
change their state to BLOCKED and have userspace select another task to
run. Such BLOCKED tasks would then also be captured before they return
to userspace, i.e. the whole admission scheduler thing.
I don't see any of that in these patches. So what are they actually
implementing? I can't find enough clues to tell :-(
So you have some of it, I just didn't find it because it's hidding in
that grouping thing.
quoted
quoted
+452 common umcg_swap sys_umcg_swap
You're presenting it like a pure optimization, but IIRC this is what
enables us to frob the scheduler state to ensure the whole thing is seen
(to the rest of the system) as the M server tasks, instead of the
constellation of N+M worker and server tasks.
Yes, you recall it correctly.
quoted
Also, you're not doing any of the frobbing required.
This is because I consider the frobbing a (very) nice to have rather
than a required feature, and so I am hoping to argue about how to
properly do it in later patchsets. This whole thing (UMCG) will be
extremely useful even without runtime accounting hacking and whatnot,
and so I hope to have everything else settled and tested and merged
before we spend another several weeks/months trying to make the
frobbing perfect.
Sure, not saying you need the frobbing from the get-go, but it's a much
stronger argument for having the API in the first place. So mentioning
this property (along with a TODO) is a stronger justification.
This goes to *why again. It's fairly easy to see what from the code, but
code rarely explains why.
That said; if we do: @next_pid, we might be able to do away with this. A
!RUNING transition will attempt to wake-and-switch to @next_tid. This is
BLOCKED from syscall or explicit using umcg_wait().
quoted
quoted
+455 common umcg_poll_worker sys_umcg_poll_worker
Shouldn't this be called idle or something, instead of poll, the whole
point of having this syscall is to that you can indeed go idle.
That's another way of looking at it. Yes, this means the server idles
until a worker becomes available. How would you call it? umcg_idle()?
I'm trying to digest the thing; it's doing *far* more than just idling,
but yes, sys_umcg_idle() or something.
quoted
This I'm confused about again.. there is no fundamental difference
between a worker or server, they're all the same.
I don't see it this way. A server runs (on CPU) by itself and blocks
when there is a worker attached; a worker runs (on CPU) only when it
has a (blocked) server attached to it and, when the worker blocks, its
server detaches and runs another worker. So workers and servers are
the opposite of each other.
So I was viewing the server more like the idle thread, its 'work' is
idle, which is always available.
quoted
quoted
+ */
+#define UMCG_TASK_NONE 0
+/* UMCG server states. */
+#define UMCG_TASK_POLLING 1
+#define UMCG_TASK_SERVING 2
+#define UMCG_TASK_PROCESSING 3
I get POLLING, although per the above, this probably wants to be IDLE.
Ack.
quoted
What are the other two again? That is, along with the diagram, each
state wants a description.
SERVING: the server is blocked, its attached worker is running
PROCESSING: the server is running (= processing a block or wake
event), has no running worker attached
Both of these states are different from POLLING/IDLE and from each other.
But if we view the server as the worker with work 'idle', then serving
becomes RUNNABLE and PROCESSING becomes RUNNING, right?
And sys_run_worker(next); becomes:
self->state = RUNNABLE;
self->next_tid = next;
sys_umcg_wait();
The question is if we need an explicit IDLE state along with calling
sys_umcg_idle(). I can't seem to make up my mind on that.
Weird order, also I can't remember why we need the UNBLOCKED, isn't that
the same as the RUNNABLE, or did we want to distinguish the state were
we're no longer BLOCKED but the user scheduler hasn't yet put us on it's
ready queue (IOW, we're on the runnable_ptr list, see below).
Yes, UNBLOCKED it a transitory state meaning the worker's blocking
operation has completed, but the wake event hasn't been delivered to
the userspace yet (and so the worker it not yet RUNNABLE)
So if I understand the proposal correctly the only possible option is
something like:
for (;;) {
next = user_sched_pick();
if (next) {
sys_umcg_run(next);
continue;
}
sys_umcg_poll(&next);
if (next) {
next->state = RUNNABLE;
user_sched_enqueue(next);
}
}
This seems incapable of implementing generic scheduling policies and has
a hard-coded FIFO policy.
The poll() thing cannot differentiate between: 'find new task' and 'go
idle'. So you cannot keep running it until all new tasks are found.
But you basically get to do a syscall to discover every new task, while
the other proposal gets you a user visible list of new tasks, no
syscalls needed at all.
It's also not quite clear to me what you do about RUNNING->BLOCKED, how
does the userspace scheduler know to dequeue a task?
My proposal gets you something like:
for (;;) {
self->state = RUNNABLE;
self->next_tid = 0; // next == self == server -> idle
p = xchg(self->blocked_ptr, NULL);
while (p) {
n = new->blocked_ptr;
user_sched_dequeue(p);
p = n;
}
// Worker can have unblocked again before we got here,
// hence we need to process blocked before runnable.
// Worker cannot have blocked again, since we didn't
// know it was runnable, hence it cannot have ran again.
p = xchg(self->runnable_ptr, NULL);
while (p) {
n = new->runnable_ptr;
user_sched_enqeue(p);
p = n;
}
n = user_sched_pick();
if (n)
self->next_tid = n->tid;
// new self->*_ptr state will have changed self->state
// to RUNNING and we'll not switch to ->next.
sys_umcg_wait();
// self->state == RUNNING
}
This allows you to implement arbitrary policies and instantly works with
preemption once we implement that. Preemption would put the running
worker in RUNNABLE, mark the server RUNNING and switch.
Hmm, looking at it written out like that, we don't need sys_umcg_wake(),
sys_umcg_swap() at all.
Anyway, and this is how I got here, UNBLOCKED is not required because we
cannot run it before we've observed it RUNNABLE. Yes the state exists
where it's no longer BLOCKED, and it's not yet on the runqueue, but when
we don't know it's RUNNABLE we'll not pick it, so its moot.
quoted
So last time I really looked at this it looked something like this:
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_server_tid; /* r */
u32 umcg_next_tid; /* r */
u32 __hole__;
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
(where r/w is from the kernel's pov)
(also see uapi/linux/rseq.h's ptr magic)
I tried doing it this way, i.e. to only have only userspace struct
added (without kernel-only data), and I found it really cumbersome and
inconvenient and much slower than the proposed implementation.
For example, when a worker blocks, it seems working with "struct
task_struct *peer" to get to the worker's server is easy and
straightforward; reading server_tid from userspace, then looking up
the task and only then doing what is needed (change state and wakeup)
is ... unnecessary?
Is find_task_by_vpid() really that slow? The advantage of having it in
userspace is that you can very easily change 'affinities' of the
workers. You can simply set ->server_tid and it goes elsewhere.
Also validating things becomes really important
but difficult (what if the user put something weird in
umcg_server_tid? or the ptr fields?).
If find_task_by_vpid() returns NULL, we return -ESRCH. If the user
cmpxchg returns -EFAULT we pass along the message. If userspace put a
valid but crap pointer in it, userspace gets to keep the pieces.
In my proposed implementation only the state is user-writable, and it
does not really affect most of the kernel-side work.
Why do you think everything should be in the userspace memory?
Because then we avoid all the kernel state and userspace gets to have
all the state without endless syscalls.
Note that with the proposal, per the above, we're at:
enum {
UMCG_STATE_RUNNING,
UMCG_STATE_RUNABLE,
UMCG_STATE_BLOCKED,
};
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_server_tid; /* r */
u32 umcg_next_tid; /* r */
u32 umcg_tid; /* r */
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
/*
* Register current's UMCG state.
*/
sys_umcg_register(struct umcg_task *self, unsigned int flags);
/*
* Just 'cause.
*/
sys_umcg_unregister(struct umcg_task *self)
/*
* UMCG context switch.
*/
sys_umcg_wait(u64 time, unsigned int flags)
{
unsigned int state = RUNNABLE;
unsigned int tid;
if (self->state == RUNNING)
return;
tid = self->next_tid;
if (!tid)
tid = self->server_tid;
if (tid == self->server_tid && tid == self->tid)
return umcg_idle(time, flags);
next = find_process_by_pid(tid);
if (!next) {
return -ESRCH;
ret = user_try_cmpxchg(next->umcg->state, &state, RUNNING);
if (!ret)
ret = -EBUSY;
if (ret < 0)
return ret;
return umcg_switch_to(next);
}
With this (and the BLOCKING bits outlined last time) we can implement
full N:1 userspace scheduling (UP).
( Note that so far we assume all UMCG workers share the same address
space, otherwise the user_try_cmpxchg() doesn't work. )
And I _think_ you can do the whole SMP thing in userspace as well, just
have the servers share queue state and reassign ->server_tid where
needed. No additional syscalls required.
All of the code above assumes userspace-only data. I did not look into
every detail of your suggestions because I want to make sure we first
agree on this: do we keep every bit of information in the userspace
(other than "struct umcg_task __user *" pointer in task_struct) or do
we have some kernel-only details as well?
Most of the kernel state you seem to have implemented seems to limit
flexibility / implement specific policy. All because apparently
find_task_by_vpid() is considered expensive?
You've enangled the whole BLOCKING stuff with the SMP stuff. And by
putting that state in the kernel you've limited flexibility.
Also, if you don't have kernel state it can't go out of sync and cause
problems.
So IMPORTANT QUESTION #2: why would we want to keep __everything__ in
the userspace memory? I understand that CRIU would like this, but
given that the implementation would at a minimum have to
1. read a umcg_server_ptr (points to the server's umcg_task)
2. get the server tid out of it (presumably by reading a field from
the server's umcg_task; what if the tid is wrong?)
3. do a tid lookup
So if we leave SMP as an exercise in scheduling queue management, And
implement the above, then you need:
- copy_from_user()/get_user() for the first 4 words
- find_task_by_vpid()
that gets you a task pointer, then we get to update a blocked_ptr.
If anything goes wrong, simply return an error and let userspace sort it
out.
to get a task_struct pointer, it will be slower; I am also not sure it
call be done safely at all: with kernel-side data and I can do rcu
locking, task locking, etc. to ensure that the value I got does not
change while I'm working with it; with userspace data, a lot of races
will have to be specially coded for that can be easily handled by
kernel-side rcu locks or spin locks... Maybe this is just my ignorance
showing, and indeed things can be done simply and easily with
userspace-only data, but I am not sure how.
A common example:
- worker W1 with server S1 calls umcg_wait()
- worker W2 with server S2 calls umcg_swap(W1)
If due to preemption and other concurrency weirdness the two syscalls
above race with each other, each trying to change the server assigned
to W1. I can easily handle the race by doing kernel-side locking;
without kernel-side locking (cannot do rcu locks and/or spin locks
while accessing userspace data) I am not sure how to handle the race.
Maybe it is possible with careful atomic writes to states and looping
to handle this specific race (what if the userspace antagonistically
writes to the same location? will it force the syscall to spin
indefinitely?); but with proper locking many potential races can be
handled; with atomic ops and looping it is more difficult... Will we
have to add a lock to struct umcg_task? And acquire it from the kernel
side? And worry about spinning forever?
What would you want locking for? I really don't see a problem here.
Both blocked_ptr and runnable_ptr are cmpxchg single-linked-lists. Yes
they can spin a little, but that's not a new problem, futex has all
that.
And ->state only needs single cmpxchg ops, no loops, either we got the
wakeup, or we didn't. The rest is done with memory ordering:
server worker
self->state = RUNNABLE; self->state = BLOCKED;
head = xchg(list, NULL) add_to_list(self, &server->blocked_ptr);
if (try_cmpxchg_user(&server->umcg->state, RUNNABLE, RUNNING) > 0)
sys_umcg_wait() wake_up_process(server);
Either server sees the add, or we see it's RUNNABLE and wake it up
(or both).
If anything on the BLOCKED side goes wrong (bad pointers, whatever),
have it segfault.
<edit> Ooh, I forgot you can't just go wake the server when it's running
something else... so that does indeed need more states/complication, the
ordering argument stands though. We'll need something like
self->current_tid or somesuch </edit>
What are the alternatives? I just picked what the futex code uses.
u64 nanoseconds. Not sure tglx really wants to do that though, but
still, timespec is a terrible thing.
In summary, two IMPORTANT QUESTIONS:
1. thin vs fat syscalls: can we push some code/logic to the userspace
(state changes, looping/retries), or do we insist on syscalls handling
everything?
Well, the way I see it it's a trade of what is handled where. I get a
smaller API (although I'm sure I've forgotten something trivial again
that wrecks everything <edit> I did :/ </edit>) and userspace gets to
deal with all of SMP and scheduling policies.
You hard-coded a global-fifo and had a enormous number of syscalls and
needed to wrap every syscall invocation in order to fix up the return.
Please have in mind that even if we choose the second
approach (fat syscalls), the userspace will most likely still have to
do everything it does under the first option just to handle
signals/interrupts (i.e. unscheduled wakeups);
IIRC sigreturn goes back into the kernel and we can resume blocking
there.
2. kernel-side data vs userspace-only: can we avoid having kernel-side
data? More specifically, what alternatives to rcu_read_lock and/or
task_lock are available when working with userspace data?
What would you want locked and why?
Anyway, this email is far too long again (basically took me all day :/),
hope it helps a bit. Thomas is stuck fixing XSAVE disasters, but I'll
ask him to chime in once that's done.
From: Peter Oskolkov <hidden> Date: 2021-06-10 20:06:18
On Thu, Jun 10, 2021 at 11:02 AM Peter Zijlstra [off-list ref] wrote:
Thanks a lot for the detailed reply!
I'll try again the data-in-userspace-only route (= everything in TLS):
if you are right and everything can be done without needing to lock
anything - great!
The last time I tried it I could not do it properly/safely, though,
because I could not fix races without rcu and/or spin locking stuff,
which was impossible with data in the userspace. I don't remember the
specifics now, though...
Thanks,
Peter
On Wed, Jun 09, 2021 at 01:18:59PM -0700, Peter Oskolkov wrote:
quoted
On Wed, Jun 9, 2021 at 5:55 AM Peter Zijlstra [off-list ref] wrote:
Finally, a high-level review - thanks a lot, Peter! My comments below,
and two high-level "important questions" at the end of my reply (with
some less important questions here and there).
[...]
quoted
You present an API without explaining, *at*all*, how it's supposed to be
used and I can't seem to figure it out from the implementation either :/
Urgh, you write RST :-( That sorta helps, but I'm still unclear on a
number of things, more below.
quoted
Or do you mean it more narrowly, i.e. I do not explain syscalls in
detail? This assessment I agree with - my approach was/is to finalize
the userpace API (libumcg) first, and make the userspace vs kernel
decisions later.
Yeah, I couldn't figure out how to use the syscalls and thus how to
interpret their implementation. A little more in the way of comments
would've been helpful.
quoted
For example, you wonder why there is no looping in umcg_wait
(do_wait). This is because the looping happens in the userspace in
libumcg. My overall approach was to make the syscalls as simple as
possible and push extra logic to the userspace.
So a simple comment on the syscall that says:
Userspace is expected to do:
do {
sys_umcg_wait();
} while (smp_load_acquire(&umcg_task->state) != RUNNING);
would've made all the difference. It provides context.
quoted
It seems that this approach is not resonating with kernel
developers/maintainers - you are the third person asking why there is
no looping in sys_umcg_wait, despite the fact that I explicitly
mentioned pushing it out to the userspace.
We've been trained, through years of 'funny' bugs, to go 'BUG BUG BUG'
when schedule() is not in a loop. And pushing the loop to userspace has
me all on edge for being 'weird'.
quoted
Let me try to make my case once more here.
umcg_wait/umcg_wake: the RUNNABLE/RUNNING state changes, checks, and
looping happen in the userspace (libumcg - see umcg_wait/umcg_wake in
patch 5 here: https://lore.kernel.org/patchwork/patch/1433971/), while
the syscalls simply sleep/wake. I find doing it in the userspace is
much simpler and easier than in the kernel, as state reads and writes
are just atomic memory accesses; in the kernel it becomes much more
difficult - rcu locked sections, tasks locked, etc.
Small difficulties as far as things go I think. The worst part is having
to do arch asm for the userspace cmpxchg. Luckily we can crib/share with
futex there.
quoted
On the other hand I agree that having syscalls more logically
complete, in the sense that they do not require much hand-holding and
retries from the userspace, is probably better from the API design
perspective. My worry here is that state validation and retries in the
userspace are unavoidable, and so going the usual way we will end up
with retry loops both in the kernel and in the userspace.
Can you expand on where you'd see the need for userspace to retry?
The canonical case in my mind is where a task, that's been BLOCKED in
kernelspace transitions to UNBLOCK/RUNNABLE in return-to-user and waits
for RUNNING.
Once it gets RUNNING, userspace can assume it can just go. It will never
have to re-check, because there's no way RUNNING can go away again. The
only way for RUNNING to become anything else, is setting it yourself
and/or doing a syscall.
Also, by having the BLOCKED thing block properly in return-to-user, you
don't have to wrap *all* the userspace syscall invocations. If you let
it return early, you get to wrap syscalls, which is both fragile and
bad for performance.
quoted
So I pose this IMPORTANT QUESTION #1 to you that I hope to get a clear
answer to: it is strongly preferable to have syscalls be "logically
complete" in the sense that they retry things internally, and in
generally try to cover all possible corner cases; or, alternatively,
is it OK to make syscalls lightweight but "logically incomplete", and
have the accompanied userspace wrappers do all of the heavy lifting
re: state changes/validation, retries, etc.?
Intuitively I'd go with complete. I'd have never even considered the
incomplete option. But let me try and get my head around the incomplete
cases.
Oooh, I found the BLOCKED stuff, you hid it inside the grouping patch,
that makes no sense :-( Reason I'm looking is that I don't see how you
get around the blocked and runnable lists. You have to tell userspace
about them.
FWIW: I think you placed umcg_on_block() wrong, it needs to be before
the terrible PI thing. Also, like said, please avoid yet another branch
here by using PF_UMCG_WORKER.
quoted
I see two additional benefits of thin/lightweight syscalls:
- reading userspace state is needed much less often (e.g. my umcg_wait
and umcg_wake syscalls do not access userspace data at all - also see
my "second important question" below)
It is also broken I think, best I can make of it is somsething like
this:
WAIT WAKE
if (smp_load_acquire(&state) == RUNNING)
return;
state = RUNNING;
do {
sys_umcg_wait()
{
in_wait = true;
sys_umcg_wake()
{
if (in_wait)
wake_up_process()
}
set_current_state(INTERRUPTIBLE);
schedule();
in_wait = false;
}
} while (smp_load_acquire(&state) != RUNNING);
missed wakeup, 'forever' stuck. You have to check your blocking
condition between setting state and scheduling. And if you do that, you
have a 'fat' syscall again.
quoted
- looping in the kernel, combined with reading/writing to userspace
memory, can easily lead to spinning in the kernel (e.g. trying to
atomically change a variable and looping until succeeding)
I don't imagine spinning in kernel or userspace matters.
quoted
quoted
Also, do we really need unregister over just letting a task
exit? Is there a sane use-case where task goes in and out of service?
I do not know of a specific use case here. On the other hand, I do not
know of a specific use case to unregister RSEQ, but the capability is
there. Maybe the assumption is that the userspace memory passed to the
kernel in register() may be freed before the task exits, and so there
should be a way to tell the kernel to no longer use it?
Fair enough I suppose.
quoted
quoted
quoted
+450 common umcg_wait sys_umcg_wait
+451 common umcg_wake sys_umcg_wake
Right, except I'm confused by the proposed implementation. I thought the
whole point was to let UMCG tasks block in kernel, at which point we'd
change their state to BLOCKED and have userspace select another task to
run. Such BLOCKED tasks would then also be captured before they return
to userspace, i.e. the whole admission scheduler thing.
I don't see any of that in these patches. So what are they actually
implementing? I can't find enough clues to tell :-(
So you have some of it, I just didn't find it because it's hidding in
that grouping thing.
quoted
quoted
quoted
+452 common umcg_swap sys_umcg_swap
You're presenting it like a pure optimization, but IIRC this is what
enables us to frob the scheduler state to ensure the whole thing is seen
(to the rest of the system) as the M server tasks, instead of the
constellation of N+M worker and server tasks.
Yes, you recall it correctly.
quoted
Also, you're not doing any of the frobbing required.
This is because I consider the frobbing a (very) nice to have rather
than a required feature, and so I am hoping to argue about how to
properly do it in later patchsets. This whole thing (UMCG) will be
extremely useful even without runtime accounting hacking and whatnot,
and so I hope to have everything else settled and tested and merged
before we spend another several weeks/months trying to make the
frobbing perfect.
Sure, not saying you need the frobbing from the get-go, but it's a much
stronger argument for having the API in the first place. So mentioning
this property (along with a TODO) is a stronger justification.
This goes to *why again. It's fairly easy to see what from the code, but
code rarely explains why.
That said; if we do: @next_pid, we might be able to do away with this. A
!RUNING transition will attempt to wake-and-switch to @next_tid. This is
BLOCKED from syscall or explicit using umcg_wait().
quoted
quoted
quoted
+455 common umcg_poll_worker sys_umcg_poll_worker
Shouldn't this be called idle or something, instead of poll, the whole
point of having this syscall is to that you can indeed go idle.
That's another way of looking at it. Yes, this means the server idles
until a worker becomes available. How would you call it? umcg_idle()?
I'm trying to digest the thing; it's doing *far* more than just idling,
but yes, sys_umcg_idle() or something.
quoted
quoted
This I'm confused about again.. there is no fundamental difference
between a worker or server, they're all the same.
I don't see it this way. A server runs (on CPU) by itself and blocks
when there is a worker attached; a worker runs (on CPU) only when it
has a (blocked) server attached to it and, when the worker blocks, its
server detaches and runs another worker. So workers and servers are
the opposite of each other.
So I was viewing the server more like the idle thread, its 'work' is
idle, which is always available.
quoted
quoted
quoted
+ */
+#define UMCG_TASK_NONE 0
+/* UMCG server states. */
+#define UMCG_TASK_POLLING 1
+#define UMCG_TASK_SERVING 2
+#define UMCG_TASK_PROCESSING 3
I get POLLING, although per the above, this probably wants to be IDLE.
Ack.
quoted
What are the other two again? That is, along with the diagram, each
state wants a description.
SERVING: the server is blocked, its attached worker is running
PROCESSING: the server is running (= processing a block or wake
event), has no running worker attached
Both of these states are different from POLLING/IDLE and from each other.
But if we view the server as the worker with work 'idle', then serving
becomes RUNNABLE and PROCESSING becomes RUNNING, right?
And sys_run_worker(next); becomes:
self->state = RUNNABLE;
self->next_tid = next;
sys_umcg_wait();
The question is if we need an explicit IDLE state along with calling
sys_umcg_idle(). I can't seem to make up my mind on that.
Weird order, also I can't remember why we need the UNBLOCKED, isn't that
the same as the RUNNABLE, or did we want to distinguish the state were
we're no longer BLOCKED but the user scheduler hasn't yet put us on it's
ready queue (IOW, we're on the runnable_ptr list, see below).
Yes, UNBLOCKED it a transitory state meaning the worker's blocking
operation has completed, but the wake event hasn't been delivered to
the userspace yet (and so the worker it not yet RUNNABLE)
So if I understand the proposal correctly the only possible option is
something like:
for (;;) {
next = user_sched_pick();
if (next) {
sys_umcg_run(next);
continue;
}
sys_umcg_poll(&next);
if (next) {
next->state = RUNNABLE;
user_sched_enqueue(next);
}
}
This seems incapable of implementing generic scheduling policies and has
a hard-coded FIFO policy.
The poll() thing cannot differentiate between: 'find new task' and 'go
idle'. So you cannot keep running it until all new tasks are found.
But you basically get to do a syscall to discover every new task, while
the other proposal gets you a user visible list of new tasks, no
syscalls needed at all.
It's also not quite clear to me what you do about RUNNING->BLOCKED, how
does the userspace scheduler know to dequeue a task?
My proposal gets you something like:
for (;;) {
self->state = RUNNABLE;
self->next_tid = 0; // next == self == server -> idle
p = xchg(self->blocked_ptr, NULL);
while (p) {
n = new->blocked_ptr;
user_sched_dequeue(p);
p = n;
}
// Worker can have unblocked again before we got here,
// hence we need to process blocked before runnable.
// Worker cannot have blocked again, since we didn't
// know it was runnable, hence it cannot have ran again.
p = xchg(self->runnable_ptr, NULL);
while (p) {
n = new->runnable_ptr;
user_sched_enqeue(p);
p = n;
}
n = user_sched_pick();
if (n)
self->next_tid = n->tid;
// new self->*_ptr state will have changed self->state
// to RUNNING and we'll not switch to ->next.
sys_umcg_wait();
// self->state == RUNNING
}
This allows you to implement arbitrary policies and instantly works with
preemption once we implement that. Preemption would put the running
worker in RUNNABLE, mark the server RUNNING and switch.
Hmm, looking at it written out like that, we don't need sys_umcg_wake(),
sys_umcg_swap() at all.
Anyway, and this is how I got here, UNBLOCKED is not required because we
cannot run it before we've observed it RUNNABLE. Yes the state exists
where it's no longer BLOCKED, and it's not yet on the runqueue, but when
we don't know it's RUNNABLE we'll not pick it, so its moot.
quoted
quoted
So last time I really looked at this it looked something like this:
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_server_tid; /* r */
u32 umcg_next_tid; /* r */
u32 __hole__;
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
(where r/w is from the kernel's pov)
(also see uapi/linux/rseq.h's ptr magic)
I tried doing it this way, i.e. to only have only userspace struct
added (without kernel-only data), and I found it really cumbersome and
inconvenient and much slower than the proposed implementation.
quoted
For example, when a worker blocks, it seems working with "struct
task_struct *peer" to get to the worker's server is easy and
straightforward; reading server_tid from userspace, then looking up
the task and only then doing what is needed (change state and wakeup)
is ... unnecessary?
Is find_task_by_vpid() really that slow? The advantage of having it in
userspace is that you can very easily change 'affinities' of the
workers. You can simply set ->server_tid and it goes elsewhere.
quoted
Also validating things becomes really important
but difficult (what if the user put something weird in
umcg_server_tid? or the ptr fields?).
If find_task_by_vpid() returns NULL, we return -ESRCH. If the user
cmpxchg returns -EFAULT we pass along the message. If userspace put a
valid but crap pointer in it, userspace gets to keep the pieces.
quoted
In my proposed implementation only the state is user-writable, and it
does not really affect most of the kernel-side work.
Why do you think everything should be in the userspace memory?
Because then we avoid all the kernel state and userspace gets to have
all the state without endless syscalls.
Note that with the proposal, per the above, we're at:
enum {
UMCG_STATE_RUNNING,
UMCG_STATE_RUNABLE,
UMCG_STATE_BLOCKED,
};
struct umcg_task {
u32 umcg_status; /* r/w */
u32 umcg_server_tid; /* r */
u32 umcg_next_tid; /* r */
u32 umcg_tid; /* r */
u64 umcg_blocked_ptr; /* w */
u64 umcg_runnable_ptr; /* w */
};
/*
* Register current's UMCG state.
*/
sys_umcg_register(struct umcg_task *self, unsigned int flags);
/*
* Just 'cause.
*/
sys_umcg_unregister(struct umcg_task *self)
/*
* UMCG context switch.
*/
sys_umcg_wait(u64 time, unsigned int flags)
{
unsigned int state = RUNNABLE;
unsigned int tid;
if (self->state == RUNNING)
return;
tid = self->next_tid;
if (!tid)
tid = self->server_tid;
if (tid == self->server_tid && tid == self->tid)
return umcg_idle(time, flags);
next = find_process_by_pid(tid);
if (!next) {
return -ESRCH;
ret = user_try_cmpxchg(next->umcg->state, &state, RUNNING);
if (!ret)
ret = -EBUSY;
if (ret < 0)
return ret;
return umcg_switch_to(next);
}
With this (and the BLOCKING bits outlined last time) we can implement
full N:1 userspace scheduling (UP).
( Note that so far we assume all UMCG workers share the same address
space, otherwise the user_try_cmpxchg() doesn't work. )
And I _think_ you can do the whole SMP thing in userspace as well, just
have the servers share queue state and reassign ->server_tid where
needed. No additional syscalls required.
quoted
All of the code above assumes userspace-only data. I did not look into
every detail of your suggestions because I want to make sure we first
agree on this: do we keep every bit of information in the userspace
(other than "struct umcg_task __user *" pointer in task_struct) or do
we have some kernel-only details as well?
Most of the kernel state you seem to have implemented seems to limit
flexibility / implement specific policy. All because apparently
find_task_by_vpid() is considered expensive?
You've enangled the whole BLOCKING stuff with the SMP stuff. And by
putting that state in the kernel you've limited flexibility.
Also, if you don't have kernel state it can't go out of sync and cause
problems.
quoted
So IMPORTANT QUESTION #2: why would we want to keep __everything__ in
the userspace memory? I understand that CRIU would like this, but
given that the implementation would at a minimum have to
1. read a umcg_server_ptr (points to the server's umcg_task)
2. get the server tid out of it (presumably by reading a field from
the server's umcg_task; what if the tid is wrong?)
3. do a tid lookup
So if we leave SMP as an exercise in scheduling queue management, And
implement the above, then you need:
- copy_from_user()/get_user() for the first 4 words
- find_task_by_vpid()
that gets you a task pointer, then we get to update a blocked_ptr.
If anything goes wrong, simply return an error and let userspace sort it
out.
quoted
to get a task_struct pointer, it will be slower; I am also not sure it
call be done safely at all: with kernel-side data and I can do rcu
locking, task locking, etc. to ensure that the value I got does not
change while I'm working with it; with userspace data, a lot of races
will have to be specially coded for that can be easily handled by
kernel-side rcu locks or spin locks... Maybe this is just my ignorance
showing, and indeed things can be done simply and easily with
userspace-only data, but I am not sure how.
A common example:
- worker W1 with server S1 calls umcg_wait()
- worker W2 with server S2 calls umcg_swap(W1)
If due to preemption and other concurrency weirdness the two syscalls
above race with each other, each trying to change the server assigned
to W1. I can easily handle the race by doing kernel-side locking;
without kernel-side locking (cannot do rcu locks and/or spin locks
while accessing userspace data) I am not sure how to handle the race.
Maybe it is possible with careful atomic writes to states and looping
to handle this specific race (what if the userspace antagonistically
writes to the same location? will it force the syscall to spin
indefinitely?); but with proper locking many potential races can be
handled; with atomic ops and looping it is more difficult... Will we
have to add a lock to struct umcg_task? And acquire it from the kernel
side? And worry about spinning forever?
What would you want locking for? I really don't see a problem here.
Both blocked_ptr and runnable_ptr are cmpxchg single-linked-lists. Yes
they can spin a little, but that's not a new problem, futex has all
that.
And ->state only needs single cmpxchg ops, no loops, either we got the
wakeup, or we didn't. The rest is done with memory ordering:
server worker
self->state = RUNNABLE; self->state = BLOCKED;
head = xchg(list, NULL) add_to_list(self, &server->blocked_ptr);
if (try_cmpxchg_user(&server->umcg->state, RUNNABLE, RUNNING) > 0)
sys_umcg_wait() wake_up_process(server);
Either server sees the add, or we see it's RUNNABLE and wake it up
(or both).
If anything on the BLOCKED side goes wrong (bad pointers, whatever),
have it segfault.
<edit> Ooh, I forgot you can't just go wake the server when it's running
something else... so that does indeed need more states/complication, the
ordering argument stands though. We'll need something like
self->current_tid or somesuch </edit>
What are the alternatives? I just picked what the futex code uses.
u64 nanoseconds. Not sure tglx really wants to do that though, but
still, timespec is a terrible thing.
quoted
In summary, two IMPORTANT QUESTIONS:
1. thin vs fat syscalls: can we push some code/logic to the userspace
(state changes, looping/retries), or do we insist on syscalls handling
everything?
Well, the way I see it it's a trade of what is handled where. I get a
smaller API (although I'm sure I've forgotten something trivial again
that wrecks everything <edit> I did :/ </edit>) and userspace gets to
deal with all of SMP and scheduling policies.
You hard-coded a global-fifo and had a enormous number of syscalls and
needed to wrap every syscall invocation in order to fix up the return.
quoted
Please have in mind that even if we choose the second
approach (fat syscalls), the userspace will most likely still have to
do everything it does under the first option just to handle
signals/interrupts (i.e. unscheduled wakeups);
IIRC sigreturn goes back into the kernel and we can resume blocking
there.
quoted
2. kernel-side data vs userspace-only: can we avoid having kernel-side
data? More specifically, what alternatives to rcu_read_lock and/or
task_lock are available when working with userspace data?
What would you want locked and why?
Anyway, this email is far too long again (basically took me all day :/),
hope it helps a bit. Thomas is stuck fixing XSAVE disasters, but I'll
ask him to chime in once that's done.
Hi,
I wanted to way-in on this. I am one of the main developer's on the Cforall
programming language (https://cforall.uwaterloo.ca), which implements
its own
M:N user-threading runtime. I want to state that this RFC is an interesting
feature, which we would be able to take advantage of immediately, assuming
performance and flexibility closely match state-of-the-art implementations.
Precisely, we would benefit from two aspects of User Managed Control Groups:
1. user-level threads would become regular pthreads, so that gdb, valgrind,
ptrace and TLS works normally, etc.
2. The user-space scheduler can react on user-threads blocking in the
kernel.
However, we would need to look at performance issues like thread
creation and
context switch to know if your scheme is performant with user-level
threading.
We are also conscious about use cases that involve a very high (100Ks to
1Ms)
number of concurrent sessions and thus threads.
Note, our team published a comprehensive look at M:N threading in ACM
Sigmetrics 2020: https://doi.org/10.1145/3379483, which highlights the
expected performance of M:N threading, and another look at high-performance
control flow in SP&E 2021:
https://onlinelibrary.wiley.com/doi/10.1002/spe.2925
> > Yes, UNBLOCKED it a transitory state meaning the worker's blocking
> > operation has completed, but the wake event hasn't been delivered to
> > the userspace yet (and so the worker it not yet RUNNABLE)
>
> So if I understand the proposal correctly the only possible option is
> something like:
>
> for (;;) {
> next = user_sched_pick();
> if (next) {
> sys_umcg_run(next);
> continue;
> }
>
> sys_umcg_poll(&next);
> if (next) {
> next->state = RUNNABLE;
> user_sched_enqueue(next);
> }
> }
>
> This seems incapable of implementing generic scheduling policies and has
> a hard-coded FIFO policy.
>
> The poll() thing cannot differentiate between: 'find new task' and 'go
> idle'. So you cannot keep running it until all new tasks are found.
>
> But you basically get to do a syscall to discover every new task, while
> the other proposal gets you a user visible list of new tasks, no
> syscalls needed at all.
I agree strongly with this comment, sys_umcg_poll() does not appear to be
flexible enough for generic policies. I also suspect it would become a
bottleneck in any SMP scheduler due to this central serial data-structure.
> But you basically get to do a syscall to discover every new task, while
> the other proposal gets you a user visible list of new tasks, no
> syscalls needed at all.
>
> It's also not quite clear to me what you do about RUNNING->BLOCKED, how
> does the userspace scheduler know to dequeue a task?
In the schedulers we have implemented, threads are dequeued *before* being
run. That is, the head of the queue is not the currently running thread.
If the currently running threads need to be in the scheduler data-structure,
I believe it can be dequeued immediately after sys_umcg_run() has returned.
More on this below.
> My proposal gets you something like:
>
> [...]
>
> struct umcg_task {
> u32 umcg_status; /* r/w */
> u32 umcg_server_tid; /* r */
> u32 umcg_next_tid; /* r */
> u32 umcg_tid; /* r */
> u64 umcg_blocked_ptr; /* w */
> u64 umcg_runnable_ptr; /* w */
> };
I believe this approach may work, but could you elaborate on it? I
wasn't able
to find a more complete description.
For example, I fail to see what purpose the umcg_blocked_ptr serves.
When could
it contain anything other then a single element that is already pointed
to by "n" in the proposed loop? The only case I can come up with, is if a
worker thread tries to context switch directly to another worker thread.
But in
that case, I do not know what state that second worker would need to be
in for
this operation to be correct. Is the objective to allow the scheduler to be
invoked from worker threads?
Also, what is the purpose of umcg_status being writable by the user-space?
(I'm assuming status == state)? The code in sys_umcg_wait suggests it is for
managing potential out-of-order wakes and waits, but the kernel should
be able
to handle them already, the same way FUTEX_WAKE and FUTEX_WAIT are handled.
When would these state transition not be handled by the kernel?
I would also point out that creating worker threads as regular pthreads and
then converting them to worker threads sounds less then ideal. It would
probably be preferable directly appended new worker threads to the
umcg_runnable_ptr list without scheduling them in the kernel. It makes the
placement of the umcg_task trickier but maintains a stronger M:N model.
Finally, I would recommend adding a 64-bit user pointer to umcg_task that is
neither read nor written from the kernel. These kind of fields are always
useful for implementers.
Thank you for your time,
Thierry
From: Peter Oskolkov <hidden> Date: 2021-07-08 21:44:16
On Wed, Jul 7, 2021 at 10:45 AM Thierry Delisle [off-list ref] wrote:
Hi,
I wanted to way-in on this. I am one of the main developer's on the Cforall
programming language (https://cforall.uwaterloo.ca), which implements
its own
M:N user-threading runtime. I want to state that this RFC is an interesting
feature, which we would be able to take advantage of immediately, assuming
performance and flexibility closely match state-of-the-art implementations.