Re: [PATCH] sched/isolation: delete redundant housekeeping_overridden check

[Steven Rostedt]

On Wed, 24 Nov 2021 09:21:03 +0800
Aili Yao <yaoaili126@xxxxxxxxx> wrote:

> On Tue, 23 Nov 2021 12:38:52 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> > On Tue, 23 Nov 2021 15:45:35 +0800
> > Aili Yao <yaoaili126@xxxxxxxxx> wrote:
> >   
> > > From: Aili Yao <yaoaili@xxxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxx>
> > > ---
> > >  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