From: Alex Belits <hidden> Date: 2020-07-22 14:45:35
This is a new version of task isolation implementation. Previous version is at
https://lore.kernel.org/lkml/07c25c246c55012981ec0296eee23e68c719333a.camel@marvell.com/
Mostly this covers race conditions prevention on breaking isolation. Early after kernel entry,
task_isolation_enter() is called to update flags visible to other CPU cores and to perform
synchronization if necessary. Before this call only "safe" operations happen, as long as
CONFIG_TRACE_IRQFLAGS is not enabled.
This is also intended for future TLB handling -- the idea is to also isolate those CPU cores from
TLB flushes while they are running isolated task in userspace, and do one flush on exiting, before
any code is called that may touch anything updated.
The functionality and interface is unchanged, except for /sys/devices/system/cpu/isolation_running
containing the list of CPUs running isolated tasks. This should be useful for userspace helper
libraries.
From: Alex Belits <hidden> Date: 2020-07-22 14:47:53
In commit f01f17d3705b ("mm, vmstat: make quiet_vmstat lighter")
the quiet_vmstat() function became asynchronous, in the sense that
the vmstat work was still scheduled to run on the core when the
function returned. For task isolation, we need a synchronous
version of the function that guarantees that the vmstat worker
will not run on the core on return from the function. Add a
quiet_vmstat_sync() function with that semantic.
Signed-off-by: Chris Metcalf <redacted>
Signed-off-by: Alex Belits <redacted>
---
include/linux/vmstat.h | 2 ++
mm/vmstat.c | 9 +++++++++
2 files changed, 11 insertions(+)
return.
+ */
+void quiet_vmstat_sync(void)
+{
+ cancel_delayed_work_sync(this_cpu_ptr(&vmstat_work));
+ refresh_cpu_vm_stats(false);
+}
+
/*
* Shepherd worker thread that checks the
* differentials of processors that have their worker
--
2.26.2
From: Alex Belits <hidden> Date: 2020-07-22 14:49:28
From 7823be8cd3ba2e66308f334a2e47f60ba7829e0b Mon Sep 17 00:00:00 2001
From: Chris Metcalf <redacted>
Date: Sat, 1 Feb 2020 08:05:45 +0000
Subject: [PATCH 02/13] task_isolation: vmstat: add vmstat_idle function
This function checks to see if a vmstat worker is not running,
and the vmstat diffs don't require an update. The function is
called from the task-isolation code to see if we need to
actually do some work to quiet vmstat.
Signed-off-by: Chris Metcalf <redacted>
Signed-off-by: Alex Belits <redacted>
---
include/linux/vmstat.h | 2 ++
mm/vmstat.c | 10 ++++++++++
2 files changed, 12 insertions(+)
From: Alex Belits <hidden> Date: 2020-07-22 14:50:42
The existing nohz_full mode is designed as a "soft" isolation mode
that makes tradeoffs to minimize userspace interruptions while
still attempting to avoid overheads in the kernel entry/exit path,
to provide 100% kernel semantics, etc.
However, some applications require a "hard" commitment from the
kernel to avoid interruptions, in particular userspace device driver
style applications, such as high-speed networking code.
This change introduces a framework to allow applications
to elect to have the "hard" semantics as needed, specifying
prctl(PR_TASK_ISOLATION, PR_TASK_ISOLATION_ENABLE) to do so.
The kernel must be built with the new TASK_ISOLATION Kconfig flag
to enable this mode, and the kernel booted with an appropriate
"isolcpus=nohz,domain,CPULIST" boot argument to enable
nohz_full and isolcpus. The "task_isolation" state is then indicated
by setting a new task struct field, task_isolation_flag, to the
value passed by prctl(), and also setting a TIF_TASK_ISOLATION
bit in the thread_info flags. When the kernel is returning to
userspace from the prctl() call and sees TIF_TASK_ISOLATION set,
it calls the new task_isolation_start() routine to arrange for
the task to avoid being interrupted in the future.
With interrupts disabled, task_isolation_start() ensures that kernel
subsystems that might cause a future interrupt are quiesced. If it
doesn't succeed, it adjusts the syscall return value to indicate that
fact, and userspace can retry as desired. In addition to stopping
the scheduler tick, the code takes any actions that might avoid
a future interrupt to the core, such as a worker thread being
scheduled that could be quiesced now (e.g. the vmstat worker)
or a future IPI to the core to clean up some state that could be
cleaned up now (e.g. the mm lru per-cpu cache).
Once the task has returned to userspace after issuing the prctl(),
if it enters the kernel again via system call, page fault, or any
other exception or irq, the kernel will send it a signal to indicate
isolation loss. In addition to sending a signal, the code supports a
kernel command-line "task_isolation_debug" flag which causes a stack
backtrace to be generated whenever a task loses isolation.
To allow the state to be entered and exited, the syscall checking
test ignores the prctl(PR_TASK_ISOLATION) syscall so that we can
clear the bit again later, and ignores exit/exit_group to allow
exiting the task without a pointless signal being delivered.
The prctl() API allows for specifying a signal number to use instead
of the default SIGKILL, to allow for catching the notification
signal; for example, in a production environment, it might be
helpful to log information to the application logging mechanism
before exiting. Or, the signal handler might choose to reset the
program counter back to the code segment intended to be run isolated
via prctl() to continue execution.
In a number of cases we can tell on a remote cpu that we are
going to be interrupting the cpu, e.g. via an IPI or a TLB flush.
In that case we generate the diagnostic (and optional stack dump)
on the remote core to be able to deliver better diagnostics.
If the interrupt is not something caught by Linux (e.g. a
hypervisor interrupt) we can also request a reschedule IPI to
be sent to the remote core so it can be sure to generate a
signal to notify the process.
Isolation also disables CPU state synchronization mechanisms that
are. normally done by IPI. In the future, more synchronization
mechanisms, such as TLB flushes, may be disabled for isolated tasks.
This requires careful handling of kernel entry from isolated task --
remote synchronization requests must be re-enabled and
synchronization procedure triggered, before anything other than
low-level kernel entry code is called. Same applies to exiting from
kernel to userspace after isolation is enabled -- either the code
should not depend on synchronization, or isolation should be broken.
For this purpose, per-CPU low-level flags ll_isol_flags are used to
indicate isolation state, and task_isolation_kernel_enter() is used
to safely clear them early in kernel entry. CPU mask corresponding
to isolation bit in ll_isol_flags is visible to userspace as
/sys/devices/system/cpu/isolation_running, and can be used for
monitoring.
Separate patches that follow provide these changes for x86, arm,
and arm64 architectures, xen and irqchip drivers.
Signed-off-by: Alex Belits <redacted>
---
.../admin-guide/kernel-parameters.txt | 6 +
drivers/base/cpu.c | 23 +
include/linux/hrtimer.h | 4 +
include/linux/isolation.h | 295 ++++++
include/linux/sched.h | 5 +
include/linux/tick.h | 3 +
include/uapi/linux/prctl.h | 6 +
init/Kconfig | 28 +
kernel/Makefile | 2 +
kernel/isolation.c | 841 ++++++++++++++++++
kernel/signal.c | 2 +
kernel/sys.c | 6 +
kernel/time/hrtimer.c | 27 +
kernel/time/tick-sched.c | 18 +
14 files changed, 1266 insertions(+)
create mode 100644 include/linux/isolation.h
create mode 100644 kernel/isolation.c
@@ -5015,6 +5015,12 @@ neutralize any effect of /proc/sys/kernel/sysrq. Useful for debugging.+ task_isolation_debug [KNL]+ In kernels built with CONFIG_TASK_ISOLATION, this+ setting will generate console backtraces to+ accompany the diagnostics generated about+ interrupting tasks running with task isolation.+ tcpmhash_entries= [KNL,NET] Set the number of tcp_metrics_hash slots. Default value is 8192 or 16384 depending on total
@@ -620,6 +620,34 @@ config CPU_ISOLATIONsource"kernel/rcu/Kconfig"+configHAVE_ARCH_TASK_ISOLATION+bool++configTASK_ISOLATION+bool"Provide hard CPU isolation from the kernel on demand"+depends onNO_HZ_FULL&&HAVE_ARCH_TASK_ISOLATION+help++Allowuserspaceprocessesthatplacethemselvesoncoreswith+nohz_fullandisolcpusenabled,andrunprctl(PR_TASK_ISOLATION),+to"isolate"themselvesfromthekernel.Priortoreturningto+userspace,isolatedtaskswillarrangethatnofuturekernel+activitywillinterruptthetaskwhilethetaskisrunningin+userspace.Attemptingtore-enterthekernelwhileinthismode+willcausethetasktobeterminatedwithasignal;youmust+explicitlyuseprctl()todisabletaskisolationbeforeresuming+normaluseofthekernel.++This"hard"isolationfromthekernelisrequiredforuserspace+tasksthatarerunninghardreal-timetasksinuserspace,suchas+ahigh-speednetworkdriverinuserspace.Withoutthisoption,but+withNO_HZ_FULLenabled,thekernelwillmakeabest-faith,"soft"+efforttoshieldasingleuserspaceprocessfrominterrupts,but+makesnoguarantees.++Youshouldsay"N"unlessyouareintendingtoruna+high-performanceuserspacedriverorsimilartask.+configBUILD_BIN2Cbooldefaultn
@@ -0,0 +1,841 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*linux/kernel/isolation.c+*+*Implementationoftaskisolation.+*+*Authors:+*ChrisMetcalf<cmetcalf@mellanox.com>+*AlexBelits<abelits@marvell.com>+*YuriNorov<ynorov@marvell.com>+*/++#include<linux/mm.h>+#include<linux/swap.h>+#include<linux/vmstat.h>+#include<linux/sched.h>+#include<linux/isolation.h>+#include<linux/syscalls.h>+#include<linux/smp.h>+#include<linux/tick.h>+#include<asm/unistd.h>+#include<asm/syscall.h>+#include<linux/hrtimer.h>++/*+*Thesevaluesarestoredintask_isolation_state.+*NotethatSTATE_NORMAL+TIF_TASK_ISOLATIONmeanswearestill+*returningfromsys_prctl()touserspace.+*/+enum{+STATE_NORMAL=0,/* Not isolated */+STATE_ISOLATED=1/* In userspace, isolated */+};++/*+*CounterforisolationstateonagivenCPU,incrementswhenentering+*isolationanddecrementswhenexitingisolation(beforeorafterthe+*cleanup).Multiplesimultaneouslyrunningproceduresenteringor+*exitingisolationarepreventedbycheckingtheresultof+*incrementingordecrementingthisvariable.Thisvariableisboth+*incrementedanddecrementedbyCPUthatcausedisolationenteringor+*exit.+*+*Thisisnecessarybecausemultipleisolation-breakingeventsmayhappen+*atonce(oroneastheresultoftheother),howeverisolationexit+*mayonlyhappenoncetotransitionfromisolatedtonon-isolatedstate.+*Therefore,ifdecrementingthiscounterresultsinavaluelessthan0,+*isolationexitprocedurecan'tbestarted--italreadyhappened,oris+*inprogress,orisolationisnotenteredyet.+*/+DEFINE_PER_CPU(atomic_t,isol_counter);++/*+*Low-levelisolationflags.+*Thoseflagsareusedbylow-levelisolationset/clear/checkroutines.+*Thoseflagsshouldbesetlastbeforereturntouserspaceandcleared+*firstuponkernelentry,andsynchronizedtoallowisolationbreaking+*detectionbeforetouchingpotentiallyunsynchronizedpartsofkernel.+*Isolatedtaskdoesnotreceivesynchronizationeventsofanykind,so+*atthetimeofthefirstentryintokernelitmightnotbereadyto+*runmostofthekernelcode.Howevertoperformsynchronization+*properly,kernelentrycodeshouldalsoenablesynchronizationevents+*atthesametime.Thispresentsaproblembecausemorekernelcode+*shouldruntodeterminethecauseofisolationbreaking,signalsmay+*havetobegenerated,etc.Sosomeflagclearingandsynchronization+*shouldhappenin"low-level"entrycodebutprocessingofisolation+*breakingshouldhappenin"high-level"code.Low-levelisolationflags+*shouldbesetinthatlow-levelcode,possiblylongbeforethecause+*ofisolationbreakingisknown.Symmetrically,enteringisolation+*shoulddisablesynchronizationeventsbeforereturningtouserspace+*butafterallpotentiallyvolatilecodeisfinished.+*/+DEFINE_PER_CPU(unsignedlong,ll_isol_flags);++/*+*DescriptionofthelasttwotasksthatranisolatedonagivenCPU.+*Thisisintendedonlyformessagesaboutisolationbreaking.We+*don'twantanyreferencestoactualtaskwhileaccessingthisfrom+*CPUthatcausedisolationbreaking--weknownothingabouttiming+*anddon'twanttouselockingorRCU.+*/+structisol_task_desc{+atomic_tcurr_index;+atomic_tcurr_index_wr;+boolwarned[2];+pid_tpid[2];+pid_ttgid[2];+charcomm[2][TASK_COMM_LEN];+};+staticDEFINE_PER_CPU(structisol_task_desc,isol_task_descs);++/*+*Counterforisolationexitingprocedures(fromrequesttothestartof+*cleanup)beingattemptedatonceonaCPU.Normallyincrementingof+*thiscounterisperformedfromtheCPUthatcausedisolationbreaking,+*howeverdecrementingisdonefromthecleanupprocedure,delegatedto+*theCPUthatisexitingisolation,notfromtheCPUthatcausedisolation+*breaking.+*+*Ifincrementingthiscounterwhilestartingisolationexitprocedure+*resultsinavaluegreaterthan0,isolationexitingisalreadyin+*progress,andcleanupdidnotstartyet.Thismeans,countershouldbe+*decrementedback,andisolationexitthatisalreadyinprogress,should+*beallowedtocomplete.Otherwise,anewisolationexitprocedureshould+*bestarted.+*/+DEFINE_PER_CPU(atomic_t,isol_exit_counter);++/*+*Descriptorforisolation-breakingSMPcalls+*/+DEFINE_PER_CPU(call_single_data_t,isol_break_csd);++cpumask_var_ttask_isolation_map;+cpumask_var_ttask_isolation_cleanup_map;+staticDEFINE_SPINLOCK(task_isolation_cleanup_lock);++/* We can run on cpus that are isolated from the scheduler and are nohz_full. */+staticint__inittask_isolation_init(void)+{+alloc_bootmem_cpumask_var(&task_isolation_cleanup_map);+if(alloc_cpumask_var(&task_isolation_map,GFP_KERNEL))+/*+*Atthispointtaskisolationshouldmatch+*nohz_full.Thismaychangeinthefuture.+*/+cpumask_copy(task_isolation_map,tick_nohz_full_mask);+return0;+}+core_initcall(task_isolation_init)++/* Enable stack backtraces of any interrupts of task_isolation cores. */+staticbooltask_isolation_debug;+staticint__inittask_isolation_debug_func(char*str)+{+task_isolation_debug=true;+return1;+}+__setup("task_isolation_debug",task_isolation_debug_func);++/*+*Recordname,pidandgrouppidofthetaskenteringisolationon+*thecurrentCPU.+*/+staticvoidrecord_curr_isolated_task(void)+{+intind;+intcpu=smp_processor_id();+structisol_task_desc*desc=&per_cpu(isol_task_descs,cpu);+structtask_struct*task=current;++/* Finish everything before recording current task */+smp_mb();+ind=atomic_inc_return(&desc->curr_index_wr)&1;+desc->comm[ind][sizeof(task->comm)-1]='\0';+memcpy(desc->comm[ind],task->comm,sizeof(task->comm)-1);+desc->pid[ind]=task->pid;+desc->tgid[ind]=task->tgid;+desc->warned[ind]=false;+/* Write everything, to be seen by other CPUs */+smp_mb();+atomic_inc(&desc->curr_index);+/* Everyone will see the new record from this point */+smp_mb();+}++/*+*Printmessageprefixedwiththedescriptionofthecurrent(or+*last)isolatedtaskonagivenCPU.Intendedforisolationbreaking+*messagesthatincludetargettaskfortheuser'sconvenience.+*+*Messagesproducedwiththisfunctionmayhaveobsoletetask+*informationifisolatedtasksmanagedtoexit,startandenter+*isolationmultipletimes,ormultipletaskstriedtoenter+*isolationonthesameCPUatonce.Forthoseunusualcasesitwould+*containavaliddescriptionofthecauseforisolationbreakingand+*targetCPUnumber,justnotthecorrectdescriptionofwhichtask+*endeduplosingisolation.+*/+inttask_isolation_message(intcpu,intlevel,boolsupp,constchar*fmt,...)+{+structisol_task_desc*desc;+structtask_struct*task;+va_listargs;+charbuf_prefix[TASK_COMM_LEN+20+3*20];+charbuf[200];+intcurr_cpu,ind_counter,ind_counter_old,ind;++curr_cpu=get_cpu();+desc=&per_cpu(isol_task_descs,cpu);+ind_counter=atomic_read(&desc->curr_index);++if(curr_cpu==cpu){+/*+*MessageisforthecurrentCPUsocurrent+*task_structshouldbeusedinsteadofcached+*information.+*+*Likeinotherdiagnosticmessages,ifissuedfrom+*interruptcontext,currentwillbetheinterrupted+*task.Unlikeotherdiagnosticmessages,thisis+*alwaysrelevantbecausethemessageisabout+*interruptingatask.+*/+ind=ind_counter&1;+if(supp&&desc->warned[ind]){+/*+*Ifsuppistrue,skipthemessageifthe+*sametaskwasmentionedinthemessage+*originatedonremoteCPU,anditdidnot+*re-enterisolatedstatesincethen(warned+*istrue).Onlylocalmessagesfollowing+*remotemessages,likelyaboutthesame+*isolationbreakingevent,areskippedto+*avoidduplication.Ifremotecauseis+*immediatelyfollowedbyalocalonebefore+*isolationisbroken,localcauseisskipped+*frommessages.+*/+put_cpu();+return0;+}+task=current;+snprintf(buf_prefix,sizeof(buf_prefix),+"isolation %s/%d/%d (cpu %d)",+task->comm,task->tgid,task->pid,cpu);+put_cpu();+}else{+/*+*MessageisforremoteCPU,usecachedinformation.+*/+put_cpu();+/*+*Makesure,indexremainedunchangedwhiledatawas+*copied.Ifitchanged,datathatwascopiedmaybe+*inconsistentbecausetwoupdatesinasequencecould+*overwritethedatawhileitwasbeingread.+*/+do{+/* Make sure we are reading up to date values */+smp_mb();+ind=ind_counter&1;+snprintf(buf_prefix,sizeof(buf_prefix),+"isolation %s/%d/%d (cpu %d)",+desc->comm[ind],desc->tgid[ind],+desc->pid[ind],cpu);+desc->warned[ind]=true;+ind_counter_old=ind_counter;+/* Record the warned flag, then re-read descriptor */+smp_mb();+ind_counter=atomic_read(&desc->curr_index);+/*+*Ifthecounterchanged,somethingwasupdated,so+*repeateverythingtogetthecurrentdata+*/+}while(ind_counter!=ind_counter_old);+}++va_start(args,fmt);+vsnprintf(buf,sizeof(buf),fmt,args);+va_end(args);++switch(level){+caseLOGLEVEL_EMERG:+pr_emerg("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_ALERT:+pr_alert("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_CRIT:+pr_crit("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_ERR:+pr_err("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_WARNING:+pr_warn("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_NOTICE:+pr_notice("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_INFO:+pr_info("%s: %s",buf_prefix,buf);+break;+caseLOGLEVEL_DEBUG:+pr_debug("%s: %s",buf_prefix,buf);+break;+default:+/* No message without a valid level */+return0;+}+return1;+}++/*+*Dumpstackifneedbe.Thiscanbehelpfulevenfromthefinalexit+*tousermodecodesincestacktracessometimescarryinformationabout+*whatputyouintothekernel,e.g.aninterruptnumberencodedin+*theinitialentrystackframethatisstillvisibleatexittime.+*/+staticvoiddebug_dump_stack(void)+{+if(task_isolation_debug)+dump_stack();+}++/*+*Settheflagswordbutdon'ttrytoactuallystarttaskisolationyet.+*Wewillstartitwhenenteringuserspaceintask_isolation_start().+*/+inttask_isolation_request(unsignedintflags)+{+structtask_struct*task=current;++/*+*Thetaskisolationflagsshouldalwaysbeclearedjustby+*virtueofhavingenteredthekernel.+*/+WARN_ON_ONCE(test_tsk_thread_flag(task,TIF_TASK_ISOLATION));+WARN_ON_ONCE(task->task_isolation_flags!=0);+WARN_ON_ONCE(task->task_isolation_state!=STATE_NORMAL);++task->task_isolation_flags=flags;+if(!(task->task_isolation_flags&PR_TASK_ISOLATION_ENABLE))+return0;++/* We are trying to enable task isolation. */+set_tsk_thread_flag(task,TIF_TASK_ISOLATION);++/*+*Shutdownthevmstatworkersowe'renotinterruptedlater.+*Wehavetotrytodothishere(withinterruptsenabled)since+*wearecancelingdelayedworkandwillcallflush_work()+*(whichenablesinterrupts)andpossiblyschedule().+*/+quiet_vmstat_sync();++/* We return 0 here but we may change that in task_isolation_start(). */+return0;+}++/*+*Performactionsthatshouldbedoneimmediatelyonexitfromisolation.+*/+staticvoidfast_task_isolation_cpu_cleanup(void*info)+{+unsignedlongflags;++/*+*ThisfunctionrunsonaCPUthatranisolatedtask.+*+*Wedon'twantthisCPUrunningcodefromtherestofkernel+*untilotherCPUsknowthatitisnolongerisolated.+*WhenCPUisrunningisolatedtaskuntilthispointanything+*thatcausesaninterruptonthisCPUmustendupcallingthis+*ortask_isolation_kernel_enter()beforetouchingtherestof+*kernel.Thatis,task_isolation_kernel_enter(),IPItothis+*functionorstop_isolation()callingit.Ifanyinterrupt,+*includingschedulingtimer,arrivesbeforeacalltothis+*function,itwillstillendupintask_isolation_kernel_enter()+*earlyafterenteringkernel.+*FromthispointinterruptsaredisableduntilallCPUswillsee+*thatthisCPUisnolongerrunningisolatedtask.+*+*Seealsotask_isolation_kernel_enter().+*/+local_irq_save(flags);+atomic_dec(&per_cpu(isol_exit_counter,smp_processor_id()));+smp_mb__after_atomic();+/*+*AtthispointbreakingisolationfromotherCPUsispossibleagain,+*howeverinterruptswon'tarriveuntillocal_irq_restore()+*/++/*+*Thistaskisnolongerisolated(andifbyanychancethis+*isthewrongtask,it'salreadynotisolated)+*/+current->task_isolation_flags=0;+clear_tsk_thread_flag(current,TIF_TASK_ISOLATION);++/* Run the rest of cleanup later */+set_tsk_thread_flag(current,TIF_NOTIFY_RESUME);++/* Clear low-level flags if they are not cleared yet */+this_cpu_write(ll_isol_flags,0);++/*+*Ifsomethinghappenedthatrequiresabarrierthatwould+*otherwisebecalledfromremoteCPUsbyCPUkickprocedure,+*thisbarrierrunsinsteadofit.Afterthisbarrier,CPU+*kickprocedurewouldseetheupdatedll_isol_flags,soit+*willrunitsownIPItotriggerabarrier.+*/+smp_mb();+/*+*Synchronizeinstructions--thisCPUwasnotkickedwhile+*inisolatedmode,soitmightrequiresynchronization.+*TheremightbeanIPIifkickprocedurehappenedand+*ll_isol_flagswasalreadyupdatedwhileitassembledaCPU+*mask.Howeverifthisdidnothappen,synchronizeeverything+*here.+*/+instr_sync();+local_irq_restore(flags);+}++/* Disable task isolation for the specified task. */+staticvoidstop_isolation(structtask_struct*p)+{+intcpu,this_cpu;+unsignedlongflags;++this_cpu=get_cpu();+cpu=task_cpu(p);+if(atomic_inc_return(&per_cpu(isol_exit_counter,cpu))>1){+/* Already exiting isolation */+atomic_dec(&per_cpu(isol_exit_counter,cpu));+put_cpu();+return;+}++if(p==current){+p->task_isolation_state=STATE_NORMAL;+fast_task_isolation_cpu_cleanup(NULL);+task_isolation_cpu_cleanup();+if(atomic_dec_return(&per_cpu(isol_counter,cpu))<0){+/* Is not isolated already */+atomic_inc(&per_cpu(isol_counter,cpu));+}+put_cpu();+}else{+if(atomic_dec_return(&per_cpu(isol_counter,cpu))<0){+/* Is not isolated already */+atomic_inc(&per_cpu(isol_counter,cpu));+atomic_dec(&per_cpu(isol_exit_counter,cpu));+put_cpu();+return;+}+/*+*Schedule"slow"cleanup.Thisrelieson+*TIF_NOTIFY_RESUMEbeingset+*/+spin_lock_irqsave(&task_isolation_cleanup_lock,flags);+cpumask_set_cpu(cpu,task_isolation_cleanup_map);+spin_unlock_irqrestore(&task_isolation_cleanup_lock,flags);+/*+*SettingflagsisdelegatedtotheCPUwhere+*isolatedtaskisrunning+*isol_exit_counterwillbedecrementedfromthereaswell.+*/+per_cpu(isol_break_csd,cpu).func=+fast_task_isolation_cpu_cleanup;+per_cpu(isol_break_csd,cpu).info=NULL;+per_cpu(isol_break_csd,cpu).flags=0;+smp_call_function_single_async(cpu,+&per_cpu(isol_break_csd,cpu));+put_cpu();+}+}++/*+*Thiscoderunswithinterruptsdisabledjustbeforethereturnto+*userspace,afteraprctl()hasrequestedenablingtaskisolation.+*Wetakewhateverstepsareneededtoavoidbeinginterruptedlater:+*drainthelrupages,stoptheschedulertick,etc.More+*functionalitymaybeaddedherelatertoavoidothertypesof+*interruptsfromotherkernelsubsystems.+*+*Ifwecan'tenabletaskisolation,weupdatethesyscallreturn+*valuewithanappropriateerror.+*/+voidtask_isolation_start(void)+{+interror;+unsignedlongflags;++/*+*WeshouldonlybecalledinSTATE_NORMAL(isolationdisabled),+*onourwayoutofthekernelfromtheprctl()thatturnediton.+*Ifweareexitingfromthekernelinanotherstate,itmeanswe+*madeitbackintothekernelwithoutdisablingtaskisolation,+*andweshouldinvestigatehow(andinanycasedisabletask+*isolationatthispoint).Weareclearlynotonthepathback+*fromtheprctl()sowedon'ttouchthesyscallreturnvalue.+*/+if(WARN_ON_ONCE(current->task_isolation_state!=STATE_NORMAL)){+/* Increment counter, this will allow isolation breaking */+if(atomic_inc_return(&per_cpu(isol_counter,+smp_processor_id()))>1){+atomic_dec(&per_cpu(isol_counter,smp_processor_id()));+}+//atomic_inc(&per_cpu(isol_counter, smp_processor_id()));+stop_isolation(current);+return;+}++/*+*Mustbeaffinitizedtoasinglecorewithtaskisolationpossible.+*Inprinciplethiscouldberemotelymodifiedbetweentheprctl()+*andthereturntouserspace,sowehavetocheckithere.+*/+if(current->nr_cpus_allowed!=1||+!is_isolation_cpu(smp_processor_id())){+error=-EINVAL;+gotoerror;+}++/* If the vmstat delayed work is not canceled, we have to try again. */+if(!vmstat_idle()){+error=-EAGAIN;+gotoerror;+}++/* Try to stop the dynamic tick. */+error=try_stop_full_tick();+if(error)+gotoerror;++/* Drain the pagevecs to avoid unnecessary IPI flushes later. */+lru_add_drain();++/*+*Taskisgoingtobemarkedasisolated.ThisdisablesIPIs+*usedforsynchronization,sotoavoidinconsistency+*don'tletanythinginterruptusandissueabarrierattheend.+*/+local_irq_save(flags);++/* Increment counter, this will allow isolation breaking */+if(atomic_inc_return(&per_cpu(isol_counter,+smp_processor_id()))>1){+atomic_dec(&per_cpu(isol_counter,smp_processor_id()));+}++/* Record isolated task IDs and name */+record_curr_isolated_task();+smp_wmb();++/* From this point this is recognized as isolated by other CPUs */+current->task_isolation_state=STATE_ISOLATED;+this_cpu_write(ll_isol_flags,FLAG_LL_TASK_ISOLATION);+smp_mb();+local_irq_restore(flags);+/*+*Ifanythinginterruptsusatthispoint,itwilltrigger+*isolationbreakingprocedure.+*/+return;++error:+/* Increment counter, this will allow isolation breaking */+if(atomic_inc_return(&per_cpu(isol_counter,+smp_processor_id()))>1){+atomic_dec(&per_cpu(isol_counter,smp_processor_id()));+}+stop_isolation(current);+syscall_set_return_value(current,current_pt_regs(),error,0);+}++/* Stop task isolation on the remote task and send it a signal. */+staticvoidsend_isolation_signal(structtask_struct*task)+{+intflags=task->task_isolation_flags;+kernel_siginfo_tinfo={+.si_signo=PR_TASK_ISOLATION_GET_SIG(flags)?:SIGKILL,+};++stop_isolation(task);+send_sig_info(info.si_signo,&info,task);+}++/* Only a few syscalls are valid once we are in task isolation mode. */+staticboolis_acceptable_syscall(intsyscall)+{+/* No need to incur an isolation signal if we are just exiting. */+if(syscall==__NR_exit||syscall==__NR_exit_group)+returntrue;++/* Check to see if it's the prctl for isolation. */+if(syscall==__NR_prctl){+unsignedlongarg[SYSCALL_MAX_ARGS];++syscall_get_arguments(current,current_pt_regs(),arg);+if(arg[0]==PR_TASK_ISOLATION)+returntrue;+}++returnfalse;+}++/*+*Thisroutineiscalledfromsyscallentry,preventsmostsyscalls+*fromexecuting,andifneededraisesasignaltonotifytheprocess.+*+*Notethatwehavetostopisolationbeforeweevenprintamessage+*here,sinceotherwisewemightendupreportinganinterruptdueto+*kickingtheprintkhandlingcode,ratherthanreportingthetrue+*causeofinterrupthere.+*+*Themessageisnotsuppressedbypreviousremotelytriggered+*messages.+*/+inttask_isolation_syscall(intsyscall)+{+structtask_struct*task=current;++if(is_acceptable_syscall(syscall)){+stop_isolation(task);+return0;+}++send_isolation_signal(task);++pr_task_isol_warn(smp_processor_id(),+"task_isolation lost due to syscall %d\n",+syscall);+debug_dump_stack();++syscall_set_return_value(task,current_pt_regs(),-ERESTARTNOINTR,-1);+return-1;+}++/*+*Thisroutineiscalledfromanyexceptionorirqthatdoesn't+*otherwisetriggerasignaltotheuserprocess(e.g.pagefault).+*+*Messageswillbesuppressedifthereisalreadyareportedremote+*causeforisolationbreaking,sowedon'tgeneratemultiple+*confusinglysimilarmessagesaboutthesameevent.+*/+void_task_isolation_interrupt(constchar*fmt,...)+{+structtask_struct*task=current;+va_listargs;+charbuf[100];++/* RCU should have been enabled prior to this point. */+RCU_LOCKDEP_WARN(!rcu_is_watching(),"kernel entry without RCU");++/* Are we exiting isolation already? */+if(atomic_read(&per_cpu(isol_exit_counter,smp_processor_id()))!=0){+task->task_isolation_state=STATE_NORMAL;+return;+}+/*+*Avoidreportinginterruptsthathappenafterwehaveprctl'ed+*toenableisolation,butbeforewehavereturnedtouserspace.+*/+if(task->task_isolation_state==STATE_NORMAL)+return;++va_start(args,fmt);+vsnprintf(buf,sizeof(buf),fmt,args);+va_end(args);++/* Handle NMIs minimally, since we can't send a signal. */+if(in_nmi()){+task_isolation_kernel_enter();+pr_task_isol_err(smp_processor_id(),+"isolation: in NMI; not delivering signal\n");+}else{+send_isolation_signal(task);+}++if(pr_task_isol_warn_supp(smp_processor_id(),+"task_isolation lost due to %s\n",buf))+debug_dump_stack();+}++/*+*Calledbeforewewakeupataskthathasasignaltoprocess.+*Needstobedonetohandleinterruptsthattriggersignals,which+*wedon'tcatchwithtask_isolation_interrupt()hooks.+*+*Thismessageisalsosuppressediftherewasalreadyaremotely+*causedmessageaboutthesameisolationbreakingevent.+*/+void_task_isolation_signal(structtask_struct*task)+{+structisol_task_desc*desc;+intind,cpu;+booldo_warn=(task->task_isolation_state==STATE_ISOLATED);++cpu=task_cpu(task);+desc=&per_cpu(isol_task_descs,cpu);+ind=atomic_read(&desc->curr_index)&1;+if(desc->warned[ind])+do_warn=false;++stop_isolation(task);++if(do_warn){+pr_warn("isolation: %s/%d/%d (cpu %d): task_isolation lost due to signal\n",+task->comm,task->tgid,task->pid,cpu);+debug_dump_stack();+}+}++/*+*Generateastackbacktraceifwearegoingtointerruptanothertask+*isolationprocess.+*/+voidtask_isolation_remote(intcpu,constchar*fmt,...)+{+structtask_struct*curr_task;+va_listargs;+charbuf[200];++smp_rmb();+if(!is_isolation_cpu(cpu)||!task_isolation_on_cpu(cpu))+return;++curr_task=current;++va_start(args,fmt);+vsnprintf(buf,sizeof(buf),fmt,args);+va_end(args);+if(pr_task_isol_warn(cpu,+"task_isolation lost due to %s by %s/%d/%d on cpu %d\n",+buf,+curr_task->comm,curr_task->tgid,+curr_task->pid,smp_processor_id()))+debug_dump_stack();+}++/*+*Generateastackbacktraceifanyofthecpusin"mask"arerunning+*taskisolationprocesses.+*/+voidtask_isolation_remote_cpumask(conststructcpumask*mask,+constchar*fmt,...)+{+structtask_struct*curr_task;+cpumask_var_twarn_mask;+va_listargs;+charbuf[200];+intcpu,first_cpu;++if(task_isolation_map==NULL||+!zalloc_cpumask_var(&warn_mask,GFP_KERNEL))+return;++first_cpu=-1;+smp_rmb();+for_each_cpu_and(cpu,mask,task_isolation_map){+if(task_isolation_on_cpu(cpu)){+if(first_cpu<0)+first_cpu=cpu;+else+cpumask_set_cpu(cpu,warn_mask);+}+}++if(first_cpu<0)+gotodone;++curr_task=current;++va_start(args,fmt);+vsnprintf(buf,sizeof(buf),fmt,args);+va_end(args);++if(cpumask_weight(warn_mask)==0)+pr_task_isol_warn(first_cpu,+"task_isolation lost due to %s by %s/%d/%d on cpu %d\n",+buf,curr_task->comm,curr_task->tgid,+curr_task->pid,smp_processor_id());+else+pr_task_isol_warn(first_cpu,+" and cpus %*pbl: task_isolation lost due to %s by %s/%d/%d on cpu %d\n",+cpumask_pr_args(warn_mask),+buf,curr_task->comm,curr_task->tgid,+curr_task->pid,smp_processor_id());+debug_dump_stack();++done:+free_cpumask_var(warn_mask);+}++/*+*SetCPUscurrentlyrunningisolatedtasksinCPUmask.+*/+voidtask_isolation_cpumask(structcpumask*mask)+{+intcpu;++if(task_isolation_map==NULL)+return;++smp_rmb();+for_each_cpu(cpu,task_isolation_map)+if(task_isolation_on_cpu(cpu))+cpumask_set_cpu(cpu,mask);+}++/*+*ClearCPUscurrentlyrunningisolatedtasksinCPUmask.+*/+voidtask_isolation_clear_cpumask(structcpumask*mask)+{+intcpu;++if(task_isolation_map==NULL)+return;++smp_rmb();+for_each_cpu(cpu,task_isolation_map)+if(task_isolation_on_cpu(cpu))+cpumask_clear_cpu(cpu,mask);+}++/*+*Cleanupprocedure.Thecalltothisproceduremaybedelayed.+*/+voidtask_isolation_cpu_cleanup(void)+{+kick_hrtimer();+}++/*+*CheckifcleanupisscheduledonthecurrentCPU,andifso,runit.+*Intendedtobecalledfromnotify_resume()oranothersuchcallback+*onthetargetCPU.+*/+voidtask_isolation_check_run_cleanup(void)+{+intcpu;+unsignedlongflags;++spin_lock_irqsave(&task_isolation_cleanup_lock,flags);++cpu=smp_processor_id();++if(cpumask_test_cpu(cpu,task_isolation_cleanup_map)){+cpumask_clear_cpu(cpu,task_isolation_cleanup_map);+spin_unlock_irqrestore(&task_isolation_cleanup_lock,flags);+task_isolation_cpu_cleanup();+}else+spin_unlock_irqrestore(&task_isolation_cleanup_lock,flags);+}
@@ -888,6 +888,24 @@ static void tick_nohz_full_update_tick(struct tick_sched *ts)#endif}+#ifdef CONFIG_TASK_ISOLATION+inttry_stop_full_tick(void)+{+intcpu=smp_processor_id();+structtick_sched*ts=this_cpu_ptr(&tick_cpu_sched);++/* For an unstable clock, we should return a permanent error code. */+if(atomic_read(&tick_dep_mask)&TICK_DEP_MASK_CLOCK_UNSTABLE)+return-EINVAL;++if(!can_stop_full_tick(cpu,ts))+return-EAGAIN;++tick_nohz_stop_sched_tick(ts,cpu);+return0;+}+#endif+staticboolcan_stop_idle_tick(intcpu,structtick_sched*ts){/*
From: Alex Belits <hidden> Date: 2020-07-22 14:52:33
This commit adds task isolation hooks as follows:
- __handle_domain_irq() and handle_domain_nmi() generate an
isolation warning for the local task
- irq_work_queue_on() generates an isolation warning for the remote
task being interrupted for irq_work (through
__smp_call_single_queue())
- generic_exec_single() generates a remote isolation warning for
the remote cpu being IPI'd (through __smp_call_single_queue())
- smp_call_function_many() generates a remote isolation warning for
the set of remote cpus being IPI'd (through
smp_call_function_many_cond())
- on_each_cpu_cond_mask() generates a remote isolation warning for
the set of remote cpus being IPI'd (through
smp_call_function_many_cond())
- __ttwu_queue_wakelist() generates a remote isolation warning for
the remote cpu being IPI'd (through __smp_call_single_queue())
- nmi_enter(), __context_tracking_exit(), __handle_domain_irq(),
handle_domain_nmi() and scheduler_ipi() clear low-level flags and
synchronize CPUs by calling task_isolation_kernel_enter()
Calls to task_isolation_remote() or task_isolation_interrupt() can
be placed in the platform-independent code like this when doing so
results in fewer lines of code changes, as for example is true of
the users of the arch_send_call_function_*() APIs. Or, they can be
placed in the per-architecture code when there are many callers,
as for example is true of the smp_send_reschedule() call.
A further cleanup might be to create an intermediate layer, so that
for example smp_send_reschedule() is a single generic function that
just calls arch_smp_send_reschedule(), allowing generic code to be
called every time smp_send_reschedule() is invoked. But for now, we
just update either callers or callees as makes most sense.
Calls to task_isolation_kernel_enter() are intended for early
kernel entry code. They may be called in platform-independent or
platform-specific code.
It may be possible to clean up low-level entry code and somehow
organize calls to task_isolation_kernel_enter() to avoid multiple
per-architecture or driver-specific calls to it. RCU initialization
may be a good reference point for those places in kernel
(task_isolation_kernel_enter() should precede it), however right now
it is not unified between architectures.
Signed-off-by: Chris Metcalf <redacted>
[abelits@marvell.com: adapted for kernel 5.8, added low-level flags handling]
Signed-off-by: Alex Belits <redacted>
---
include/linux/hardirq.h | 2 ++
include/linux/sched.h | 2 ++
kernel/context_tracking.c | 4 ++++
kernel/irq/irqdesc.c | 13 +++++++++++++
kernel/smp.c | 6 +++++-
5 files changed, 26 insertions(+), 1 deletion(-)
@@ -545,6 +548,7 @@ static void smp_call_function_many_cond(const struct cpumask *mask,}/* Send a message to all CPUs in the map */+task_isolation_remote_cpumask(cfd->cpumask_ipi,"IPI function");arch_send_call_function_ipi_mask(cfd->cpumask_ipi);if(wait){
From: Alex Belits <hidden> Date: 2020-07-22 14:52:38
xen_evtchn_do_upcall() should call task_isolation_kernel_enter()
to indicate that isolation is broken and perform synchronization.
Signed-off-by: Alex Belits <redacted>
---
drivers/xen/events/events_base.c | 3 +++
1 file changed, 3 insertions(+)
From: Alex Belits <hidden> Date: 2020-07-22 14:55:01
In prepare_exit_to_usermode(), run cleanup for tasks exited fromi
isolation and call task_isolation_start() for tasks that entered
TIF_TASK_ISOLATION.
In syscall_trace_enter(), add the necessary support for reporting
syscalls for task-isolation processes.
Add task_isolation_remote() calls for the kernel exception types
that do not result in signals, namely non-signalling page faults.
Add task_isolation_kernel_enter() calls to interrupt and syscall
entry handlers.
This mechanism relies on calls to functions that call
task_isolation_kernel_enter() early after entry into kernel. Those
functions are:
enter_from_user_mode()
called from do_syscall_64(), do_int80_syscall_32(),
do_fast_syscall_32(), idtentry_enter_user(),
idtentry_enter_cond_rcu()
idtentry_enter_cond_rcu()
called from non-raw IDT macros and other entry points
idtentry_enter_user()
nmi_enter()
xen_call_function_interrupt()
xen_call_function_single_interrupt()
xen_irq_work_interrupt()
Signed-off-by: Chris Metcalf <redacted>
[abelits@marvell.com: adapted for kernel 5.8]
Signed-off-by: Alex Belits <redacted>
---
arch/x86/Kconfig | 1 +
arch/x86/entry/common.c | 20 +++++++++++++++++++-
arch/x86/include/asm/barrier.h | 2 ++
arch/x86/include/asm/thread_info.h | 4 +++-
arch/x86/kernel/apic/ipi.c | 2 ++
arch/x86/mm/fault.c | 4 ++++
arch/x86/xen/smp.c | 3 +++
arch/x86/xen/smp_pv.c | 2 ++
8 files changed, 36 insertions(+), 2 deletions(-)
@@ -136,7 +138,7 @@ struct thread_info {/* Work to do before invoking the actual syscall. */#define _TIF_WORK_SYSCALL_ENTRY \(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_EMU|_TIF_SYSCALL_AUDIT|\-_TIF_SECCOMP|_TIF_SYSCALL_TRACEPOINT)+_TIF_SECCOMP|_TIF_SYSCALL_TRACEPOINT|_TIF_TASK_ISOLATION)/* flags to check in __switch_to() */#define _TIF_WORK_CTXSW_BASE \
@@ -1332,6 +1333,9 @@ void do_user_addr_fault(struct pt_regs *regs,perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN,1,regs,address);}+/* No signal was generated, but notify task-isolation tasks. */+task_isolation_interrupt("page fault at %#lx",address);+check_v8086_mode(regs,address,tsk);}NOKPROBE_SYMBOL(do_user_addr_fault);
From: Alex Belits <hidden> Date: 2020-07-22 14:56:08
From: Chris Metcalf <redacted>
In do_notify_resume(), call task_isolation_start() for
TIF_TASK_ISOLATION tasks. Add _TIF_TASK_ISOLATION to _TIF_WORK_MASK,
and define a local NOTIFY_RESUME_LOOP_FLAGS to check in the loop,
since we don't clear _TIF_TASK_ISOLATION in the loop.
We instrument the smp_send_reschedule() routine so that it checks for
isolated tasks and generates a suitable warning if needed.
Finally, report on page faults in task-isolation processes in
do_page_faults().
Early kernel entry code calls task_isolation_kernel_enter(). In
particular:
Vectors:
el1_sync -> el1_sync_handler() -> task_isolation_kernel_enter()
el1_irq -> asm_nmi_enter(), handle_arch_irq()
el1_error -> do_serror()
el0_sync -> el0_sync_handler()
el0_irq -> handle_arch_irq()
el0_error -> do_serror()
el0_sync_compat -> el0_sync_compat_handler()
el0_irq_compat -> handle_arch_irq()
el0_error_compat -> do_serror()
SDEI entry:
__sdei_asm_handler -> __sdei_handler() -> nmi_enter()
Functions called from there:
asm_nmi_enter() -> nmi_enter() -> task_isolation_kernel_enter()
asm_nmi_exit() -> nmi_exit() -> task_isolation_kernel_return()
Handlers:
do_serror() -> nmi_enter() -> task_isolation_kernel_enter()
or task_isolation_kernel_enter()
el1_sync_handler() -> task_isolation_kernel_enter()
el0_sync_handler() -> task_isolation_kernel_enter()
el0_sync_compat_handler() -> task_isolation_kernel_enter()
handle_arch_irq() is irqchip-specific, most call handle_domain_irq()
or handle_IPI()
There is a separate patch for irqchips that do not follow this rule.
handle_domain_irq() -> task_isolation_kernel_enter()
handle_IPI() -> task_isolation_kernel_enter()
nmi_enter() -> task_isolation_kernel_enter()
Signed-off-by: Chris Metcalf <redacted>
[abelits@marvell.com: simplified to match kernel 5.6]
Signed-off-by: Alex Belits <redacted>
---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/barrier.h | 2 ++
arch/arm64/include/asm/thread_info.h | 5 ++++-
arch/arm64/kernel/entry-common.c | 7 +++++++
arch/arm64/kernel/ptrace.c | 16 +++++++++++++++-
arch/arm64/kernel/sdei.c | 2 ++
arch/arm64/kernel/signal.c | 13 ++++++++++++-
arch/arm64/kernel/smp.c | 9 +++++++++
arch/arm64/mm/fault.c | 5 +++++
9 files changed, 57 insertions(+), 3 deletions(-)
@@ -1859,6 +1864,15 @@ int syscall_trace_enter(struct pt_regs *regs)returnNO_SYSCALL;}+/*+*Intaskisolationmode,wemaypreventthesyscallfrom+*running,andifsowealsodeliverasignaltotheprocess.+*/+if(test_thread_flag(TIF_TASK_ISOLATION)){+if(task_isolation_syscall(regs->syscallno)==-1)+returnNO_SYSCALL;+}+/* Do the secure computing after ptrace; failures should be fast. */if(secure_computing()==-1)returnNO_SYSCALL;
@@ -539,6 +540,10 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,*/if(likely(!(fault&(VM_FAULT_ERROR|VM_FAULT_BADMAP|VM_FAULT_BADACCESS)))){+/* No signal was generated, but notify task-isolation tasks. */+if(user_mode(regs))+task_isolation_interrupt("page fault at %#lx",addr);+/**Major/minorpagefaultaccountingisonlydone*once.Ifwegothrougharetry,itisextremely
From: Alex Belits <hidden> Date: 2020-07-22 14:57:08
From: Francis Giraldeau <redacted>
This patch is a port of the task isolation functionality to the arm 32-bit
architecture. The task isolation needs an additional thread flag that
requires to change the entry assembly code to accept a bitfield larger than
one byte. The constants _TIF_SYSCALL_WORK and _TIF_WORK_MASK are now
defined in the literal pool. The rest of the patch is straightforward and
reflects what is done on other architectures.
To avoid problems with the tst instruction in the v7m build, we renumber
TIF_SECCOMP to bit 8 and let TIF_TASK_ISOLATION use bit 7.
Early kernel entry relies on task_isolation_kernel_enter().
vector_swi to label __sys_trace
-> syscall_trace_enter() when task isolation is enabled,
-> task_isolation_kernel_enter()
nvic_handle_irq()
-> handle_IRQ() -> __handle_domain_irq() -> task_isolation_kernel_enter()
__fiq_svc, __fiq_abt __fiq_usr
-> handle_fiq_as_nmi() -> uses nmi_enter() / nmi_exit()
__irq_svc -> irq_handler
__irq_usr -> irq_handler
irq_handler
-> (handle_arch_irq or
(arch_irq_handler_default -> (asm_do_IRQ() -> __handle_domain_irq())
or do_IPI() -> handle_IPI())
asm_do_IRQ()
-> __handle_domain_irq() -> task_isolation_kernel_enter()
do_IPI()
-> handle_IPI() -> task_isolation_kernel_enter()
handle_arch_irq for arm-specific controllers calls
(handle_IRQ() -> __handle_domain_irq() -> task_isolation_kernel_enter())
or (handle_domain_irq() -> __handle_domain_irq()
-> task_isolation_kernel_enter())
Not covered:
__dabt_svc -> dabt_helper
__dabt_usr -> dabt_helper
dabt_helper -> CPU_DABORT_HANDLER (cpu-specific)
-> do_DataAbort or PROCESSOR_DABT_FUNC
-> _data_abort (cpu-specific) -> do_DataAbort
__pabt_svc -> pabt_helper
__pabt_usr -> pabt_helper
pabt_helper -> CPU_PABORT_HANDLER (cpu-specific)
-> do_PrefetchAbort or PROCESSOR_PABT_FUNC
-> _prefetch_abort (cpu-specific) -> do_PrefetchAbort
Signed-off-by: Francis Giraldeau <redacted>
Signed-off-by: Chris Metcalf <redacted> [with modifications]
[abelits@marvell.com: modified for kernel 5.6, added isolation cleanup]
Signed-off-by: Alex Belits <redacted>
---
arch/arm/Kconfig | 1 +
arch/arm/include/asm/barrier.h | 2 ++
arch/arm/include/asm/thread_info.h | 10 +++++++---
arch/arm/kernel/entry-common.S | 15 ++++++++++-----
arch/arm/kernel/ptrace.c | 12 ++++++++++++
arch/arm/kernel/signal.c | 13 ++++++++++++-
arch/arm/kernel/smp.c | 6 ++++++
arch/arm/mm/fault.c | 8 +++++++-
8 files changed, 57 insertions(+), 10 deletions(-)
@@ -251,7 +255,8 @@ local_restart:ldrr10,[tsk,#TI_FLAGS] @ check for syscall tracingstmdbsp!,{r4,r5}@pushfifthandsixthargs-tstr10,#_TIF_SYSCALL_WORK @ are we tracing syscalls?+ldrr11,=_TIF_SYSCALL_WORK@arewetracingsyscalls?+tstr10,r11bne__sys_traceinvoke_syscalltbl,scno,r10,__ret_fast_syscall
@@ -917,9 +918,20 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno){current_thread_info()->syscall=scno;+task_isolation_kernel_enter();+if(test_thread_flag(TIF_SYSCALL_TRACE))tracehook_report_syscall(regs,PTRACE_SYSCALL_ENTER);+/*+*Intaskisolationmode,wemaypreventthesyscallfrom+*running,andifsowealsodeliverasignaltotheprocess.+*/+if(test_thread_flag(TIF_TASK_ISOLATION)){+if(task_isolation_syscall(scno)==-1)+return-1;+}+/* Do seccomp after ptrace; syscall may have changed. */#ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTERif(secure_computing()==-1)
@@ -330,8 +331,13 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)/**Handlethe"normal"casefirst-VM_FAULT_MAJOR*/-if(likely(!(fault&(VM_FAULT_ERROR|VM_FAULT_BADMAP|VM_FAULT_BADACCESS))))+if(likely(!(fault&(VM_FAULT_ERROR|VM_FAULT_BADMAP|+VM_FAULT_BADACCESS)))){+/* No signal was generated, but notify task-isolation tasks. */+if(user_mode(regs))+task_isolation_interrupt("page fault at %#lx",addr);return0;+}/**Ifweareinkernelmodeatthispoint,we
From: Alex Belits <hidden> Date: 2020-07-22 14:58:04
From: Yuri Norov <redacted>
For nohz_full CPUs the desirable behavior is to receive interrupts
generated by tick_nohz_full_kick_cpu(). But for hard isolation it's
obviously not desirable because it breaks isolation.
This patch adds check for it.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: updated, only exclude CPUs running isolated tasks]
Signed-off-by: Alex Belits <redacted>
---
kernel/time/tick-sched.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Alex Belits <hidden> Date: 2020-07-22 14:58:55
From: Yuri Norov <redacted>
If CPU runs isolated task, there's no any backlog on it, and
so we don't need to flush it. Currently flush_all_backlogs()
enqueues corresponding work on all CPUs including ones that run
isolated tasks. It leads to breaking task isolation for nothing.
In this patch, backlog flushing is enqueued only on non-isolated CPUs.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: use safe task_isolation_on_cpu() implementation]
Signed-off-by: Alex Belits <redacted>
---
net/core/dev.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Alex Belits <hidden> Date: 2020-07-22 14:59:49
From: Yuri Norov <redacted>
CPUs running isolated tasks are in userspace, so they don't have to
perform ring buffer updates immediately. If ring_buffer_resize()
schedules the update on those CPUs, isolation is broken. To prevent
that, updates for CPUs running isolated tasks are performed locally,
like for offline CPUs.
A race condition between this update and isolation breaking is avoided
at the cost of disabling per_cpu buffer writing for the time of update
when it coincides with isolation breaking.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: updated to prevent race with isolation breaking]
Signed-off-by: Alex Belits <redacted>
---
kernel/trace/ring_buffer.c | 63 ++++++++++++++++++++++++++++++++++----
1 file changed, 57 insertions(+), 6 deletions(-)
@@ -1705,6 +1706,38 @@ static void update_pages_handler(struct work_struct *work)complete(&cpu_buffer->update_done);}+staticboolupdate_if_isolated(structring_buffer_per_cpu*cpu_buffer,+intcpu)+{+boolrv=false;++smp_rmb();+if(task_isolation_on_cpu(cpu)){+/*+*CPUisrunningisolatedtask.Sinceitmaylose+*isolationandre-enterkernelsimultaneouslywith+*thisupdate,disablerecordinguntilit'sdone.+*/+atomic_inc(&cpu_buffer->record_disabled);+/* Make sure, update is done, and isolation state is current */+smp_mb();+if(task_isolation_on_cpu(cpu)){+/*+*IfCPUisstillrunningisolatedtask,we+*canbesurethatbreakingisolationwill+*happenwhilerecordingisdisabled,andCPU+*willnottouchthisbufferuntiltheupdate+*isdone.+*/+rb_update_pages(cpu_buffer);+cpu_buffer->nr_pages_to_update=0;+rv=true;+}+atomic_dec(&cpu_buffer->record_disabled);+}+returnrv;+}+/***ring_buffer_resize-resizetheringbuffer*@buffer:thebuffertoresize.
@@ -1794,13 +1827,22 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,if(!cpu_buffer->nr_pages_to_update)continue;-/* Can't run something on an offline CPU. */+/*+*Can'trunsomethingonanofflineCPU.+*+*CPUsrunningisolatedtasksdon'thaveto+*updateringbuffersuntiltheyexit+*isolationbecausetheyarein+*userspace.Usetheprocedurethatprevents+*raceconditionwithisolationbreaking.+*/if(!cpu_online(cpu)){rb_update_pages(cpu_buffer);cpu_buffer->nr_pages_to_update=0;}else{-schedule_work_on(cpu,-&cpu_buffer->update_pages_work);+if(!update_if_isolated(cpu_buffer,cpu))+schedule_work_on(cpu,+&cpu_buffer->update_pages_work);}}
@@ -1849,13 +1891,22 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,get_online_cpus();-/* Can't run something on an offline CPU. */+/*+*Can'trunsomethingonanofflineCPU.+*+*CPUsrunningisolatedtasksdon'thavetoupdate+*ringbuffersuntiltheyexitisolationbecausethey+*areinuserspace.Usetheprocedurethatprevents+*raceconditionwithisolationbreaking.+*/if(!cpu_online(cpu_id))rb_update_pages(cpu_buffer);else{-schedule_work_on(cpu_id,+if(!update_if_isolated(cpu_buffer,cpu_id))+schedule_work_on(cpu_id,&cpu_buffer->update_pages_work);-wait_for_completion(&cpu_buffer->update_done);+wait_for_completion(&cpu_buffer->update_done);+}}cpu_buffer->nr_pages_to_update=0;
@@ -803,9 +803,21 @@ static void do_nothing(void *unused)*/voidkick_all_cpus_sync(void){+structcpumaskmask;+/* Make sure the change is visible before we kick the cpus */smp_mb();-smp_call_function(do_nothing,NULL,1);++preempt_disable();+#ifdef CONFIG_TASK_ISOLATION+cpumask_clear(&mask);+task_isolation_cpumask(&mask);+cpumask_complement(&mask,&mask);+#else+cpumask_setall(&mask);+#endif+smp_call_function_many(&mask,do_nothing,NULL,1);+preempt_enable();}EXPORT_SYMBOL_GPL(kick_all_cpus_sync);
From: Thomas Gleixner <hidden> Date: 2020-07-23 13:17:09
Alex,
Alex Belits [off-list ref] writes:
This is a new version of task isolation implementation. Previous version is at
https://lore.kernel.org/lkml/07c25c246c55012981ec0296eee23e68c719333a.camel@marvell.com/
Mostly this covers race conditions prevention on breaking isolation. Early after kernel entry,
task_isolation_enter() is called to update flags visible to other CPU cores and to perform
synchronization if necessary. Before this call only "safe" operations happen, as long as
CONFIG_TRACE_IRQFLAGS is not enabled.
Without going into details of the individual patches, let me give you a
high level view of this series:
1) Entry code handling:
That's completely broken vs. the careful ordering and instrumentation
protection of the entry code. You can't just slap stuff randomly
into places which you think are safe w/o actually trying to understand
why this code is ordered in the way it is.
This clearly was never built and tested with any of the relevant
debug options enabled. Both build and boot would have told you.
2) Instruction synchronization
Trying to do instruction synchronization delayed is a clear recipe
for hard to diagnose failures. Just because it blew not up in your
face does not make it correct in any way. It's broken by design and
violates _all_ rules of safe instruction patching and introduces a
complete trainwreck in x86 NMI processing.
If you really think that this is correct, then please have at least
the courtesy to come up with a detailed and precise argumentation
why this is a valid approach.
While writing that up you surely will find out why it is not.
3) Debug calls
Sprinkling debug calls around the codebase randomly is not going to
happen. That's an unmaintainable mess.
Aside of that none of these dmesg based debug things is necessary.
This can simply be monitored with tracing.
4) Tons of undocumented smp barriers
See Documentation/process/submit-checklist.rst #25
5) Signal on page fault
Why is this a magic task isolation feature instead of making it
something which can be used in general? There are other legit
reasons why a task might want a notification about an unexpected
(resolved) page fault.
6) Coding style violations all over the place
Using checkpatch.pl is mandatory
7) Not Cc'ed maintainers
While your Cc list is huge, you completely fail to Cc the relevant
maintainers of various files and subsystems as requested in
Documentation/process/*
8) Changelogs
Most of the changelogs have something along the lines:
'task isolation does not want X, so do Y to make it not do X'
without any single line of explanation why this approach was chosen
and why it is correct under all circumstances and cannot have nasty
side effects.
It's not the job of the reviewers/maintainers to figure this out.
Please come up with a coherent design first and then address the
identified issues one by one in a way which is palatable and reviewable.
Throwing a big pile of completely undocumented 'works for me' mess over
the fence does not get you anywhere, not even to the point that people
are willing to review it in detail.
Thanks,
tglx
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-07-23 14:26:38
On Thu, Jul 23, 2020 at 03:17:04PM +0200, Thomas Gleixner wrote:
2) Instruction synchronization
Trying to do instruction synchronization delayed is a clear recipe
for hard to diagnose failures. Just because it blew not up in your
face does not make it correct in any way. It's broken by design and
violates _all_ rules of safe instruction patching and introduces a
complete trainwreck in x86 NMI processing.
If you really think that this is correct, then please have at least
the courtesy to come up with a detailed and precise argumentation
why this is a valid approach.
While writing that up you surely will find out why it is not.
So delaying the sync_core() IPIs for kernel text patching _might_ be
possible, but it very much wants to be a separate patchset and not
something hidden inside a 'gem' like this.
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-07-23 14:29:20
On Thu, Jul 23, 2020 at 03:17:04PM +0200, Thomas Gleixner wrote:
8) Changelogs
Most of the changelogs have something along the lines:
'task isolation does not want X, so do Y to make it not do X'
without any single line of explanation why this approach was chosen
and why it is correct under all circumstances and cannot have nasty
side effects.
It's not the job of the reviewers/maintainers to figure this out.
Please come up with a coherent design first and then address the
identified issues one by one in a way which is palatable and reviewable.
Throwing a big pile of completely undocumented 'works for me' mess over
the fence does not get you anywhere, not even to the point that people
are willing to review it in detail.
This.. as presented it is an absolutely unreviewable pile of junk. It
presents code witout any coherent problem description and analysis. And
the patches are not split sanely either.
From: Thomas Gleixner <hidden> Date: 2020-07-23 14:53:23
Peter Zijlstra [off-list ref] writes:
On Thu, Jul 23, 2020 at 03:17:04PM +0200, Thomas Gleixner wrote:
quoted
2) Instruction synchronization
Trying to do instruction synchronization delayed is a clear recipe
for hard to diagnose failures. Just because it blew not up in your
face does not make it correct in any way. It's broken by design and
violates _all_ rules of safe instruction patching and introduces a
complete trainwreck in x86 NMI processing.
If you really think that this is correct, then please have at least
the courtesy to come up with a detailed and precise argumentation
why this is a valid approach.
While writing that up you surely will find out why it is not.
So delaying the sync_core() IPIs for kernel text patching _might_ be
possible, but it very much wants to be a separate patchset and not
something hidden inside a 'gem' like this.
I'm not saying it's impossible, but the proposed hack is definitely
beyond broken and you really don't want to be the one who has to mop up
the pieces later.
Thanks,
tglx
From: Alex Belits <hidden> Date: 2020-07-23 15:19:42
On Thu, 2020-07-23 at 15:17 +0200, Thomas Gleixner wrote:
Without going into details of the individual patches, let me give you a
high level view of this series:
1) Entry code handling:
That's completely broken vs. the careful ordering and instrumentation
protection of the entry code. You can't just slap stuff randomly
into places which you think are safe w/o actually trying to understand
why this code is ordered in the way it is.
This clearly was never built and tested with any of the relevant
debug options enabled. Both build and boot would have told you.
This is intended to avoid a race condition when entry or exit from isolation
happens at the same time as an event that requires synchronization. The idea
is, it is possible to insulate the core from all events while it is running
isolated task in userspace, it will receive those calls normally after
breaking isolation and entering kernel, and it will synchronize itself on
kernel entry.
This has two potential problems that I am trying to solve:
1. Without careful ordering, there will be a race condition with events that
happen at the same time as kernel entry or exit.
2. CPU runs some kernel code after entering but before synchronization. This
code should be restricted to early entry that is not affected by the "stale"
state, similar to how IPI code that receives synchronization events does it
normally.
I can't say that I am completely happy with the amount of kernel entry
handling that had to be added. The problem is, I am trying to introduce a
feature that allows CPU cores to go into "de-synchronized" state while running
isolated tasks and not receiving synchronization events that normally would
reach them. This means, there should be established some point on kernel entry
when it is safe for the core to catch up with the rest of kernel. It may be
useful for other purposes, however at this point task isolation is the first
to need it, so I had to determine where such point is for every supported
architecture and method of kernel entry.
I have found that each architecture has its own way of handling this,
and sometimes individual interrupt controller drivers vary in their
sequence of calls on early kernel entry. For x86 I also have an
implementation for kernel 5.6, before your changes to IDT macros.
That version is much less straightforward, so I am grateful for those
relatively recent improvements.
Nevertheless, I believe that the goal of finding those points and using
them for synchronization is valid. If you can recommend me a better way
for at least x86, I will be happy to follow your advice. I have tried to
cover kernel entry in a generic way while making the changes least
disruptive, and this is why it looks simple and spread over multiple
places. I also had to do the same for arm and arm64 (that I use for
development), and for each architecture I had to produce sequences of
entry points and function calls to determine the correct placement of
task_isolation_enter() calls in them. It is not random, however it does
reflect the complex nature of kernel entry code. I believe, RCU
implementation faced somewhat similar requirements for calls on kernel
entry, however it is not completely unified, either
2) Instruction synchronization
Trying to do instruction synchronization delayed is a clear recipe
for hard to diagnose failures. Just because it blew not up in your
face does not make it correct in any way. It's broken by design and
violates _all_ rules of safe instruction patching and introduces a
complete trainwreck in x86 NMI processing.
The idea is that just like synchronization events are handled by regular IPI,
we already use some code with the assumption that it is safe to be entered in
"stale" state before synchronization. I have extended it to allow
synchronization points on all kernel entry points.
If you really think that this is correct, then please have at least
the courtesy to come up with a detailed and precise argumentation
why this is a valid approach.
While writing that up you surely will find out why it is not.
I had to document a sequence of calls for every entry point on three supported
architectures, to determine the points for synchronization. It is possible that
I have somehow missed something, however I don't see a better approach, save
for establishing a kernel-wide infrastructure for this. And even if we did just
that, it would be possible to implement this kind of synchronization point
calls first, and convert them to something more generic later.
3) Debug calls
Sprinkling debug calls around the codebase randomly is not going to
happen. That's an unmaintainable mess.
Those report isolation breaking causes, and are intended for application and
system debugging.
Aside of that none of these dmesg based debug things is necessary.
This can simply be monitored with tracing.
I think, it would be better to make all that information available to the
userspace application, however I have based this on the Chris Metcalf code,
and gradually updated the mechanisms and interfaces. The original reporting
of isolation breaking causes had far greater problems, so at first I wanted
to have something that produces easily visible and correct reporting, and
does not break things while doing so.
4) Tons of undocumented smp barriers
See Documentation/process/submit-checklist.rst #25
That should be fixed.
5) Signal on page fault
Why is this a magic task isolation feature instead of making it
something which can be used in general? There are other legit
reasons why a task might want a notification about an unexpected
(resolved) page fault.
Page fault causes isolation breaking. When a task runs in isolated mode it
does so because it requires predictable timing, so causing page faults and
expecting them to be handled along the way would defeat the purpose of
isolation. So if page fault did happen, it is important that application will
receive notification about isolation being broken, and then may decide to do
something about it, re-enter isolation, etc.
6) Coding style violations all over the place
Using checkpatch.pl is mandatory
7) Not Cc'ed maintainers
While your Cc list is huge, you completely fail to Cc the relevant
maintainers of various files and subsystems as requested in
Documentation/process/*
To be honest, I am not sure, whom I have missed, I tried to include everyone
from my previous attempt.
8) Changelogs
Most of the changelogs have something along the lines:
'task isolation does not want X, so do Y to make it not do X'
without any single line of explanation why this approach was chosen
and why it is correct under all circumstances and cannot have nasty
side effects.
This is the same as the previous version, except for the addition of kernel
entry handling. As far as I can tell, the rest was discussed before, and not
many questions remained except for the race condition on kernel entry. I
agree that kernel entry handling is a complex issue in itself, so I have
included explanation of entry points / function calls sequences for each
supported architecture. I have longer call diagram, that I used to track
each particular function, it probably should be included as a separate
document.
It's not the job of the reviewers/maintainers to figure this out.
Please come up with a coherent design first and then address the
identified issues one by one in a way which is palatable and reviewable.
Throwing a big pile of completely undocumented 'works for me' mess over
the fence does not get you anywhere, not even to the point that people
are willing to review it in detail.
There is a design, and it is a result of a careful tracking of calls in the
kernel source. It has multiple point where task_isolation_enter() is called
for a reason similar to why RCU-related functions are called in multiple
places.
If someone can recommend a better way to introduce a kernel entry
checkpoint for synchronization that did not exist before, I will be happy
to hear it.
--
Alex
From: Alex Belits <hidden> Date: 2020-07-23 15:42:33
On Thu, 2020-07-23 at 16:29 +0200, Peter Zijlstra wrote:
.
This.. as presented it is an absolutely unreviewable pile of junk. It
presents code witout any coherent problem description and analysis.
And
the patches are not split sanely either.
There is a more complete and slightly outdated description in the
previous version of the patch at
https://lore.kernel.org/lkml/07c25c246c55012981ec0296eee23e68c719333a.camel@marvell.com/
.
It allows userspace application to take a CPU core for itself and run
completely isolated, with no disturbances. There is work in progress
that also disables and re-enables TLB flushes, and depending on CPU it
may be possible to also pre-allocate cache, so it would not be affected
by the rest of the system. Events that cause interaction with isolated
task, cause isolation breaking, turning the task into a regular
userspace task that can continue running normally and enter isolated
state again if necessary.
To make this feature suitable for any practical use, many mechanisms
that normally would cause events on a CPU, should exclude CPU cores in
this state, and synchronization should happen later, at the time of
isolation breaking.
There are three architectures supported, x86, arm and arm64, and it
should be possible to extend it to others. Unfortunately kernel entry
procedures are neither unified, nor straightforward, so introducing new
feature to them causes an appearance of a mess.
--
Alex
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-07-23 15:48:36
On Thu, Jul 23, 2020 at 03:41:46PM +0000, Alex Belits wrote:
On Thu, 2020-07-23 at 16:29 +0200, Peter Zijlstra wrote:
quoted
.
This.. as presented it is an absolutely unreviewable pile of junk. It
presents code witout any coherent problem description and analysis.
And
the patches are not split sanely either.
Not the point, you're mixing far too many things in one go. You also
have the patches split like 'generic / arch-1 / arch-2' which is wrong
per definition, as patches should be split per change and not care about
sily boundaries.
Also, if you want generic entry code, there's patches for that here:
https://lkml.kernel.org/r/20200722215954.464281930@linutronix.de
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-07-23 15:49:46
On Thu, Jul 23, 2020 at 03:18:42PM +0000, Alex Belits wrote:
On Thu, 2020-07-23 at 15:17 +0200, Thomas Gleixner wrote:
quoted
Without going into details of the individual patches, let me give you a
high level view of this series:
1) Entry code handling:
That's completely broken vs. the careful ordering and instrumentation
protection of the entry code. You can't just slap stuff randomly
into places which you think are safe w/o actually trying to understand
why this code is ordered in the way it is.
This clearly was never built and tested with any of the relevant
debug options enabled. Both build and boot would have told you.
This is intended to avoid a race condition when entry or exit from isolation
happens at the same time as an event that requires synchronization. The idea
is, it is possible to insulate the core from all events while it is running
isolated task in userspace, it will receive those calls normally after
breaking isolation and entering kernel, and it will synchronize itself on
kernel entry.
'What does noinstr mean? and why do we have it" -- don't dare touch the
entry code until you can answer that.
From: Alex Belits <hidden> Date: 2020-07-23 16:20:12
On Thu, 2020-07-23 at 17:48 +0200, Peter Zijlstra wrote:
On Thu, Jul 23, 2020 at 03:41:46PM +0000, Alex Belits wrote:
quoted
On Thu, 2020-07-23 at 16:29 +0200, Peter Zijlstra wrote:
quoted
.
This.. as presented it is an absolutely unreviewable pile of
junk. It
presents code witout any coherent problem description and
analysis.
And
the patches are not split sanely either.
Not the point, you're mixing far too many things in one go. You also
have the patches split like 'generic / arch-1 / arch-2' which is
wrong
per definition, as patches should be split per change and not care
about
sily boundaries.
This follows the original patch by Chris Metcalf. There is a reason for
that -- per-architecture changes are independent from each other and
affect not just code but functionality that was implemented per-
architecture. To support more architectures, it will be necessary to do
it separately for each, and mark them supported with
HAVE_ARCH_TASK_ISOLATION. Having only some architectures supported does
not break anything for the rest -- architectures that are not covered,
would not have this functionality.
That looks useful. Why didn't Thomas Gleixner mention it in his
criticism of my approach if he already solved that exact problem, at
least for x86?
--
Alex
From: Alex Belits <hidden> Date: 2020-07-23 16:51:22
On Thu, 2020-07-23 at 17:49 +0200, Peter Zijlstra wrote:
'What does noinstr mean? and why do we have it" -- don't dare touch
the
entry code until you can answer that.
noinstr disables instrumentation, so there would not be calls and
dependencies on other parts of the kernel when it's not yet safe to
call them. Relevant functions already have it, and I add an inline call
to perform flags update and synchronization. Unless something else is
involved, those operations are safe, so I am not adding anything that
can break those.
--
Alex
From: Thomas Gleixner <hidden> Date: 2020-07-23 21:31:27
Alex,
Alex Belits [off-list ref] writes:
On Thu, 2020-07-23 at 15:17 +0200, Thomas Gleixner wrote:
quoted
Without going into details of the individual patches, let me give you a
high level view of this series:
1) Entry code handling:
That's completely broken vs. the careful ordering and instrumentation
protection of the entry code. You can't just slap stuff randomly
into places which you think are safe w/o actually trying to understand
why this code is ordered in the way it is.
This clearly was never built and tested with any of the relevant
debug options enabled. Both build and boot would have told you.
This is intended to avoid a race condition when entry or exit from isolation
happens at the same time as an event that requires synchronization. The idea
is, it is possible to insulate the core from all events while it is running
isolated task in userspace, it will receive those calls normally after
breaking isolation and entering kernel, and it will synchronize itself on
kernel entry.
It does not matter what your intention is. Fact is that you disrupt a
carefully designed entry code sequence without even trying to point out
that you did so because you don't know how to do it better. There is a
big fat comment above enter_from_user_mode() which should have make you
ask at least. Peter and myself spent month on getting this correct
vs. RCU, instrumentation, code patching and some more things.
From someone who tries to fiddle with such a sensitive area of code it's
not too much asked that he follows or reads up on these changes instead
of just making uninformed choices of placement by defining that this new
stuff is the most important thing on the planet or at least documenting
why this is correct and not violating any of the existing constraints.
This has two potential problems that I am trying to solve:
1. Without careful ordering, there will be a race condition with events that
happen at the same time as kernel entry or exit.
Entry code is all about ordering. News at 11.
2. CPU runs some kernel code after entering but before synchronization. This
code should be restricted to early entry that is not affected by the "stale"
state, similar to how IPI code that receives synchronization events does it
normally.
And because of that you define that you can place anything you need
_before_ functionality which is essential for establishing kernel state
correctly without providing the minimum proof that this does not violate
any of the existing contraints.
reach them. This means, there should be established some point on kernel entry
when it is safe for the core to catch up with the rest of kernel. It may be
useful for other purposes, however at this point task isolation is the first
to need it, so I had to determine where such point is for every supported
architecture and method of kernel entry.
You decided that your feature has to run first. Where is the analysis
that this is safe and correct vs. the existing ordering constraints?
Why does this trigger build and run time warnings? (I neither built nor
ran it, but with full debug enabled it will for sure).
Nevertheless, I believe that the goal of finding those points and using
them for synchronization is valid.
The goal does not justify the means.
If you can recommend me a better way for at least x86, I will be happy
to follow your advice. I have tried to cover kernel entry in a generic
way while making the changes least disruptive, and this is why it
looks simple and spread over multiple places.
It does not look simple. It looks random and like the outcome of try and
error. Oh, here it explodes, lets slap another instance into it.
I also had to do the same for arm and arm64 (that I use for
development), and for each architecture I had to produce sequences of
entry points and function calls to determine the correct placement of
task_isolation_enter() calls in them. It is not random, however it does
reflect the complex nature of kernel entry code. I believe, RCU
implementation faced somewhat similar requirements for calls on kernel
entry, however it is not completely unified, either
But RCU has a well defined design and requirement list and people are
working on making the entry sequence generic and convert architectures
over to it. And no, we don't try to do 5 architectures at once. We did
x86 with an eye on others. It's not perfect and it never will be because
of hardware.
quoted
2) Instruction synchronization
Trying to do instruction synchronization delayed is a clear recipe
for hard to diagnose failures. Just because it blew not up in your
face does not make it correct in any way. It's broken by design and
violates _all_ rules of safe instruction patching and introduces a
complete trainwreck in x86 NMI processing.
The idea is that just like synchronization events are handled by regular IPI,
we already use some code with the assumption that it is safe to be entered in
"stale" state before synchronization. I have extended it to allow
synchronization points on all kernel entry points.
The idea is clear, just where is the analysis that this is safe?
Just from quickly skimming the code it's clear that this has never been
done. Experimental development on the base of 'does not explode' is not
a valid approach in the kernel whatever your goal is.
quoted
If you really think that this is correct, then please have at least
the courtesy to come up with a detailed and precise argumentation
why this is a valid approach.
While writing that up you surely will find out why it is not.
I had to document a sequence of calls for every entry point on three supported
architectures, to determine the points for synchronization.
Why is that documentation not part of the patches in form of
documentation or proper changelogs?
It is possible that I have somehow missed something, however I don't
see a better approach, save for establishing a kernel-wide
infrastructure for this. And even if we did just that, it would be
possible to implement this kind of synchronization point calls first,
and convert them to something more generic later.
You're putting the cart before the horse.
You want delayed instruction patching synchronization. So the right
approach is to:
1) Analyze the constraints of instruction patching on a given
architecture.
2) Implement a scheme for this architecture to handle delayed
patching as a stand alone feature with well documented and fine
grained patches and proper prove that none of the constraints is
violated.
Find good arguments why such a feature is generally useful and not
only for your personal pet pieve.
Once you've done that, then you'll find out that there is no need for
magic task isolation hackery simply because it's already there.
Code patching is very much architecture specific and the constraints
vary due to the different hardware requirements. The idea of making this
generic is laudable, but naive at best. Once you have done #1 above on
two architectures you will know why.
quoted
3) Debug calls
Sprinkling debug calls around the codebase randomly is not going to
happen. That's an unmaintainable mess.
Those report isolation breaking causes, and are intended for application and
system debugging.
I don't care what they do as that does not make them more palatable or
maintainable.
quoted
Aside of that none of these dmesg based debug things is necessary.
This can simply be monitored with tracing.
I think, it would be better to make all that information available to the
userspace application, however I have based this on the Chris Metcalf code,
and gradually updated the mechanisms and interfaces. The original reporting
of isolation breaking causes had far greater problems, so at first I wanted
to have something that produces easily visible and correct reporting, and
does not break things while doing so.
Why are you exposing other people to these horrors? I don't care what
you use in your development branch and I don't care what you share with
your friends, but if you want maintainers and reviewers to look at that
stuff then ensure that what you present:
- Makes sense
- Is properly implemented
- Is properly documented
- Is properly argumented why this is the right approach.
'I need', 'I want', 'this does' are non-arguments to begin with.
quoted
5) Signal on page fault
Why is this a magic task isolation feature instead of making it
something which can be used in general? There are other legit
reasons why a task might want a notification about an unexpected
(resolved) page fault.
Page fault causes isolation breaking. When a task runs in isolated mode it
does so because it requires predictable timing, so causing page faults and
expecting them to be handled along the way would defeat the purpose of
isolation. So if page fault did happen, it is important that application will
receive notification about isolation being broken, and then may decide to do
something about it, re-enter isolation, etc.
Did you actually read what I wrote? I very much understood what you are
trying to do and why. Otherwise I wouldn't have written the above.
quoted
6) Coding style violations all over the place
Using checkpatch.pl is mandatory
7) Not Cc'ed maintainers
While your Cc list is huge, you completely fail to Cc the relevant
maintainers of various files and subsystems as requested in
Documentation/process/*
To be honest, I am not sure, whom I have missed, I tried to include everyone
from my previous attempt.
May I ask you to read, understand and follow the documentation I pointed
you to?
quoted
8) Changelogs
Most of the changelogs have something along the lines:
'task isolation does not want X, so do Y to make it not do X'
without any single line of explanation why this approach was chosen
and why it is correct under all circumstances and cannot have nasty
side effects.
This is the same as the previous version, except for the addition of kernel
entry handling. As far as I can tell, the rest was discussed before, and not
many questions remained except for the race condition on kernel entry.
How is that related to changelogs which are useless?
agree that kernel entry handling is a complex issue in itself, so I have
included explanation of entry points / function calls sequences for each
supported architecture.
Which explanations? Let's talk about 7/13 the x86 part:
In prepare_exit_to_usermode(), run cleanup for tasks exited fromi
isolation and call task_isolation_start() for tasks that entered
TIF_TASK_ISOLATION.
In syscall_trace_enter(), add the necessary support for reporting
syscalls for task-isolation processes.
Add task_isolation_remote() calls for the kernel exception types
that do not result in signals, namely non-signalling page faults.
Add task_isolation_kernel_enter() calls to interrupt and syscall
entry handlers.
This mechanism relies on calls to functions that call
task_isolation_kernel_enter() early after entry into kernel. Those
functions are:
enter_from_user_mode()
called from do_syscall_64(), do_int80_syscall_32(),
do_fast_syscall_32(), idtentry_enter_user(),
idtentry_enter_cond_rcu()
idtentry_enter_cond_rcu()
called from non-raw IDT macros and other entry points
idtentry_enter_user()
nmi_enter()
xen_call_function_interrupt()
xen_call_function_single_interrupt()
xen_irq_work_interrupt()
Can you point me to a single word of explanation in this blurb?
It's a list of things WHAT the patch does without a single word of WHY
and without a single word of WHY any of this would be correct.
I have longer call diagram, that I used to track each particular
function, it probably should be included as a separate document.
Call diagrams are completely useless. The people who have to review this
know how that works. They want real explanations:
- Why is this the right approach
- Why does this not violate constraints A, B, C
- What are the potential side effects
- ...
All of this is asked for in Documentation/process/* for a reason.
quoted
It's not the job of the reviewers/maintainers to figure this out.
Please come up with a coherent design first and then address the
identified issues one by one in a way which is palatable and reviewable.
Throwing a big pile of completely undocumented 'works for me' mess over
the fence does not get you anywhere, not even to the point that people
are willing to review it in detail.
There is a design, and it is a result of a careful tracking of calls in the
kernel source. It has multiple point where task_isolation_enter() is called
for a reason similar to why RCU-related functions are called in multiple
places.
Design based on call tracking? That must be some newfangled method of
design which was not taught when I was in school.
You can do analysis with call tracking, but not design.
Comparing this to RCU is beyond hillarious. RCU has design and
requirements documented and every single instance of RCU state
establishment has been argued in the changelogs and is most of the time
(except for the obvious places) extensively commented.
If someone can recommend a better way to introduce a kernel entry
checkpoint for synchronization that did not exist before, I will be happy
to hear it.
Start with a coherent explanation of:
- What you are trying to achieve
- Which problems did you observe in your analysis including the
impact of the problem on your goal.
- A per problem conceptual approach to solve it along with cleanly
implemented and independent RFC code for each particular problem
without tons of debug hacks and the vain attempts to make everything
generic. There might be common parts of it, but as explained with
code patching and #PF signals they can be completely independent of
each other.
Thanks,
tglx
From: Thomas Gleixner <hidden> Date: 2020-07-23 21:44:28
Alex Belits [off-list ref] writes:
On Thu, 2020-07-23 at 17:49 +0200, Peter Zijlstra wrote:
quoted
'What does noinstr mean? and why do we have it" -- don't dare touch
the
entry code until you can answer that.
noinstr disables instrumentation, so there would not be calls and
dependencies on other parts of the kernel when it's not yet safe to
call them. Relevant functions already have it, and I add an inline call
to perform flags update and synchronization. Unless something else is
involved, those operations are safe, so I am not adding anything that
can break those.
Sure.
1) That inline function can be put out of line by the compiler and
placed into the regular text section which makes it subject to
instrumentation
2) That inline function invokes local_irq_save() which is subject to
instrumentation _before_ the entry state for the instrumentation
mechanisms is established.
3) That inline function invokes sync_core() before important state has
been established, which is especially interesting in NMI like
exceptions.
As you clearly documented why all of the above is safe and does not
cause any problems, it's just me and Peter being silly, right?
Try again.
Thanks,
tglx
From: Alex Belits <hidden> Date: 2020-07-24 03:00:56
On Thu, 2020-07-23 at 23:44 +0200, Thomas Gleixner wrote:
External Email
-------------------------------------------------------------------
---
Alex Belits [off-list ref] writes:
quoted
On Thu, 2020-07-23 at 17:49 +0200, Peter Zijlstra wrote:
quoted
'What does noinstr mean? and why do we have it" -- don't dare
touch
the
entry code until you can answer that.
noinstr disables instrumentation, so there would not be calls and
dependencies on other parts of the kernel when it's not yet safe to
call them. Relevant functions already have it, and I add an inline
call
to perform flags update and synchronization. Unless something else
is
involved, those operations are safe, so I am not adding anything
that
can break those.
Sure.
1) That inline function can be put out of line by the compiler and
placed into the regular text section which makes it subject to
instrumentation
2) That inline function invokes local_irq_save() which is subject to
instrumentation _before_ the entry state for the instrumentation
mechanisms is established.
3) That inline function invokes sync_core() before important state
has
been established, which is especially interesting in NMI like
exceptions.
As you clearly documented why all of the above is safe and does not
cause any problems, it's just me and Peter being silly, right?
Try again.
I don't think, accusations and mockery are really necessary here.
I am trying to do the right thing here. In particular, I am trying to
port the code that was developed on platforms that have not yet
implemented those useful instrumentation safety features of x86 arch
support. For most of the development time I had to figure out, where
the synchronization can be safely inserted into kernel entry code on
three platforms and tens of interrupt controller drivers, with some of
those presenting unusual exceptions (forgive me the pun) from platform-
wide conventions. I really appreciate the work you did cleaning up
kernel entry procedures, my 5.6 version of this patch had to follow a
much more complex and I would say, convoluted entry handling on x86,
and now I don't have to do that, thanks to you.
Unfortunately, most of my mental effort recently had to be spent on
three things:
1. (small): finding a way to safely enable events and synchronize state
on kernel entry, so it will not have a race condition between
isolation-breaking kernel entry and an event that was disabled while
the task was isolated.
2. (big): trying to derive any useful rules applicable to kernel entry
in various architectures, finding that there is very little consistency
across architectures, and whatever exists, can be broken by interrupt
controller drivers that don't all follow the same rules as the rest of
the platform.
3. (medium): introducing calls to synchronization on all kernel entry
procedures, in places where it is guaranteed to not normally yet have
done any calls to parts of the kernel that may be affected by "stale"
state, and do it in a manner as consistent and generalized as possible.
The current state of kernel entry handling on arm and arm64
architectures has significant differences from x86 and from each other.
There is also a matter of interrupt controllers. As can be seen in
interrupt controller-specific patch, I had to accommodate some variety
of custom interrupt entry code. What can not be seen, is that I had to
check that all other interrupt controller drivers and architecture-
specific entry procedures, and find that they _do_ follow some
understandable rules -- unfortunately architecture-specific and not
documented in any manner.
I have no valid reasons for complaining about it. I could not expect
that authors of all kernel entry procedures would have any
foreknowledge that someone at some point may have a reason to establish
any kind of synchronization point for CPU cores. And this is why I had
to do my research by manually drawing call trees and sequences,
separately for every entry on every supported architecture, and across
two or three versions of kernel, as those were changing along the way.
The result of this may be not a "design" per se, but an understanding
of how things are implemented, and what rules are being followed, so I
could add my code in a manner consistent with what is done, and
document the whole thing. Then there will be some written rules to
check for, when anything of this kind will be necessary again (say,
with TLB, but considering how much now is done in userspace, possibly
to accommodate more exotic CPU features that may have state messed up
by userspace). I am afraid, this task, kernel entry documentation,
would take me some time, and I did not want to delay my task isolation
patch for this reason.
As I followed whatever rules I have determined to be applicable, I have
produced code that introduces hooks in multiple seemingly unrelated to
each other places. Whenever there was a, forgive me the pun, exception
to those rules, another special case had to be handled.
So no, I did not just add entry hooks randomly, and your accusations of
having no design are unwarranted. My patches reflect what is already in
code and in its design, I have added one simple rule that entry hook
runs at the point when no dependency on something that requires
synchronization, exists yet. The entry hook is small, you have already
seen all of it while listing things that are not compatible with
noinst. Its mechanism and purpose are explained in general description
of task isolation. I don't think, I can provide a better explanation.
I have to apologize for not taking into account all your carefully
built instrumentation safety support. That was one thing I have missed.
However at this point the only way for me to find out that I am wrong
about it, and my code does not comply with expectations defined by
advanced state of x86 architecture development, was to present whatever
I could do right, based on experience with other platforms. I don't
think, this warrants such hostility.
Another issue that you have asked me to defend is the existence and
scope of task isolation itself. I have provided long explanation in
changelog and previous discussions of this patch, and before me so did
Chris Metcalf and occasionally Yuri Norov. While I understand that this
is an unusual feature and by its nature it affects kernel in multiple
places, it does not deserve to be called a "mess" and other synonyms of
"mess". It's an attempt to introduce a feature that turns Linux
userspace into superior replacement of RTOS. Considering current state
of CPU and SoC development, it is becoming very difficult even for
vendor-specific RTOS to keep up with advanced hardware features. Linux
keeps up with them just fine, however it lacks the ability to truly
leave the CPU core alone, to run the performance-critical and latency-
critical part of a task in a manner that RTOS user would expect. Very
close but not yet. Task isolation provides the last step for this RTOS
replacement. It is implemented in a manner that allows the user to
combine Linux resource handling and initialization with RTOS isolation
and latency. The decision about page faults is a part of this design,
as well as many other decisions implemented in this code. Many may
disagree with either those decisions, or the validity of a goal, some
may even argue that it's a bad thing to provide a reason to stop RTOS
development (I think, this is a good thing but that's not the point).
However most definitely this is not a "mess", and it I do not believe
that I have to defend the validity of this direction of development, or
be accused of general incompetence every time someone finds a
frustrating mistake in my code. As I said, I am trying to do the right
thing, and want to bring my code not only to the state where x86
support is on par with other platforms (that is, working when
instrumentation is disabled), but also make it fully compliant with
current requirements of x86 platform.
--
Alex
From: Thomas Gleixner <hidden> Date: 2020-07-24 16:08:20
Alex,
Alex Belits [off-list ref] writes:
On Thu, 2020-07-23 at 23:44 +0200, Thomas Gleixner wrote:
quoted
1) That inline function can be put out of line by the compiler and
placed into the regular text section which makes it subject to
instrumentation
2) That inline function invokes local_irq_save() which is subject to
instrumentation _before_ the entry state for the instrumentation
mechanisms is established.
3) That inline function invokes sync_core() before important state
has
been established, which is especially interesting in NMI like
exceptions.
As you clearly documented why all of the above is safe and does not
cause any problems, it's just me and Peter being silly, right?
Try again.
I don't think, accusations and mockery are really necessary here.
Let's get some context to this.
I told you in my first mail, that this breaks noinstr and that
building with full debug would have told you.
Peter gave you a clear hint where to look.
Now it might be expected that you investigate that or at least ask
questions before making the bold claim:
quoted
quoted
Unless something else is involved, those operations are safe, so I
am not adding anything that can break those.
Surely I could have avoided the snide remark, but after you demonstrably
ignored technically valid concerns and suggestions in your other reply,
I was surely not in the mood to be overly careful in my choice of words.
The result of this may be not a "design" per se, but an understanding
of how things are implemented, and what rules are being followed, so I
could add my code in a manner consistent with what is done, and
document the whole thing.
Every other big and technically complex project which has to change the
very inner workings of the kernel started the same way. I'm not aware of
any of them getting accepted as is or in a big code dump.
What you have now qualifies as proof of concept and the big challenge is
to turn it into something which is acceptable and maintainable.
You talk in great length about how inconsistent stuff is all over the
place. Yes, it is indeed. You even call that inconsistency an existing
design:
My patches reflect what is already in code and in its design.
I agree that you just work with the code as is, but you might have
noticed that quite some of this stuff is clearly not designed at all or
designed badly.
The solution is not to pile on top of the inconsistency, the solution is
to make it consistent in the first place.
You are surely going to say, that's beyond the scope of your project. I
can tell you that it is in the scope of your project simply because just
proliferating the status quo and piling new stuff on top is not an
option. And no, there are no people waiting in a row to mop up after
you either.
Quite some of the big technology projects have spent and still spend
considerable amount of time to do exactly this kind of consolidation
work upfront in order to make their features acceptable in a
maintainable form.
All of these projects have been merged or are still being merged
piecewise in reviewable chunks.
We are talking about intrusive technology which requires a very careful
integration to prevent it from becoming a roadblock or a maintenaince
headache. The approach and implementation has to be _agreed_ on by the
involved parties, i.e. submitters, reviewers and maintainers.
While I understand that this is an unusual feature and by its nature
it affects kernel in multiple places, it does not deserve to be called
a "mess" and other synonyms of "mess".
The feature is perfectly fine and I completely understand why you want
it. Guess who started to lay the grounds for NOHZ_FULL more than a
decade ago and why?
The implementation is not acceptable on technical grounds,
maintainability reasons, lack of design and proper consolidated
integration.
Another issue that you have asked me to defend is the existence and
scope of task isolation itself.
I have not asked you to defend the existance. I asked you for coherent
explanations how the implementation works and why the chosen approach is
correct and valid. That's a completely different thing.
It's an attempt to introduce a feature that turns Linux userspace into
superior replacement of RTOS.....
Can you please spare me the advertising and marketing? I'm very well
aware what an RTOS is and I'm also very well aware that there is no such
thing like a 'superior replacement' for RTOS in general.
If your view of RTOS is limited to this particular feature, then I have
to tell you that this particular feature is only useful for a very small
portion of the overall RTOS use cases.
However most definitely this is not a "mess", and it I do not believe
that I have to defend the validity of this direction of development, or
be accused of general incompetence every time someone finds a
frustrating mistake in my code.
Nobody accuses you of incompetence, but you will have to defend the
validity of your approach and implementation and accept that things
might not be as shiny as you think they are. That's not hostility,
that's just how Linux kernel development works whether you like it or
not.
I surely can understand your frustration over my view of this series,
but you might have noticed that aside of criticism I gave you very clear
technical arguments and suggestions how to proceed.
It's your decision what you make of that.
Thanks,
tglx
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
+/*
+ * Description of the last two tasks that ran isolated on a given CPU.
+ * This is intended only for messages about isolation breaking. We
+ * don't want any references to actual task while accessing this from
+ * CPU that caused isolation breaking -- we know nothing about timing
+ * and don't want to use locking or RCU.
+ */
+struct isol_task_desc {
+ atomic_t curr_index;
+ atomic_t curr_index_wr;
+ bool warned[2];
+ pid_t pid[2];
+ pid_t tgid[2];
+ char comm[2][TASK_COMM_LEN];
+};
+static DEFINE_PER_CPU(struct isol_task_desc, isol_task_descs);
So that's quite a huge patch that would have needed to be split up.
Especially this tracing engine.
Speaking of which, I agree with Thomas that it's unnecessary. It's too much
code and complexity. We can use the existing trace events and perform the
analysis from userspace to find the source of the disturbance.
Thanks.
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
+/**
+ * task_isolation_kernel_enter() - clear low-level task isolation flag
+ *
+ * This should be called immediately after entering kernel.
+ */
+static inline void task_isolation_kernel_enter(void)
+{
+ unsigned long flags;
+
+ /*
+ * This function runs on a CPU that ran isolated task.
+ *
+ * We don't want this CPU running code from the rest of kernel
+ * until other CPUs know that it is no longer isolated.
+ * When CPU is running isolated task until this point anything
+ * that causes an interrupt on this CPU must end up calling this
+ * before touching the rest of kernel. That is, this function or
+ * fast_task_isolation_cpu_cleanup() or stop_isolation() calling
+ * it. If any interrupt, including scheduling timer, arrives, it
+ * will still end up here early after entering kernel.
+ * From this point interrupts are disabled until all CPUs will see
+ * that this CPU is no longer running isolated task.
+ *
+ * See also fast_task_isolation_cpu_cleanup().
+ */
+ smp_rmb();
I'm a bit confused what this read memory barrier is ordering. Also against
what it pairs.
+ if((this_cpu_read(ll_isol_flags) & FLAG_LL_TASK_ISOLATION) == 0)
+ return;
+
+ local_irq_save(flags);
+
+ /* Clear low-level flags */
+ this_cpu_write(ll_isol_flags, 0);
+
+ /*
+ * If something happened that requires a barrier that would
+ * otherwise be called from remote CPUs by CPU kick procedure,
+ * this barrier runs instead of it. After this barrier, CPU
+ * kick procedure would see the updated ll_isol_flags, so it
+ * will run its own IPI to trigger a barrier.
+ */
+ smp_mb();
+ /*
+ * Synchronize instructions -- this CPU was not kicked while
+ * in isolated mode, so it might require synchronization.
+ * There might be an IPI if kick procedure happened and
+ * ll_isol_flags was already updated while it assembled a CPU
+ * mask. However if this did not happen, synchronize everything
+ * here.
+ */
+ instr_sync();
It's the first time I meet an instruction barrier. I should get information
about that but what is it ordering here?
On Wed, Jul 22, 2020 at 02:57:33PM +0000, Alex Belits wrote:
quoted hunk
From: Yuri Norov <redacted>
For nohz_full CPUs the desirable behavior is to receive interrupts
generated by tick_nohz_full_kick_cpu(). But for hard isolation it's
obviously not desirable because it breaks isolation.
This patch adds check for it.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: updated, only exclude CPUs running isolated tasks]
Signed-off-by: Alex Belits <redacted>
---
kernel/time/tick-sched.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
+ if (!tick_nohz_full_cpu(cpu) || task_isolation_on_cpu(cpu))
return;
You can't simply ignore an IPI. There is always a reason for a nohz_full CPU
to be kicked. Something triggered a tick dependency. It can be posix cpu timers
for example, or anything.
On Wed, Jul 22, 2020 at 02:58:24PM +0000, Alex Belits wrote:
From: Yuri Norov <redacted>
If CPU runs isolated task, there's no any backlog on it, and
so we don't need to flush it.
What guarantees that we have no backlog on it?
quoted hunk
Currently flush_all_backlogs()
enqueues corresponding work on all CPUs including ones that run
isolated tasks. It leads to breaking task isolation for nothing.
In this patch, backlog flushing is enqueued only on non-isolated CPUs.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: use safe task_isolation_on_cpu() implementation]
Signed-off-by: Alex Belits <redacted>
---
net/core/dev.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Alex Belits <hidden> Date: 2020-10-04 14:45:44
On Thu, 2020-10-01 at 15:56 +0200, Frederic Weisbecker wrote:
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
quoted
+/*
+ * Description of the last two tasks that ran isolated on a given
CPU.
+ * This is intended only for messages about isolation breaking. We
+ * don't want any references to actual task while accessing this
from
+ * CPU that caused isolation breaking -- we know nothing about
timing
+ * and don't want to use locking or RCU.
+ */
+struct isol_task_desc {
+ atomic_t curr_index;
+ atomic_t curr_index_wr;
+ bool warned[2];
+ pid_t pid[2];
+ pid_t tgid[2];
+ char comm[2][TASK_COMM_LEN];
+};
+static DEFINE_PER_CPU(struct isol_task_desc, isol_task_descs);
So that's quite a huge patch that would have needed to be split up.
Especially this tracing engine.
Speaking of which, I agree with Thomas that it's unnecessary. It's
too much
code and complexity. We can use the existing trace events and perform
the
analysis from userspace to find the source of the disturbance.
The idea behind this is that isolation breaking events are supposed to
be known to the applications while applications run normally, and they
should not require any analysis or human intervention to be handled.
A process may exit isolation because some leftover delayed work, for
example, a timer or a workqueue, is still present on a CPU, or because
a page fault or some other exception, normally handled silently, is
caused by the task. It is also possible to direct an interrupt to a CPU
that is running an isolated task -- currently it's perfectly valid to
set interrupt smp affinity to a CPU running isolated task, and then
interrupt will cause breaking isolation. While it's probably not the
best way of handling interrupts, I would rather not prohibit this
explicitly.
There is also a matter of avoiding race conditions on entering
isolation. Once CPU entered isolation, other CPUs should avoid
disturbing it when they know that CPU is running a task in isolated
mode. However for a short time after entering isolation other CPUs may
be unaware of this, and will still send IPIs to it. Preventing this
scenario completely would be very costly in terms of what other CPUs
will have to do before notifying others, so similar to how EINTR works,
we can simply specify that this is allowed, and task is supposed to re-
enter isolation after this. It's still a bad idea to specify that
isolation breaking can continue happening while application is running
in isolated mode, however allowing some "grace period" after entering
is acceptable as long as application is aware of this happening.
In libtmc I have moved this handling of isolation breaking into a
separate thread, intended to become a separate daemon if necessary. In
part it was done because initial implementation of isolation made it
very difficult to avoid repeating delayed work on isolated CPUs, so
something had to watch for it from non-isolated CPU. It's possible that
now, when delayed work does not appear on isolated CPUs out of nowhere,
the need in isolation manager thread will disappear, and task itself
will be able to handle all isolation breaking, like original
implementation by Chris was supposed to.
However in either case it's still useful for the task, or isolation
manager, to get a description of the isolation-breaking event. This is
what those things are intended for. Now they only produce log messages
because this is where initially all description of isolation-breaking
events went, however I would prefer to make logging optional but always
let applications read those events descriptions, regardless of any
tracing mechanism being used. I was more focused on making the
reporting mechanism properly detect the cause of isolation breaking
because that functionality was not quite working in earlier work by
Chris and Yuri, so I have kept logging as the only output, but made it
suitable for producing events that applications will be able to
receive. Application, or isolation manager, will receive clear and
unambiguous reporting, so there will be no need for any additional
analysis or guesswork.
After adding a proper "low-level" isolation flags, I got the idea that
we might have a better yet reporting mechanism. Early isolation
breaking detection on kernel entry may set a flag that says that
isolation breaking happened, however its cause is unknown. Or, more
likely, only some general information about isolation breaking is
available, like a type of exception. Then, once a known isolation-
breaking reporting mechanism is called from interrupt, syscall, IPI or
exception processing, the flag is cleared, and reporting is supposed to
be done. However if then kernel returns to userspace on isolated task
but isolation breaking is not reported yet, an isolation breaking
reporting with "unknown cause" will happen. We may even add some more
optional lightweight tracing for debugging purposes, however the fact
that reporting will be done, will allow us to make sure that no matter
how complicated exception processing is, or how we managed to miss some
subtle details of an architecture where we are implementing task
isolation, there will be a reliable way to tell the user that something
is wrong, and tell the task that there is something it has to react to.
From: Alex Belits <hidden> Date: 2020-10-04 15:03:07
On Thu, 2020-10-01 at 16:40 +0200, Frederic Weisbecker wrote:
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
quoted
+/**
+ * task_isolation_kernel_enter() - clear low-level task isolation
flag
+ *
+ * This should be called immediately after entering kernel.
+ */
+static inline void task_isolation_kernel_enter(void)
+{
+ unsigned long flags;
+
+ /*
+ * This function runs on a CPU that ran isolated task.
+ *
+ * We don't want this CPU running code from the rest of kernel
+ * until other CPUs know that it is no longer isolated.
+ * When CPU is running isolated task until this point anything
+ * that causes an interrupt on this CPU must end up calling
this
+ * before touching the rest of kernel. That is, this function
or
+ * fast_task_isolation_cpu_cleanup() or stop_isolation()
calling
+ * it. If any interrupt, including scheduling timer, arrives,
it
+ * will still end up here early after entering kernel.
+ * From this point interrupts are disabled until all CPUs will
see
+ * that this CPU is no longer running isolated task.
+ *
+ * See also fast_task_isolation_cpu_cleanup().
+ */
+ smp_rmb();
I'm a bit confused what this read memory barrier is ordering. Also
against
what it pairs.
My bad, I have kept it after there were left no write accesses from
other CPUs.
quoted
+ if((this_cpu_read(ll_isol_flags) & FLAG_LL_TASK_ISOLATION) ==
0)
+ return;
+
+ local_irq_save(flags);
+
+ /* Clear low-level flags */
+ this_cpu_write(ll_isol_flags, 0);
+
+ /*
+ * If something happened that requires a barrier that would
+ * otherwise be called from remote CPUs by CPU kick procedure,
+ * this barrier runs instead of it. After this barrier, CPU
+ * kick procedure would see the updated ll_isol_flags, so it
+ * will run its own IPI to trigger a barrier.
+ */
+ smp_mb();
+ /*
+ * Synchronize instructions -- this CPU was not kicked while
+ * in isolated mode, so it might require synchronization.
+ * There might be an IPI if kick procedure happened and
+ * ll_isol_flags was already updated while it assembled a CPU
+ * mask. However if this did not happen, synchronize everything
+ * here.
+ */
+ instr_sync();
It's the first time I meet an instruction barrier. I should get
information
about that but what is it ordering here?
Against barriers in instruction cache flushing (flush_icache_range()
and such).
From: Alex Belits <hidden> Date: 2020-10-04 15:22:56
On Thu, 2020-10-01 at 16:44 +0200, Frederic Weisbecker wrote:
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:57:33PM +0000, Alex Belits wrote:
quoted
From: Yuri Norov <redacted>
For nohz_full CPUs the desirable behavior is to receive interrupts
generated by tick_nohz_full_kick_cpu(). But for hard isolation it's
obviously not desirable because it breaks isolation.
This patch adds check for it.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: updated, only exclude CPUs running isolated
tasks]
Signed-off-by: Alex Belits <redacted>
---
kernel/time/tick-sched.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
ll_isol_flags will be read in task_isolation_on_cpu(), that accrss
should be ordered against writing in
task_isolation_kernel_enter(), fast_task_isolation_cpu_cleanup()
and task_isolation_start().
Since task_isolation_on_cpu() is often called for multiple CPUs in a
sequence, it would be wasteful to include a barrier inside it.
quoted
+ if (!tick_nohz_full_cpu(cpu) || task_isolation_on_cpu(cpu))
return;
You can't simply ignore an IPI. There is always a reason for a
nohz_full CPU
to be kicked. Something triggered a tick dependency. It can be posix
cpu timers
for example, or anything.
I realize that this is unusual, however the idea is that while the task
is running in isolated mode in userspace, we assume that from this CPUs
point of view whatever is happening in kernel, can wait until CPU is
back in kernel, and when it first enters kernel from this mode, it
should "catch up" with everything that happened in its absence.
task_isolation_kernel_enter() is supposed to do that, so by the time
anything should be done involving the rest of the kernel, CPU is back
to normal.
It is application's responsibility to avoid triggering things that
break its isolation, so the application assumes that everything that
involves entering kernel will not be available while it is isolated. If
isolation will be broken, or application will request return from
isolation, everything will go back to normal environment with all
functionality available.
From: Alex Belits <hidden> Date: 2020-10-04 17:12:59
On Thu, 2020-10-01 at 16:47 +0200, Frederic Weisbecker wrote:
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:58:24PM +0000, Alex Belits wrote:
quoted
From: Yuri Norov <redacted>
If CPU runs isolated task, there's no any backlog on it, and
so we don't need to flush it.
What guarantees that we have no backlog on it?
I believe, the logic was that it is not supposed to have backlog
because it could not be produced while the CPU was in userspace,
because one has to enter kernel to receive (by interrupt) or send (by
syscall) anything.
Now, looking at this patch. I don't think, it can be guaranteed that
there was no backlog before it entered userspace. Then backlog
processing will be delayed until exit from isolation. It won't be
queued, and flush_work() will not wait when no worker is assigned, so
there won't be a deadlock, however this delay may not be such a great
idea.
So it may be better to flush backlog before entering isolation, and in
flush_all_backlogs() instead of skipping all CPUs in isolated mode,
check if their per-CPU softnet_data->input_pkt_queue and softnet_data-
process_queue are empty, and if they are not, call backlog anyway.
Then, if for whatever reason backlog will appear after flushing (we
can't guarantee that nothing preempted us then), it will cause one
isolation breaking event, and if nothing will be queued before re-
entering isolation, there will be no backlog until exiting isolation.
quoted
Currently flush_all_backlogs()
enqueues corresponding work on all CPUs including ones that run
isolated tasks. It leads to breaking task isolation for nothing.
In this patch, backlog flushing is enqueued only on non-isolated
CPUs.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: use safe task_isolation_on_cpu()
implementation]
Signed-off-by: Alex Belits <redacted>
---
net/core/dev.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
On Sun, Oct 04, 2020 at 02:44:39PM +0000, Alex Belits wrote:
On Thu, 2020-10-01 at 15:56 +0200, Frederic Weisbecker wrote:
quoted
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
quoted
+/*
+ * Description of the last two tasks that ran isolated on a given
CPU.
+ * This is intended only for messages about isolation breaking. We
+ * don't want any references to actual task while accessing this
from
+ * CPU that caused isolation breaking -- we know nothing about
timing
+ * and don't want to use locking or RCU.
+ */
+struct isol_task_desc {
+ atomic_t curr_index;
+ atomic_t curr_index_wr;
+ bool warned[2];
+ pid_t pid[2];
+ pid_t tgid[2];
+ char comm[2][TASK_COMM_LEN];
+};
+static DEFINE_PER_CPU(struct isol_task_desc, isol_task_descs);
So that's quite a huge patch that would have needed to be split up.
Especially this tracing engine.
Speaking of which, I agree with Thomas that it's unnecessary. It's
too much
code and complexity. We can use the existing trace events and perform
the
analysis from userspace to find the source of the disturbance.
The idea behind this is that isolation breaking events are supposed to
be known to the applications while applications run normally, and they
should not require any analysis or human intervention to be handled.
Sure but you can use trace events for that. Just trace interrupts, workqueues,
timers, syscalls, exceptions and scheduler events and you get all the local
disturbance. You might want to tune a few filters but that's pretty much it.
As for the source of the disturbances, if you really need that information,
you can trace the workqueue and timer queue events and just filter those that
target your isolated CPUs.
A process may exit isolation because some leftover delayed work, for
example, a timer or a workqueue, is still present on a CPU, or because
a page fault or some other exception, normally handled silently, is
caused by the task. It is also possible to direct an interrupt to a CPU
that is running an isolated task -- currently it's perfectly valid to
set interrupt smp affinity to a CPU running isolated task, and then
interrupt will cause breaking isolation. While it's probably not the
best way of handling interrupts, I would rather not prohibit this
explicitly.
Sure, but you can trace all these events with the existing tracing
interface we have.
There is also a matter of avoiding race conditions on entering
isolation. Once CPU entered isolation, other CPUs should avoid
disturbing it when they know that CPU is running a task in isolated
mode. However for a short time after entering isolation other CPUs may
be unaware of this, and will still send IPIs to it. Preventing this
scenario completely would be very costly in terms of what other CPUs
will have to do before notifying others, so similar to how EINTR works,
we can simply specify that this is allowed, and task is supposed to re-
enter isolation after this. It's still a bad idea to specify that
isolation breaking can continue happening while application is running
in isolated mode, however allowing some "grace period" after entering
is acceptable as long as application is aware of this happening.
Right but that doesn't look related to tracing. Anyway I guess we
can make the CPU enter some specific mode after calling synchronize_rcu().
In libtmc I have moved this handling of isolation breaking into a
separate thread, intended to become a separate daemon if necessary. In
part it was done because initial implementation of isolation made it
very difficult to avoid repeating delayed work on isolated CPUs, so
something had to watch for it from non-isolated CPU. It's possible that
now, when delayed work does not appear on isolated CPUs out of nowhere,
the need in isolation manager thread will disappear, and task itself
will be able to handle all isolation breaking, like original
implementation by Chris was supposed to.
However in either case it's still useful for the task, or isolation
manager, to get a description of the isolation-breaking event. This is
what those things are intended for. Now they only produce log messages
because this is where initially all description of isolation-breaking
events went, however I would prefer to make logging optional but always
let applications read those events descriptions, regardless of any
tracing mechanism being used. I was more focused on making the
reporting mechanism properly detect the cause of isolation breaking
because that functionality was not quite working in earlier work by
Chris and Yuri, so I have kept logging as the only output, but made it
suitable for producing events that applications will be able to
receive. Application, or isolation manager, will receive clear and
unambiguous reporting, so there will be no need for any additional
analysis or guesswork.
That still look like a job for userspace, based on trace events.
Thanks.
On Sun, Oct 04, 2020 at 02:44:39PM +0000, Alex Belits wrote:
quoted
On Thu, 2020-10-01 at 15:56 +0200, Frederic Weisbecker wrote:
quoted
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
quoted
+/*
+ * Description of the last two tasks that ran isolated on a given
CPU.
+ * This is intended only for messages about isolation breaking. We
+ * don't want any references to actual task while accessing this
from
+ * CPU that caused isolation breaking -- we know nothing about
timing
+ * and don't want to use locking or RCU.
+ */
+struct isol_task_desc {
+ atomic_t curr_index;
+ atomic_t curr_index_wr;
+ bool warned[2];
+ pid_t pid[2];
+ pid_t tgid[2];
+ char comm[2][TASK_COMM_LEN];
+};
+static DEFINE_PER_CPU(struct isol_task_desc, isol_task_descs);
So that's quite a huge patch that would have needed to be split up.
Especially this tracing engine.
Speaking of which, I agree with Thomas that it's unnecessary. It's
too much
code and complexity. We can use the existing trace events and perform
the
analysis from userspace to find the source of the disturbance.
The idea behind this is that isolation breaking events are supposed to
be known to the applications while applications run normally, and they
should not require any analysis or human intervention to be handled.
Sure but you can use trace events for that. Just trace interrupts, workqueues,
timers, syscalls, exceptions and scheduler events and you get all the local
disturbance. You might want to tune a few filters but that's pretty much it.
As for the source of the disturbances, if you really need that information,
you can trace the workqueue and timer queue events and just filter those that
target your isolated CPUs.
I agree that we can do all those things with tracing.
However, IMHO having a simplified logging mechanism to gather the source of
violation may help in reducing the manual effort.
Although, I am not sure how easy will it be to maintain such an interface
over time.
--
Thanks
Nitesh
On Mon, Oct 05, 2020 at 02:52:49PM -0400, Nitesh Narayan Lal wrote:
On 10/4/20 7:14 PM, Frederic Weisbecker wrote:
quoted
On Sun, Oct 04, 2020 at 02:44:39PM +0000, Alex Belits wrote:
quoted
On Thu, 2020-10-01 at 15:56 +0200, Frederic Weisbecker wrote:
quoted
External Email
-------------------------------------------------------------------
---
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
quoted
+/*
+ * Description of the last two tasks that ran isolated on a given
CPU.
+ * This is intended only for messages about isolation breaking. We
+ * don't want any references to actual task while accessing this
from
+ * CPU that caused isolation breaking -- we know nothing about
timing
+ * and don't want to use locking or RCU.
+ */
+struct isol_task_desc {
+ atomic_t curr_index;
+ atomic_t curr_index_wr;
+ bool warned[2];
+ pid_t pid[2];
+ pid_t tgid[2];
+ char comm[2][TASK_COMM_LEN];
+};
+static DEFINE_PER_CPU(struct isol_task_desc, isol_task_descs);
So that's quite a huge patch that would have needed to be split up.
Especially this tracing engine.
Speaking of which, I agree with Thomas that it's unnecessary. It's
too much
code and complexity. We can use the existing trace events and perform
the
analysis from userspace to find the source of the disturbance.
The idea behind this is that isolation breaking events are supposed to
be known to the applications while applications run normally, and they
should not require any analysis or human intervention to be handled.
Sure but you can use trace events for that. Just trace interrupts, workqueues,
timers, syscalls, exceptions and scheduler events and you get all the local
disturbance. You might want to tune a few filters but that's pretty much it.
As for the source of the disturbances, if you really need that information,
you can trace the workqueue and timer queue events and just filter those that
target your isolated CPUs.
I agree that we can do all those things with tracing.
However, IMHO having a simplified logging mechanism to gather the source of
violation may help in reducing the manual effort.
Although, I am not sure how easy will it be to maintain such an interface
over time.
The thing is: tracing is your simplified logging mechanism here. You can achieve
the same in userspace with _way_ less code, no race, and you can do it in
bash.
Thanks.
From: Alex Belits <hidden> Date: 2020-10-06 11:02:48
On Mon, 2020-10-05 at 01:14 +0200, Frederic Weisbecker wrote:
Speaking of which, I agree with Thomas that it's unnecessary.
quoted
quoted
It's
too much
code and complexity. We can use the existing trace events and
perform
the
analysis from userspace to find the source of the disturbance.
The idea behind this is that isolation breaking events are supposed
to
be known to the applications while applications run normally, and
they
should not require any analysis or human intervention to be
handled.
Sure but you can use trace events for that. Just trace interrupts,
workqueues,
timers, syscalls, exceptions and scheduler events and you get all the
local
disturbance. You might want to tune a few filters but that's pretty
much it.
And keep all tracing enabled all the time, just to be able to figure
out that disturbance happened at all?
Or do you mean that we can use kernel entry mechanism to reliably
determine that isolation breaking event happened (so the isolation-
breaking procedure can be triggered as early as possible), yet avoid
trying to determine why exactly it happened, and use tracing if we want
to know?
Original patch did the opposite, it triggered any isolation-breaking
procedure only once it was known specifically, what kind of event
happened -- a hardware interrupt, IPI, syscall, page fault, or any
other kind of exception, possibly something architecture-specific.
This, of course, always had a potential problem with coverage -- if
handling of something is missing, isolation breaking is not handled at
all, and there is no obvious way of finding if we covered everything.
This also made the patch large and somewhat ugly.
When I have added a mechanism for low-level isolation breaking handling
on kernel entry, it also partially improved the problem with
completeness. Partially because I have not yet added handling of
"unknown cause" before returning to userspace, however that would be a
logical thing to do. Then if we entered kernel from isolation, did
something, and are returning to userspace still not knowing what kind
of isolation-breaking event happened, we can still trigger isolation
breaking.
Did I get it right, and you mean that we can remove all specific
handling of isolation breaking causes, except for syscall that exits
isolation, and report isolation breaking instead of normally returning
to userspace? Then isolation breaking will be handled reliably without
knowing the cause, and we can leave determining the cause to the
tracing mechanism (if enabled)?
This does make sense. However for me it looks somewhat strange, because
I assume isolation breaking to be a kind of runtime error, that
userspace software is supposed to get some basic information about --
like, signals distinguishing between, say, SIGSEGV and SIGPIPE, or
write() being able to set errno to ENOSPC or EIO. Then userspace
receives basic information about the cause of exception or error, and
can do some meaningful reporting, or decide if the error should be
fatal for the application or handled differently, based on its internal
logic. To get those distinctions, application does not have to be aware
of anything internal to the kernel.
Similarly distinguishing between, say, a page fault, device interrupt
and a timer may be important for a logic implemented in userspace, and
I think, it may be nice to allow userspace to get this information
immediately and without being aware of any additional details of kernel
implementation. The current patch doesn't do this yet, however the
intention is to implement reliable isolation breaking by checking on
userspace re-entry, plus make reporting of causes, if any were found,
visible to the userspace in some convenient way.
The part that determines the cause can be implemented separately from
isolation breaking mechanism. Then we can have isolation breaking on
kernel entry (or potentially some other condition on kernel entry that
requires logging the cause) enable reporting, then reporting mechanism,
if it exists will fill the blanks, and once either cause is known, or
it's time to return to userspace, notification will be done with
whatever information is available. For some in-depth analysis, if
necessary for debugging the kernel, we can have tracing check if we are
in this "suspicious kernel entry" mode, and log things that otherwise
would not be.
As for the source of the disturbances, if you really need that
information,
you can trace the workqueue and timer queue events and just filter
those that
target your isolated CPUs.
For the purpose of human debugging the kernel or application, the more
information is (usually) the better, so the only concern here is that
now user is responsible for completeness of things he is tracing.
However from application's point of view, or for logging in a
production environment it's usually more important to get general type
of events, so it's possible to, say, confirm that nothing "really bad"
happened, or to trigger the emergency response if it did. Say, if the
only causes of isolation breaking was IPI within few moments of
application startup, or signal from somewhere else when application was
restarted, there is no cause for concern. However if hardware
interrupts arrive at random points in time, something is clearly wrong.
And if page faults happen, most likely application forgot to page-in
and lock its address space.
Again, in my opinion this is not unlike reporting ENOSPC vs. EIO while
doing file I/O -- the former (usually) indicates a common problem that
may require application-level cleanup, the latter (also usually) means
that something is seriously wrong.
quoted
A process may exit isolation because some leftover delayed work,
for
example, a timer or a workqueue, is still present on a CPU, or
because
a page fault or some other exception, normally handled silently, is
caused by the task. It is also possible to direct an interrupt to a
CPU
that is running an isolated task -- currently it's perfectly valid
to
set interrupt smp affinity to a CPU running isolated task, and then
interrupt will cause breaking isolation. While it's probably not
the
best way of handling interrupts, I would rather not prohibit this
explicitly.
Sure, but you can trace all these events with the existing tracing
interface we have.
Right. However it would require someone to intentionally do tracing of
all those events, all for the purpose of obtaining a type of runtime
error. As an embedded systems developer, who had to look for signs of
unusual bugs on a large number of customers' systems, and had to
distinguish them from reports of hardware malfunctions, I would prefer
something clearly identifiable in the logs (of kernel, application, or
anything else) when no one is specifically investigating any problem.
When anything suspicious happens, often the system is physically
unreachable, and the problem may or may not happen again, so the first
report from a running system may be the only thing available. When
everything is going well, the same systems more often have hardware
failures than report valid software bugs (or, ideally, all reports are
from hardware failures), so it's much better to know that if software
will do something wrong, it would be possible to identify the problem
from the first report, rather than guess.
Sometimes equipment gets firmware updates many years after production,
when there are reports of all kinds of failures due to mechanical or
thermal damage, faulty parts, bad repair work, deteriorating flash,
etc. Among those there might be something that indicates new bugs made
by a new generation of developers (occasionally literally),
regressions, etc. In those situations getting useful information from
the error message in the first report can make a difference between
quickly identifying the problem and going on a wild goose chase.
--
Alex
ll_isol_flags will be read in task_isolation_on_cpu(), that accrss
should be ordered against writing in
task_isolation_kernel_enter(), fast_task_isolation_cpu_cleanup()
and task_isolation_start().
Since task_isolation_on_cpu() is often called for multiple CPUs in a
sequence, it would be wasteful to include a barrier inside it.
Then I think you meant a full barrier: smp_mb()
quoted
quoted
+ if (!tick_nohz_full_cpu(cpu) || task_isolation_on_cpu(cpu))
return;
You can't simply ignore an IPI. There is always a reason for a
nohz_full CPU
to be kicked. Something triggered a tick dependency. It can be posix
cpu timers
for example, or anything.
I realize that this is unusual, however the idea is that while the task
is running in isolated mode in userspace, we assume that from this CPUs
point of view whatever is happening in kernel, can wait until CPU is
back in kernel and when it first enters kernel from this mode, it
should "catch up" with everything that happened in its absence.
task_isolation_kernel_enter() is supposed to do that, so by the time
anything should be done involving the rest of the kernel, CPU is back
to normal.
You can't assume that. If something needs the tick, this can't wait.
If the user did something wrong, such as setting a posix cpu timer
to an isolated task, that's his fault and the kernel has to stick with
correctness and kick that task out of isolation mode.
It is application's responsibility to avoid triggering things that
break its isolation
Precisely.
so the application assumes that everything that
involves entering kernel will not be available while it is isolated.
We can't do things that way and just ignore IPIs. You need to solve the
source of the noise, not the symptoms.
Thanks.
ll_isol_flags will be read in task_isolation_on_cpu(), that accrss
should be ordered against writing in
task_isolation_kernel_enter(), fast_task_isolation_cpu_cleanup()
and task_isolation_start().
Since task_isolation_on_cpu() is often called for multiple CPUs in
a
sequence, it would be wasteful to include a barrier inside it.
Then I think you meant a full barrier: smp_mb()
For read-only operation? task_isolation_on_cpu() is the only place
where per-cpu ll_isol_flags is accessed, read-only, from multiple CPUs.
All other access to ll_isol_flags is done from the local CPU, and
writes are followed by smp_mb(). There are no other dependencies here,
except operations that depend on the value returned from
task_isolation_on_cpu().
If/when more flags will be added, those rules will be still followed,
because the intention is to store the state of isolation and phases of
entering/breaking/reporting it that can only be updated from the local
CPUs.
quoted
quoted
quoted
+ if (!tick_nohz_full_cpu(cpu) ||
task_isolation_on_cpu(cpu))
return;
You can't simply ignore an IPI. There is always a reason for a
nohz_full CPU
to be kicked. Something triggered a tick dependency. It can be
posix
cpu timers
for example, or anything.
This was added some time ago, when timers appeared and CPUs were kicked
seemingly out of nowhere. At that point breaking posix timers when
running tasks that are not supposed to rely on posix timers, was the
least problematic solution. From user's point of view in this case
entering isolation had an effect on timer similar to task exiting while
the timer is running.
Right now, there are still sources of superfluous calls to this, when
tick_nohz_full_kick_all() is used. If I will be able to confirm that
this is the only problematic place, I would rather fix calls to it, and
make this condition produce a warning.
This gives me an idea that if there will be a mechanism specifically
for reporting kernel entry and isolation breaking, maybe it should be
possible to add a distinction between:
1. isolation breaking that already happened upon kernel entry;
2. performing operation that will immediately and synchronously cause
isolation breaking;
3. operations or conditions that will eventually or asynchronously
cause isolation breaking (having timers running, possibly sending
signals should be in the same category).
This will be (2).
I assume that when reporting of isolation breaking will be separated
from the isolation implementation, it will be implemented as a runtime
error condition reporting mechanism. Then it can be focused on
providing information about category of events and their sources, and
have internal logic designed for that purpose, as opposed to designed
entirely for debugging, providing flexibility and obtaining maximum
details about internals involved.
quoted
I realize that this is unusual, however the idea is that while the
task
is running in isolated mode in userspace, we assume that from this
CPUs
point of view whatever is happening in kernel, can wait until CPU
is
back in kernel and when it first enters kernel from this mode, it
should "catch up" with everything that happened in its absence.
task_isolation_kernel_enter() is supposed to do that, so by the
time
anything should be done involving the rest of the kernel, CPU is
back
to normal.
You can't assume that. If something needs the tick, this can't wait.
If the user did something wrong, such as setting a posix cpu timer
to an isolated task, that's his fault and the kernel has to stick
with
correctness and kick that task out of isolation mode.
That would be true if not multiple "let's just tell all other CPUs that
they should check if they have to update something" situations like the
above.
In case of timers it's possible that I will be able to eliminate all
specific instances when this is done, however I think that as a general
approach we have to establish some distinction between things that must
cause IPI (and break isolation) and things that may be delayed until
the isolated userspace task will allow that or some other unavoidable
isolation-breaking event will happen.
quoted
It is application's responsibility to avoid triggering things that
break its isolation
Precisely.
Right. However there are tings like tick_nohz_full_kick_all() and
similar procedures that result in mass-sending of IPIs without
determining if target CPUs have anything to do with the event at all,
leave alone have to handle it right now, it does not give me an
impression that we can blame application for it. I realize that this is
done for a reason, with the assumption that sending IPIs is "cheaper"
and does not require complex synchronization compared to determining
what and when should be notified, however this is not compatible with
goals of task isolation.
quoted
so the application assumes that everything that
involves entering kernel will not be available while it is
isolated.
We can't do things that way and just ignore IPIs. You need to solve
the
source of the noise, not the symptoms.
It may be that eventually we can completely eliminate those things (at
least when isolation is enabled and this is relevant), however for the
purpose of having usable code without massive changes in numerous
callers, in my opinion, we should acknowledge that some things should
be disabled while the task is isolated, and called on isolation exit --
either unconditionally or conditionally if they were requested while
the task was isolated.
I believe that as long as we create a distinction between "must break
isolation", "delayed until the end of isolation" and "can be safely
ignored if the task is isolated" IPIs, we will end up with less
intrusive changes and reliably working functionality.
Then if we will be able to eliminate the sources of things in the last
two categories, we can treat them as if they were in the first one.
It may be that the timers are already ready to this, and I should just
check what causes tick_nohz_full_kick_all() calls. If so, this
particular check won't be necessary because all calls will happen for a
good reason in situations controlled by application. However as a
general approach I think, we need this longer way with decisions about
delaying or ignoring events.
--
Alex
From: Alex Belits <hidden> Date: 2020-10-17 05:42:03
On Tue, 2020-10-06 at 12:35 +0200, Frederic Weisbecker wrote:
On Mon, Oct 05, 2020 at 02:52:49PM -0400, Nitesh Narayan Lal wrote:
quoted
On 10/4/20 7:14 PM, Frederic Weisbecker wrote:
quoted
On Sun, Oct 04, 2020 at 02:44:39PM +0000, Alex Belits wrote:
quoted
The idea behind this is that isolation breaking events are
supposed to
be known to the applications while applications run normally,
and they
should not require any analysis or human intervention to be
handled.
Sure but you can use trace events for that. Just trace
interrupts, workqueues,
timers, syscalls, exceptions and scheduler events and you get all
the local
disturbance. You might want to tune a few filters but that's
pretty much it.
formation,
quoted
quoted
you can trace the workqueue and timer queue events and just
filter those that
target your isolated CPUs.
I agree that we can do all those things with tracing.
However, IMHO having a simplified logging mechanism to gather the
source of
violation may help in reducing the manual effort.
Although, I am not sure how easy will it be to maintain such an
interface
over time.
The thing is: tracing is your simplified logging mechanism here. You
can achieve
the same in userspace with _way_ less code, no race, and you can do
it in
bash.
The idea is that this mechanism should be usable when no one is there
to run things in bash, or no information about what might happen. It
should be able to report rare events in production when users may not
be able to reproduce them.
--
Alex
From: Alex Belits <hidden> Date: 2020-10-17 05:44:48
On Mon, 2020-10-05 at 14:52 -0400, Nitesh Narayan Lal wrote:
On 10/4/20 7:14 PM, Frederic Weisbecker wrote:
quoted
On Sun, Oct 04, 2020 at 02:44:39PM +0000, Alex Belits wrote:
quoted
On Thu, 2020-10-01 at 15:56 +0200, Frederic Weisbecker wrote:
quoted
External Email
-------------------------------------------------------------
------
---
On Wed, Jul 22, 2020 at 02:49:49PM +0000, Alex Belits wrote:
quoted
+/*
+ * Description of the last two tasks that ran isolated on a
given
CPU.
+ * This is intended only for messages about isolation
breaking. We
+ * don't want any references to actual task while accessing
this
from
+ * CPU that caused isolation breaking -- we know nothing
about
timing
+ * and don't want to use locking or RCU.
+ */
+struct isol_task_desc {
+ atomic_t curr_index;
+ atomic_t curr_index_wr;
+ bool warned[2];
+ pid_t pid[2];
+ pid_t tgid[2];
+ char comm[2][TASK_COMM_LEN];
+};
+static DEFINE_PER_CPU(struct isol_task_desc,
isol_task_descs);
So that's quite a huge patch that would have needed to be split
up.
Especially this tracing engine.
Speaking of which, I agree with Thomas that it's unnecessary.
It's
too much
code and complexity. We can use the existing trace events and
perform
the
analysis from userspace to find the source of the disturbance.
The idea behind this is that isolation breaking events are
supposed to
be known to the applications while applications run normally, and
they
should not require any analysis or human intervention to be
handled.
Sure but you can use trace events for that. Just trace interrupts,
workqueues,
timers, syscalls, exceptions and scheduler events and you get all
the local
disturbance. You might want to tune a few filters but that's pretty
much it.
As for the source of the disturbances, if you really need that
information,
you can trace the workqueue and timer queue events and just filter
those that
target your isolated CPUs.
I agree that we can do all those things with tracing.
However, IMHO having a simplified logging mechanism to gather the
source of
violation may help in reducing the manual effort.
Although, I am not sure how easy will it be to maintain such an
interface
over time.
I think that the goal of "finding source of disturbance" interface is
different from what can be accomplished by tracing in two ways:
1. "Source of disturbance" should provide some useful information about
category of event and it cause as opposed to determining all precise
details about things being called that resulted or could result in
disturbance. It should not depend on the user's knowledge about details
of implementations, it should provide some definite answer of what
happened (with whatever amount of details can be given in a generic
mechanism) even if the user has no idea how those things happen and
what part of kernel is responsible for either causing or processing
them. Then if the user needs further details, they can be obtained with
tracing.
2. It should be usable as a runtime error handling mechanism, so the
information it provides should be suitable for application use and
logging. It should be usable when applications are running on a system
in production, and no specific tracing or monitoring mechanism can be
in use. If, say, thousands of devices are controlling neutrino
detectors on an ocean floor, and in a month of work one of them got one
isolation breaking event, it should be able to report that isolation
was broken by an interrupt from a network interface, so the users will
be able to track it down to some userspace application reconfiguring
those interrupts.
It will be a good idea to make such mechanism optional and suitable for
tracking things on conditions other than "always enabled" and "enabled
with task isolation". However in my opinion, there should be something
in kernel entry procedure that, if enabled, prepared something to be
filled by the cause data, and we know at least one such situation when
this kernel entry procedure should be triggered -- when task isolation
is on.
--
Alex
From: Thomas Gleixner <hidden> Date: 2020-10-17 16:08:24
On Sat, Oct 17 2020 at 01:08, Alex Belits wrote:
On Mon, 2020-10-05 at 14:52 -0400, Nitesh Narayan Lal wrote:
quoted
On 10/4/20 7:14 PM, Frederic Weisbecker wrote:
I think that the goal of "finding source of disturbance" interface is
different from what can be accomplished by tracing in two ways:
1. "Source of disturbance" should provide some useful information about
category of event and it cause as opposed to determining all precise
details about things being called that resulted or could result in
disturbance. It should not depend on the user's knowledge about
details
Tracepoints already give you selectively useful information.
of implementations, it should provide some definite answer of what
happened (with whatever amount of details can be given in a generic
mechanism) even if the user has no idea how those things happen and
what part of kernel is responsible for either causing or processing
them. Then if the user needs further details, they can be obtained with
tracing.
It's just a matter of defining the tracepoint at the right place.
2. It should be usable as a runtime error handling mechanism, so the
information it provides should be suitable for application use and
logging. It should be usable when applications are running on a system
in production, and no specific tracing or monitoring mechanism can be
in use.
That's a strawman really. There is absolutely no reason why a specific
set of tracepoints cannot be enabled on a production system.
Your tracker is a monitoring mechanism, just a different flavour. By
your logic above it cannot be enabled on a production system either.
Also you can enable tracepoints from a control application, consume, log
and act upon them. It's not any different from opening some magic
isolation tracker interface. There are even multiple ways to do that
including libraries.
If, say, thousands of devices are controlling neutrino detectors on an
ocean floor, and in a month of work one of them got one isolation
breaking event, it should be able to report that isolation was broken
by an interrupt from a network interface, so the users will be able to
track it down to some userspace application reconfiguring those
interrupts.
Tracing can do that and it can do it selectively on the isolated
CPUs. It's just a matter of proper configuration and usage.
It will be a good idea to make such mechanism optional and suitable for
tracking things on conditions other than "always enabled" and "enabled
with task isolation".
Tracing already provides that. Tracepoints are individually controlled
and filtered.
However in my opinion, there should be something in kernel entry
procedure that, if enabled, prepared something to be filled by the
cause data, and we know at least one such situation when this kernel
entry procedure should be triggered -- when task isolation is on.
A tracepoint will gather that information for you.
task isolation is not special, it's just yet another way to configure
and use a system and tracepoints provide everything you need with the
bonus that you can gather more correlated information when you need it.
In fact tracing and tracepoints have replaced all specialized trackers
which were in the kernel before tracing was available. We're not going
to add a new one just because.
If there is anything which you find that tracing and tracepoints cannot
provide then the obvious solution is to extend that infrastructure so it
can serve your usecase.
Thanks,
tglx
From: Alex Belits <hidden> Date: 2020-10-17 16:15:54
On Sat, 2020-10-17 at 18:08 +0200, Thomas Gleixner wrote:
On Sat, Oct 17 2020 at 01:08, Alex Belits wrote:
quoted
On Mon, 2020-10-05 at 14:52 -0400, Nitesh Narayan Lal wrote:
quoted
On 10/4/20 7:14 PM, Frederic Weisbecker wrote:
I think that the goal of "finding source of disturbance" interface
is
different from what can be accomplished by tracing in two ways:
1. "Source of disturbance" should provide some useful information
about
category of event and it cause as opposed to determining all
precise
details about things being called that resulted or could result in
disturbance. It should not depend on the user's knowledge about
details
Tracepoints already give you selectively useful information.
Carefully placed tracepoints also can give the user information about
failures of open(), write(), execve() or mmap(). However syscalls still
provide an error code instead of returning generic failure and letting
user debug the cause.
--
Alex
From: Thomas Gleixner <hidden> Date: 2020-10-17 20:03:56
On Sat, Oct 17 2020 at 16:15, Alex Belits wrote:
On Sat, 2020-10-17 at 18:08 +0200, Thomas Gleixner wrote:
quoted
On Sat, Oct 17 2020 at 01:08, Alex Belits wrote:
quoted
I think that the goal of "finding source of disturbance" interface
is
different from what can be accomplished by tracing in two ways:
1. "Source of disturbance" should provide some useful information
about
category of event and it cause as opposed to determining all
precise
details about things being called that resulted or could result in
disturbance. It should not depend on the user's knowledge about
details
Tracepoints already give you selectively useful information.
Carefully placed tracepoints also can give the user information about
failures of open(), write(), execve() or mmap(). However syscalls still
provide an error code instead of returning generic failure and letting
user debug the cause.
I have absolutely no idea what you are trying to tell me.
Thanks,
tglx
On Thu, Oct 01, 2020 at 04:47:31PM +0200, Frederic Weisbecker wrote:
On Wed, Jul 22, 2020 at 02:58:24PM +0000, Alex Belits wrote:
quoted
From: Yuri Norov <redacted>
quoted
so we don't need to flush it.
What guarantees that we have no backlog on it?
From Paolo's work to use lockless reading of
per-CPU skb lists
https://www.spinics.net/lists/netdev/msg682693.html
It also exposed skb queue length to userspace
https://www.spinics.net/lists/netdev/msg684939.html
But if i remember correctly waiting for a RCU grace
period was also necessary to ensure no backlog !?!
Paolo would you please remind us what was the sequence of steps?
(and then also, for the userspace isolation interface, where
the application informs the kernel that its entering isolated
mode, is just confirming the queues have zero length is
sufficient?).
TIA!
quoted
Currently flush_all_backlogs()
enqueues corresponding work on all CPUs including ones that run
isolated tasks. It leads to breaking task isolation for nothing.
In this patch, backlog flushing is enqueued only on non-isolated CPUs.
Signed-off-by: Yuri Norov <redacted>
[abelits@marvell.com: use safe task_isolation_on_cpu() implementation]
Signed-off-by: Alex Belits <redacted>
---
net/core/dev.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Paolo Abeni <pabeni@redhat.com> Date: 2021-01-22 16:22:19
On Fri, 2021-01-22 at 11:13 -0300, Marcelo Tosatti wrote:
On Thu, Oct 01, 2020 at 04:47:31PM +0200, Frederic Weisbecker wrote:
quoted
On Wed, Jul 22, 2020 at 02:58:24PM +0000, Alex Belits wrote:
quoted
From: Yuri Norov <redacted>
so we don't need to flush it.
What guarantees that we have no backlog on it?
From Paolo's work to use lockless reading of
per-CPU skb lists
https://www.spinics.net/lists/netdev/msg682693.html
It also exposed skb queue length to userspace
https://www.spinics.net/lists/netdev/msg684939.html
But if i remember correctly waiting for a RCU grace
period was also necessary to ensure no backlog !?!
Paolo would you please remind us what was the sequence of steps?
(and then also, for the userspace isolation interface, where
the application informs the kernel that its entering isolated
mode, is just confirming the queues have zero length is
sufficient?).
After commit 2de79ee27fdb52626ac4ac48ec6d8d52ba6f9047, for CONFIG_RPS
enabled build, with no RFS in place to ensure backlog will be empty on
CPU X, the user must:
- configure the RPS map on each device before the device goes up to
explicitly exclude CPU X.
If CPU X is isolated after some network device already went up, to
ensure that the backlog will be empty on CPU X the user must:
- configure RPS on all the network device to exclude CPU X (as in the
previous scenario)
- wait a RCU grace period
- wait untill the backlog len on CPU X reported by procfs is 0
Cheers,
Paolo