Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 19 Jul 2018 23:22:00 +0300
From:   Yury Norov <ynorov@caviumnetworks.com>
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     Frederic Weisbecker <fweisbec@gmail.com>,
        Ingo Molnar <mingo@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Goutham, Sunil" <Sunil.Goutham@cavium.com>,
        Chris Metcalf <cmetcalf@mellanox.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] nohz: don't kick non-idle CPUs in
 tick_nohz_full_kick_cpu()
Message-ID: <20180719202200.GA17106@yury-thinkpad>
References: <20180712181922.23073-1-ynorov@caviumnetworks.com>
 <20180716153109.GA29270@lerouge>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180716153109.GA29270@lerouge>
User-Agent: Mutt/1.9.4 (2018-02-28)
Received-SPF: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jul 2018 20:22:12.5701 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e5c96a9-082d-4a1b-eb9f-08d5edb5505b
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR07MB4372
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Jul 16, 2018 at 05:31:10PM +0200, Frederic Weisbecker wrote:
> External Email
> 
> On Thu, Jul 12, 2018 at 09:19:22PM +0300, Yury Norov wrote:
> > IIUC, tick_nohz_full_kick_cpu() is intended to wakeup idle CPUs
> > that will not be poked by scheduler because they are actually
> > nohz_full.
> 
> Not exactly. It is intended to trigger an interrupt on a nohz_full
> CPU that may be running in userspace without any tick. The irq_exit()
> code let us reprogramm the tick with the latest dependency updates.
> 
> >
> > But in fact this function kicks all CPUs listed in tick_nohz_full_mask,
> > namely:
> >  - idle CPUs;
> >  - CPUs runnung normal tasks;
> >  - CPUs running isolated tasks [1];
> >
> > For normal tasks it introduces unneeded latency, and for isolated tasks
> > it's fatal because isolation gets broken and task receives SIGKILL.
> 
> So this patch applies on Chris series right?

This patch may be applied on master. That's why I sent it to you.

> For now there is no such
> distinction between normal and isolated tasks. Any task running in a
> nohz_full CPU is considered to be isolated.
>
> > The patch below makes tick_nohz_full_kick_cpu() kicking only idle CPUs.
> > Non-idle nohz_full CPUs will observe changed system settings just like
> > non-idle normal (i.e. not nohz_full) CPUs, at next reschedule.
> 
> That's not exactly what we want. In fact when a task runs in a nohz_full CPU,
> it may not meet any reschedule interrupt for a long while. This is why we have
> tick_nohz_full_kick_cpu() in order to force a nohz_full CPU to see the latest
> changes.

OK, got it.

So if my understanding correct, there is 'soft isolation' which is
nohz_full, and 'hard isolation' which is Chris' task_isonation feature. For
soft isolation, the desirable behavior is to receive interrupts generated 
by tick_nohz_full_kick_cpu(), and for hard isolation it's obviously not
desirable because it kills application. 

The patch below adds check against task isolation in tick_nohz_full_kick_cpu().
It is on top of Chris' series. Is it OK from nohz point of view?

---

While here. I just wonder, on my system IRQs are sent to nohz_full CPUs
at every incoming ssh connection. The trace is like this:
[  206.835533] Call trace:
[  206.848411] [<ffff00000889f984>] dump_stack+0x84/0xa8
[  206.853455] [<ffff0000081ea308>] _task_isolation_remote+0x130/0x140
[  206.859714] [<ffff0000081bf5ec>] irq_work_queue_on+0xcc/0xfc
[  206.865365] [<ffff0000081478ac>] tick_nohz_full_kick_cpu+0x88/0x94
[  206.871536] [<ffff000008147930>] tick_nohz_dep_set_all+0x78/0xa8
[  206.877533] [<ffff000008147b58>] tick_nohz_dep_set_signal+0x28/0x34
[  206.883792] [<ffff0000081421fc>] set_process_cpu_timer+0xd0/0x128
[  206.889876] [<ffff0000081422ac>] update_rlimit_cpu+0x58/0x7c
[  206.895528] [<ffff0000083aa3d0>] selinux_bprm_committing_creds+0x180/0x1fc
[  206.902394] [<ffff00000839e394>] security_bprm_committing_creds+0x40/0x5c
[  206.909173] [<ffff00000828c4a0>] install_exec_creds+0x20/0x6c
[  206.914911] [<ffff0000082e15b0>] load_elf_binary+0x368/0xbb8
[  206.920561] [<ffff00000828d09c>] search_binary_handler+0xb8/0x224
[  206.926645] [<ffff00000828d99c>] do_execveat_common+0x44c/0x5f0
[  206.932555] [<ffff00000828db78>] do_execve+0x38/0x44
[  206.937510] [<ffff00000828dd74>] SyS_execve+0x34/0x44

