Date:   Wed, 24 Nov 2021 15:42:14 +0800
From:   Aili Yao <yaoaili126@gmail.com>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        bsegall@google.com, mgorman@suse.de, linux-kernel@vger.kernel.org,
        yaoaili@kingsoft.com
Subject: Re: [PATCH] sched/isolation: delete redundant
 housekeeping_overridden check
Message-ID: <20211124154214.43972a31@gmail.com>
In-Reply-To: <20211123204200.0976e065@rorschach.local.home>
References: <20211123154535.48be4399@gmail.com>
        <20211123123852.11a84a9e@gandalf.local.home>
        <20211124092103.64e93376@gmail.com>
        <20211123204200.0976e065@rorschach.local.home>
Organization: ksyun
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


> The static branch is a jump and a nop, and the two conditions are not
> the same. So nothing above is redundant.
>=20
> 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").
>=20
> 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().
>=20
> 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.
>=20
> The only thing your patch does is remove the optimization of the static
> branch logic.
>=20
Great thanks for your detailed explanation, and in some way, I get your poi=
nts
and partially understand the static branch's necessary, Thanks!

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

=46rom 40c13fad29b22242df6ff5ac97bddafd59e04f3e Mon Sep 17 00:00:00 2001
From: Aili Yao <yaoaili@kingsoft.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@kingsoft.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/isolatio=
n.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
=20
 static inline int housekeeping_any_cpu(enum hk_flags flags)
@@ -46,15 +45,11 @@ static inline bool housekeeping_enabled(enum hk_flags f=
lags)
 static inline void housekeeping_affine(struct task_struct *t,
 				       enum hk_flags flags) { }
 static inline void housekeeping_init(void) { }
-#endif /* CONFIG_CPU_ISOLATION */
=20
 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 */
=20
 #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);
=20
-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);
=20
 void __init housekeeping_init(void)
 {
--=20
2.27.0