From: Aili Yao <[email protected]>
housekeeping_test_cpu is only called by housekeeping_cpu(),
and in housekeeping_cpu(), there is already one same check;
So delete the redundant check.
Signed-off-by: Aili Yao <[email protected]>
---
kernel/sched/isolation.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 7f06eaf..5c4d533 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -56,9 +56,8 @@ void housekeeping_affine(struct task_struct *t, enum
hk_flags flags)
bool housekeeping_test_cpu(int cpu, enum hk_flags flags)
{
- if (static_branch_unlikely(&housekeeping_overridden))
- if (housekeeping_flags & flags)
- return cpumask_test_cpu(cpu,
housekeeping_mask);
+ if (housekeeping_flags & flags)
+ return cpumask_test_cpu(cpu, housekeeping_mask);
return true;
}
EXPORT_SYMBOL_GPL(housekeeping_test_cpu);
--
1.8.3.1
On Tue, 23 Nov 2021 15:45:35 +0800
Aili Yao <[email protected]> wrote:
> From: Aili Yao <[email protected]>
>
> housekeeping_test_cpu is only called by housekeeping_cpu(),
> and in housekeeping_cpu(), there is already one same check;
>
> So delete the redundant check.
>
> Signed-off-by: Aili Yao <[email protected]>
> ---
> kernel/sched/isolation.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 7f06eaf..5c4d533 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -56,9 +56,8 @@ void housekeeping_affine(struct task_struct *t, enum
> hk_flags flags)
> bool housekeeping_test_cpu(int cpu, enum hk_flags flags)
> {
> - if (static_branch_unlikely(&housekeeping_overridden))
> - if (housekeeping_flags & flags)
> - return cpumask_test_cpu(cpu,
> housekeeping_mask);
Not only is your email client broken, you don't seem to understand what
static_branch_unlikely() is.
NACK.
-- Steve
> + if (housekeeping_flags & flags)
> + return cpumask_test_cpu(cpu, housekeeping_mask);
> return true;
> }
> EXPORT_SYMBOL_GPL(housekeeping_test_cpu);
On Tue, 23 Nov 2021 12:38:52 -0500
Steven Rostedt <[email protected]> wrote:
> On Tue, 23 Nov 2021 15:45:35 +0800
> Aili Yao <[email protected]> wrote:
>
> > From: Aili Yao <[email protected]>
> >
> > housekeeping_test_cpu is only called by housekeeping_cpu(),
> > and in housekeeping_cpu(), there is already one same check;
> >
> > So delete the redundant check.
> >
> > Signed-off-by: Aili Yao <[email protected]>
> > ---
> > kernel/sched/isolation.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > index 7f06eaf..5c4d533 100644
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -56,9 +56,8 @@ void housekeeping_affine(struct task_struct *t, enum
> > hk_flags flags)
> > bool housekeeping_test_cpu(int cpu, enum hk_flags flags)
> > {
> > - if (static_branch_unlikely(&housekeeping_overridden))
> > - if (housekeeping_flags & flags)
> > - return cpumask_test_cpu(cpu,
> > housekeeping_mask);
>
> Not only is your email client broken, you don't seem to understand what
> static_branch_unlikely() is.
Yes, My mail client is not properly configured, sorry for that, I will make it work.
And Yes again, I have limited knowledge about static key, But still don't understand why we
need two same check(jump or nop instruction?) here, could you be kindly to explain this?
A lot Thanks!
Aili Yao
On Wed, 24 Nov 2021 09:21:03 +0800
Aili Yao <[email protected]> wrote:
> On Tue, 23 Nov 2021 12:38:52 -0500
> Steven Rostedt <[email protected]> wrote:
>
> > On Tue, 23 Nov 2021 15:45:35 +0800
> > Aili Yao <[email protected]> wrote:
> >
> > > From: Aili Yao <[email protected]>
> > >
> > > housekeeping_test_cpu is only called by housekeeping_cpu(),
> > > and in housekeeping_cpu(), there is already one same check;
> > >
> > > So delete the redundant check.
> > >
> > > Signed-off-by: Aili Yao <[email protected]>
> > > ---
> > > kernel/sched/isolation.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > > index 7f06eaf..5c4d533 100644
> > > --- a/kernel/sched/isolation.c
> > > +++ b/kernel/sched/isolation.c
> > > @@ -56,9 +56,8 @@ void housekeeping_affine(struct task_struct *t, enum
> > > hk_flags flags)
> > > bool housekeeping_test_cpu(int cpu, enum hk_flags flags)
> > > {
> > > - if (static_branch_unlikely(&housekeeping_overridden))
> > > - if (housekeeping_flags & flags)
> > > - return cpumask_test_cpu(cpu,
> > > housekeeping_mask);
> >
> > Not only is your email client broken, you don't seem to understand what
> > static_branch_unlikely() is.
>
> Yes, My mail client is not properly configured, sorry for that, I will make it work.
>
> And Yes again, I have limited knowledge about static key, But still don't understand why we
> need two same check(jump or nop instruction?) here, could you be kindly to explain this?
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.
-- Steve
> 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 <[email protected]>
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 <[email protected]>
---
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
On Wed, 24 Nov 2021 15:42:14 +0800
Aili Yao <[email protected]> wrote:
> Signed-off-by: Aili Yao <[email protected]>
> ---
> 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);
Now you added an unneeded function call (which is also expensive).
There's a reason the static_branch was inlined.
-- Steve
> 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);