I suspect that scp, ssh tunneling and similar network activities will source 
ticks on nohz_full CPUs as well. On high-loaded server it may generate
significant interrupt traffic on nohz_full CPUs. Is it desirable behavior?

---
Yury

From 9be3c9996c06319a8070ac182291d08acfdc588d Mon Sep 17 00:00:00 2001
From: Yury Norov <ynorov@caviumnetworks.com>
Date: Tue, 17 Jul 2018 12:40:49 +0300
Subject: [PATCH] task_isolation: don't kick isolated CPUs with
 tick_nohz_full_kick_cpu()
To: Chris Metcalf <cmetcalf@mellanox.com>, 
        Frederic Weisbecker <frederic@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Goutham, Sunil" <Sunil.Goutham@cavium.com>,
	linux-kernel@vger.kernel.org

On top of Chris Metcalf series:
https://lkml.org/lkml/2017/11/3/589

tick_nohz_full_kick_cpu() currently interrupts CPUs that may run isolated
task. It's not desirable because that kick will kill isolated application.

The patch below adds check against task isolation in
tick_nohz_full_kick_cpu() to prevent breaking the isolation.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 include/linux/isolation.h | 7 +++++++
 kernel/isolation.c        | 6 ------
 kernel/time/tick-sched.c  | 5 +++--
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/linux/isolation.h b/include/linux/isolation.h
index b7f0a9085b13..fad606cdcd5e 100644
--- a/include/linux/isolation.h
+++ b/include/linux/isolation.h
@@ -158,6 +158,12 @@ static inline void task_isolation_user_exit(void)
 #endif
 }
 
+static inline bool is_isolation_cpu(int cpu)
+{
+	return task_isolation_map != NULL &&
+		cpumask_test_cpu(cpu, task_isolation_map);
+}
+
 #else /* !CONFIG_TASK_ISOLATION */
 static inline int task_isolation_request(unsigned int flags) { return -EINVAL; }
 static inline void task_isolation_start(void) { }
@@ -172,6 +178,7 @@ static inline void task_isolation_remote_cpumask_interrupt(
 	const struct cpumask *mask, const char *fmt, ...) { }
 static inline void task_isolation_signal(struct task_struct *task) { }
 static inline void task_isolation_user_exit(void) { }
+static inline bool is_isolation_cpu(int cpu) { return 0; }
 #endif
 
 #endif /* _LINUX_ISOLATION_H */
diff --git a/kernel/isolation.c b/kernel/isolation.c
index 1e39a1493e76..05db247924ef 100644
--- a/kernel/isolation.c
+++ b/kernel/isolation.c
@@ -41,12 +41,6 @@ static int __init task_isolation_init(void)
 }
 core_initcall(task_isolation_init)
 
-static inline bool is_isolation_cpu(int cpu)
-{
-	return task_isolation_map != NULL &&
-		cpumask_test_cpu(cpu, task_isolation_map);
-}
-
 /* Enable stack backtraces of any interrupts of task_isolation cores. */
 static bool task_isolation_debug;
 static int __init task_isolation_debug_func(char *str)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index c026145eba2f..91928a6afd81 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -23,6 +23,7 @@
 #include <linux/sched/clock.h>
 #include <linux/sched/stat.h>
 #include <linux/sched/nohz.h>
+#include <linux/isolation.h>
 #include <linux/module.h>
 #include <linux/irq_work.h>
 #include <linux/posix-timers.h>
@@ -242,12 +243,12 @@ static void tick_nohz_full_kick(void)
 }
 
 /*
- * Kick the CPU if it's full dynticks in order to force it to
+ * Kick the CPU if it's full dynticks and not isolated in order to force it to
  * re-evaluate its dependency on the tick and restart it if necessary.
  */
 void tick_nohz_full_kick_cpu(int cpu)
 {
-	if (!tick_nohz_full_cpu(cpu))
+	if (!tick_nohz_full_cpu(cpu) || is_isolation_cpu(cpu))
 		return;
 
 	irq_work_queue_on(&per_cpu(nohz_full_kick_work, cpu), cpu);
-- 
2.17.1