* [PATCH] sched/isolation: delete redundant housekeeping_overridden check @ 2021-11-23 7:45 Aili Yao 2021-11-23 17:38 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Aili Yao @ 2021-11-23 7:45 UTC (permalink / raw) To: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman Cc: linux-kernel, yaoaili From: Aili Yao <yaoaili@kingsoft.com> 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 <yaoaili@kingsoft.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/isolation: delete redundant housekeeping_overridden check 2021-11-23 7:45 [PATCH] sched/isolation: delete redundant housekeeping_overridden check Aili Yao @ 2021-11-23 17:38 ` Steven Rostedt 2021-11-24 1:21 ` Aili Yao 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2021-11-23 17:38 UTC (permalink / raw) To: Aili Yao Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, bsegall, mgorman, linux-kernel, yaoaili On Tue, 23 Nov 2021 15:45:35 +0800 Aili Yao <yaoaili126@gmail.com> wrote: > From: Aili Yao <yaoaili@kingsoft.com> > > 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 <yaoaili@kingsoft.com> > --- > 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/isolation: delete redundant housekeeping_overridden check 2021-11-23 17:38 ` Steven Rostedt @ 2021-11-24 1:21 ` Aili Yao 2021-11-24 1:42 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Aili Yao @ 2021-11-24 1:21 UTC (permalink / raw) To: Steven Rostedt Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, bsegall, mgorman, linux-kernel, yaoaili On Tue, 23 Nov 2021 12:38:52 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Tue, 23 Nov 2021 15:45:35 +0800 > Aili Yao <yaoaili126@gmail.com> wrote: > > > From: Aili Yao <yaoaili@kingsoft.com> > > > > 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 <yaoaili@kingsoft.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/isolation: delete redundant housekeeping_overridden check 2021-11-24 1:21 ` Aili Yao @ 2021-11-24 1:42 ` Steven Rostedt 2021-11-24 7:42 ` Aili Yao 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2021-11-24 1:42 UTC (permalink / raw) To: Aili Yao Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, bsegall, mgorman, linux-kernel, yaoaili On Wed, 24 Nov 2021 09:21:03 +0800 Aili Yao <yaoaili126@gmail.com> wrote: > On Tue, 23 Nov 2021 12:38:52 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Tue, 23 Nov 2021 15:45:35 +0800 > > Aili Yao <yaoaili126@gmail.com> wrote: > > > > > From: Aili Yao <yaoaili@kingsoft.com> > > > > > > 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 <yaoaili@kingsoft.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/isolation: delete redundant housekeeping_overridden check 2021-11-24 1:42 ` Steven Rostedt @ 2021-11-24 7:42 ` Aili Yao 2021-11-24 14:13 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Aili Yao @ 2021-11-24 7:42 UTC (permalink / raw) To: Steven Rostedt Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, bsegall, mgorman, linux-kernel, yaoaili > 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@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/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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/isolation: delete redundant housekeeping_overridden check 2021-11-24 7:42 ` Aili Yao @ 2021-11-24 14:13 ` Steven Rostedt 0 siblings, 0 replies; 6+ messages in thread From: Steven Rostedt @ 2021-11-24 14:13 UTC (permalink / raw) To: Aili Yao Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann, bsegall, mgorman, linux-kernel, yaoaili On Wed, 24 Nov 2021 15:42:14 +0800 Aili Yao <yaoaili126@gmail.com> wrote: > 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/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); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-24 14:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-23 7:45 [PATCH] sched/isolation: delete redundant housekeeping_overridden check Aili Yao 2021-11-23 17:38 ` Steven Rostedt 2021-11-24 1:21 ` Aili Yao 2021-11-24 1:42 ` Steven Rostedt 2021-11-24 7:42 ` Aili Yao 2021-11-24 14:13 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).