linux-kernel - Re: [PATCH] sched/isolation: delete redundant housekeeping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Nov 2021 15:42:14 +0800
From:   Aili Yao <yaoaili126@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        bsegall@...gle.com, mgorman@...e.de, linux-kernel@...r.kernel.org,
        yaoaili@...gsoft.com
Subject: Re: [PATCH] sched/isolation: delete redundant
 housekeeping_overridden check


> The static branch is a jump and a nop, and the two conditions are not
> the same. So nothing above is redundant.
> 
> The static_branch_unlikely(&housekeeping_overridden) is a switch that
> is either a nop which being an unlikely, is the fast path, and the
> content of the if block is the out-of-band condition. That is, the
> static branch keeps the expensive if conditional from ever being tested
> (because it is "overridden").
> 
> Now when it's not overridden, that static branch turns into a jump to
> the flags test. Which then performs the expensive conditional compare
> against flags to see if it should do the cpumask_test_cpu().
> 
> I state "expensive" because compared to a jmp or nop, any branch based
> on a test causes cache speculation to be executed. Which means branch
> prediction, etc.  The jmp and nop are just like any other atomic
> instruction that goes through the pipeline and is considered 100%
> predictable, hence it doesn't need the extra logic in the CPU to figure
> it out.
> 
> The only thing your patch does is remove the optimization of the static
> branch logic.
> 
Great thanks for your detailed explanation, and in some way, I get your points
and partially understand the static branch's necessary, Thanks!

I follow you explanations and dig more into the code, and i have another RFC option,
And I want it shared and reviewed:

From 40c13fad29b22242df6ff5ac97bddafd59e04f3e Mon Sep 17 00:00:00 2001
From: Aili Yao <yaoaili@...gsoft.com>
Date: Wed, 24 Nov 2021 02:15:28 -0500
Subject: [PATCH] sched/isolation: little optimization for housekeeping_cpu

the housekeeping_test_cpu resides in isolation.c and is not a inline
function; For general housekeeping_cpu interface, we seems want to unify
the inline definition, so housekeeping_cpu call housekeeping_test_cpu
when CONFIG_CPU_ISOLATION, while this public inline function still will
call non-static housekeeping_test_cpu; this seems not nessary;

For different CPU_ISOLATION configuration, we may use different inline
property, this will gain a little optimization;

Signed-off-by: Aili Yao <yaoaili@...gsoft.com>
---
 include/linux/sched/isolation.h | 9 ++-------
 kernel/sched/isolation.c        | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/include/linux/sched/isolation.h b/include/linux/sched/isolation.h
index cc9f393e2a70..3ccc19e52186 100644
--- a/include/linux/sched/isolation.h
+++ b/include/linux/sched/isolation.h
@@ -23,9 +23,8 @@ extern int housekeeping_any_cpu(enum hk_flags flags);
 extern const struct cpumask *housekeeping_cpumask(enum hk_flags flags);
 extern bool housekeeping_enabled(enum hk_flags flags);
 extern void housekeeping_affine(struct task_struct *t, enum hk_flags flags);
-extern bool housekeeping_test_cpu(int cpu, enum hk_flags flags);
+extern bool housekeeping_cpu(int cpu, enum hk_flags flags);
 extern void __init housekeeping_init(void);
-
 #else
 
 static inline int housekeeping_any_cpu(enum hk_flags flags)
@@ -46,15 +45,11 @@ static inline bool housekeeping_enabled(enum hk_flags flags)
 static inline void housekeeping_affine(struct task_struct *t,
 				       enum hk_flags flags) { }
 static inline void housekeeping_init(void) { }
-#endif /* CONFIG_CPU_ISOLATION */
 
 static inline bool housekeeping_cpu(int cpu, enum hk_flags flags)
 {
-#ifdef CONFIG_CPU_ISOLATION
-	if (static_branch_unlikely(&housekeeping_overridden))
-		return housekeeping_test_cpu(cpu, flags);
-#endif
 	return true;
 }
+#endif /* CONFIG_CPU_ISOLATION */
 
 #endif /* _LINUX_SCHED_ISOLATION_H */
diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 7f06eaf12818..b5e81df4b04a 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -54,14 +54,14 @@ void housekeeping_affine(struct task_struct *t, enum hk_flags flags)
 }
 EXPORT_SYMBOL_GPL(housekeeping_affine);
 
-bool housekeeping_test_cpu(int cpu, enum hk_flags flags)
+bool housekeeping_cpu(int cpu, enum hk_flags flags)
 {
 	if (static_branch_unlikely(&housekeeping_overridden))
 		if (housekeeping_flags & flags)
 			return cpumask_test_cpu(cpu, housekeeping_mask);
 	return true;
 }
-EXPORT_SYMBOL_GPL(housekeeping_test_cpu);
+EXPORT_SYMBOL_GPL(housekeeping_cpu);
 
 void __init housekeeping_init(void)
 {
-- 
2.27.0