Re: [PATCH v5 4/9] task_isolation: Add task isolation hooks to arch-independent code
From: Thomas Gleixner <hidden>
Date: 2020-11-23 22:31:36
Also in:
linux-arch, linux-arm-kernel, lkml, netdev
Alex, On Mon, Nov 23 2020 at 17:57, Alex Belits wrote:
Kernel entry and exit functions for task isolation are added to context tracking and common entry points. Common handling of pending work on exit to userspace now processes isolation breaking, cleanup and start.
Again: You fail to explain the rationale and just explain what the patch is doing. I can see what the patch is doing from the patch itself.
--- include/linux/hardirq.h | 2 ++ include/linux/sched.h | 2 ++ kernel/context_tracking.c | 5 +++++ kernel/entry/common.c | 10 +++++++++- kernel/irq/irqdesc.c | 5 +++++
At least 3 different subsystems, which means this again failed to be split into seperate patches.
quoted hunk ↗ jump to hunk
extern void synchronize_irq(unsigned int irq);@@ -115,6 +116,7 @@ extern void rcu_nmi_exit(void); do { \ lockdep_off(); \ arch_nmi_enter(); \ + task_isolation_kernel_enter(); \
Where is the explanation why this is safe and correct vs. this fragile code path?
quoted hunk ↗ jump to hunk
@@ -1762,6 +1763,7 @@ extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk); #ifdef CONFIG_SMP static __always_inline void scheduler_ipi(void) { + task_isolation_kernel_enter();
Why is the scheduler_ipi() special? Just because everything else cannot happen at all? Oh well...
quoted hunk ↗ jump to hunk
#define CREATE_TRACE_POINTS #include <trace/events/context_tracking.h>@@ -100,6 +101,8 @@ void noinstr __context_tracking_enter(enum ctx_state state) __this_cpu_write(context_tracking.state, state); } context_tracking_recursion_exit(); + + task_isolation_exit_to_user_mode();
Why is this here at all and why is it outside of the recursion protection
quoted hunk ↗ jump to hunk
} EXPORT_SYMBOL_GPL(__context_tracking_enter);@@ -148,6 +151,8 @@ void noinstr __context_tracking_exit(enum ctx_state state) if (!context_tracking_recursion_enter()) return; + task_isolation_kernel_enter();
while this is inside? And why has the scheduler_ipi() on x86 call this twice? Just because?
quoted hunk ↗ jump to hunk
if (__this_cpu_read(context_tracking.state) == state) { if (__this_cpu_read(context_tracking.active)) { /*diff --git a/kernel/entry/common.c b/kernel/entry/common.c static void exit_to_user_mode_prepare(struct pt_regs *regs) { - unsigned long ti_work = READ_ONCE(current_thread_info()->flags); + unsigned long ti_work; lockdep_assert_irqs_disabled(); + task_isolation_before_pending_work_check(); + + ti_work = READ_ONCE(current_thread_info()->flags); + if (unlikely(ti_work & EXIT_TO_USER_MODE_WORK)) ti_work = exit_to_user_mode_loop(regs, ti_work); + if (unlikely(ti_work & _TIF_TASK_ISOLATION)) + task_isolation_start();
Where is the explaination of this change?
Aside of that how does anything of this compile on x86 at all?
Answer: It does not ...
Stop this frenzy right now. It's going nowhere and all you achieve is to
make people more grumpy than they are already.
Thanks,
tglx