Re: [PATCH v9 04/13] task_isolation: add initial support
From: Frederic Weisbecker <hidden>
Date: 2016-01-19 15:42:27
Also in:
lkml
On Mon, Jan 04, 2016 at 02:34:42PM -0500, Chris Metcalf wrote:
quoted hunk ↗ jump to hunk
diff --git a/kernel/isolation.c b/kernel/isolation.c new file mode 100644 index 000000000000..68a9f7457bc0 --- /dev/null +++ b/kernel/isolation.c@@ -0,0 +1,105 @@ +/* + * linux/kernel/isolation.c + * + * Implementation for task isolation. + * + * Distributed under GPLv2. + */ + +#include <linux/mm.h> +#include <linux/swap.h> +#include <linux/vmstat.h> +#include <linux/isolation.h> +#include <linux/syscalls.h> +#include "time/tick-sched.h" + +cpumask_var_t task_isolation_map; + +/* + * Isolation requires both nohz and isolcpus support from the scheduler. + * We provide a boot flag that enables both for now, and which we can + * add other functionality to over time if needed. Note that just + * specifying "nohz_full=... isolcpus=..." does not enable task isolation. + */ +static int __init task_isolation_setup(char *str) +{ + alloc_bootmem_cpumask_var(&task_isolation_map); + if (cpulist_parse(str, task_isolation_map) < 0) { + pr_warn("task_isolation: Incorrect cpumask '%s'\n", str); + return 1; + } + + alloc_bootmem_cpumask_var(&cpu_isolated_map); + cpumask_copy(cpu_isolated_map, task_isolation_map); + + alloc_bootmem_cpumask_var(&tick_nohz_full_mask); + cpumask_copy(tick_nohz_full_mask, task_isolation_map); + tick_nohz_full_running = true;
How about calling tick_nohz_full_setup() instead? I'd rather prefer that nohz full implementation details stay in tick-sched.c Also what happens if nohz_full= is given as well as task_isolation= ? Don't we risk a memory leak and maybe breaking the fact that (nohz_full & task_isolation != task_isolation) which is really a requirement?
+
+ return 1;
+}
+__setup("task_isolation=", task_isolation_setup);
+
+/*
+ * This routine controls whether we can enable task-isolation mode.
+ * The task must be affinitized to a single task_isolation core or we will
+ * return EINVAL. Although the application could later re-affinitize
+ * to a housekeeping core and lose task isolation semantics, this
+ * initial test should catch 99% of bugs with task placement prior to
+ * enabling task isolation.
+ */
+int task_isolation_set(unsigned int flags)
+{
+ if (cpumask_weight(tsk_cpus_allowed(current)) != 1 ||
+ !task_isolation_possible(smp_processor_id()))
+ return -EINVAL;
+
+ current->task_isolation_flags = flags;
+ return 0;
+}What if we concurrently change the task's affinity? Also it seems that preemption isn't disabled, so we can also migrate concurrently. I'm surprised you haven't seen warnings with smp_processor_id(). Also we should protect against task's affinity change when task_isolation_flags is set.
+
+/*
+ * In task isolation mode we try to return to userspace only after
+ * attempting to make sure we won't be interrupted again. To handle
+ * the periodic scheduler tick, we test to make sure that the tick is
+ * stopped, and if it isn't yet, we request a reschedule so that if
+ * another task needs to run to completion first, it can do so.
+ * Similarly, if any other subsystems require quiescing, we will need
+ * to do that before we return to userspace.
+ */
+bool _task_isolation_ready(void)
+{
+ WARN_ON_ONCE(!irqs_disabled());
+
+ /* If we need to drain the LRU cache, we're not ready. */
+ if (lru_add_drain_needed(smp_processor_id()))
+ return false;
+
+ /* If vmstats need updating, we're not ready. */
+ if (!vmstat_idle())
+ return false;
+
+ /* Request rescheduling unless we are in full dynticks mode. */
+ if (!tick_nohz_tick_stopped()) {
+ set_tsk_need_resched(current);I'm not sure doing this will help getting the tick to get stopped.
+ return false; + } + + return true; +}
Thanks!