[PATCH] sched/fair: Return true,false in voluntary_active_balance()

[PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Jason Yan]

Fix the following coccicheck warning:

kernel/sched/fair.c:9375:9-10: WARNING: return of 0/1 in function
'voluntary_active_balance' with return type bool

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 kernel/sched/fair.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b3bb4d6e49c3..e8390106ada4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9373,7 +9373,7 @@ voluntary_active_balance(struct lb_env *env)
 	struct sched_domain *sd = env->sd;
 
 	if (asym_active_balance(env))
-		return 1;
+		return true;
 
 	/*
 	 * The dst_cpu is idle and the src_cpu CPU has only 1 CFS task.
@@ -9385,13 +9385,13 @@ voluntary_active_balance(struct lb_env *env)
 	    (env->src_rq->cfs.h_nr_running == 1)) {
 		if ((check_cpu_capacity(env->src_rq, sd)) &&
 		    (capacity_of(env->src_cpu)*sd->imbalance_pct < capacity_of(env->dst_cpu)*100))
-			return 1;
+			return true;
 	}
 
 	if (env->migration_type == migrate_misfit)
-		return 1;
+		return true;
 
-	return 0;
+	return false;
 }
 
 static int need_active_balance(struct lb_env *env)
-- 
2.21.1

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Valentin Schneider]

On 07/05/20 12:06, Jason Yan wrote:
> Fix the following coccicheck warning:
>
> kernel/sched/fair.c:9375:9-10: WARNING: return of 0/1 in function
> 'voluntary_active_balance' with return type bool
>

It's perfectly safe to return 0/1 in a boolean function; that said seeing
as this is the second attempt at "fixing" this I'm tempted to say we should
pick it up...

> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  kernel/sched/fair.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b3bb4d6e49c3..e8390106ada4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9373,7 +9373,7 @@ voluntary_active_balance(struct lb_env *env)
>       struct sched_domain *sd = env->sd;
>
>       if (asym_active_balance(env))
> -		return 1;
> +		return true;
>
>       /*
>        * The dst_cpu is idle and the src_cpu CPU has only 1 CFS task.
> @@ -9385,13 +9385,13 @@ voluntary_active_balance(struct lb_env *env)
>           (env->src_rq->cfs.h_nr_running == 1)) {
>               if ((check_cpu_capacity(env->src_rq, sd)) &&
>                   (capacity_of(env->src_cpu)*sd->imbalance_pct < capacity_of(env->dst_cpu)*100))
> -			return 1;
> +			return true;
>       }
>
>       if (env->migration_type == migrate_misfit)
> -		return 1;
> +		return true;
>
> -	return 0;
> +	return false;
>  }
>
>  static int need_active_balance(struct lb_env *env)

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Steven Rostedt]

On Thu, 07 May 2020 12:17:36 +0100
Valentin Schneider <valentin.schneider@arm.com> wrote:

> On 07/05/20 12:06, Jason Yan wrote:
> > Fix the following coccicheck warning:
> >
> > kernel/sched/fair.c:9375:9-10: WARNING: return of 0/1 in function
> > 'voluntary_active_balance' with return type bool
> >  
> 
> It's perfectly safe to return 0/1 in a boolean function; that said seeing
> as this is the second attempt at "fixing" this I'm tempted to say we should
> pick it up...
> 

Actually, I disagree. We should push back on the check to not warn on 0/1
of boolean. Why is this a warning?

Fixes like this just add noise to the git history.

-- Steve

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Steven Rostedt]

On Thu, 7 May 2020 13:28:28 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > It's perfectly safe to return 0/1 in a boolean function; that said seeing
> > as this is the second attempt at "fixing" this I'm tempted to say we should
> > pick it up...
> >   
> 
> Actually, I disagree. We should push back on the check to not warn on 0/1
> of boolean. Why is this a warning?

If anything, we can teach people to try to understand their fixes, to see
if something is really a fix or not. Blindly accepting changes like this,
is no different than blindly submitting patches because some tool says its
an issue.

-- Steve

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Valentin Schneider]

On 07/05/20 18:30, Steven Rostedt wrote:
> On Thu, 7 May 2020 13:28:28 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> > It's perfectly safe to return 0/1 in a boolean function; that said seeing
>> > as this is the second attempt at "fixing" this I'm tempted to say we should
>> > pick it up...
>> >
>>
>> Actually, I disagree. We should push back on the check to not warn on 0/1
>> of boolean. Why is this a warning?
>
> If anything, we can teach people to try to understand their fixes, to see
> if something is really a fix or not. Blindly accepting changes like this,
> is no different than blindly submitting patches because some tool says its
> an issue.
>

I don't disagree. To play devil's advocate, AFAICT that one coccinelle script
is part of the kernel tree, so some folks may think it worth to reduce the
warnings we get from those.

To give my side of things, this one felt a bit like the
"s/borked/broken/" patches that folks send out because they have a
spellcheck linter, i.e. the change is purely cosmetic. But yeah, I suppose
less gunk to go through via git blame is preferable.

> -- Steve

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Joe Perches]

On Thu, 2020-05-07 at 13:30 -0400, Steven Rostedt wrote:
> On Thu, 7 May 2020 13:28:28 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > It's perfectly safe to return 0/1 in a boolean function; that said seeing
> > > as this is the second attempt at "fixing" this I'm tempted to say we should
> > > pick it up...
> > >   
> > 
> > Actually, I disagree. We should push back on the check to not warn on 0/1
> > of boolean. Why is this a warning?
> 
> If anything, we can teach people to try to understand their fixes, to see
> if something is really a fix or not. Blindly accepting changes like this,
> is no different than blindly submitting patches because some tool says its
> an issue.

<shrug>

Most people seem to prefer bool returns with apparent bool constants
even though true and false are enumerator constants (int) of 1 and 0
in the kernel.

from include/linux/stddef.h:

enum {
	false	= 0,
	true	= 1
};

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Steven Rostedt]

On Thu, 07 May 2020 10:55:33 -0700
Joe Perches <joe@perches.com> wrote:

> > If anything, we can teach people to try to understand their fixes, to see
> > if something is really a fix or not. Blindly accepting changes like this,
> > is no different than blindly submitting patches because some tool says its
> > an issue.  
> 
> <shrug>
> 
> Most people seem to prefer bool returns with apparent bool constants
> even though true and false are enumerator constants (int) of 1 and 0
> in the kernel.
> 
> from include/linux/stddef.h:
> 
> enum {
> 	false	= 0,
> 	true	= 1
> };

Sure, do that for new code, but we don't need these patches popping up for
current code. That is, it's a preference not a bug.

-- Steve

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Joe Perches]

On Thu, 2020-05-07 at 14:45 -0400, Steven Rostedt wrote:
> On Thu, 07 May 2020 10:55:33 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > If anything, we can teach people to try to understand their fixes, to see
> > > if something is really a fix or not. Blindly accepting changes like this,
> > > is no different than blindly submitting patches because some tool says its
> > > an issue.  
> > 
> > <shrug>
> > 
> > Most people seem to prefer bool returns with apparent bool constants
> > even though true and false are enumerator constants (int) of 1 and 0
> > in the kernel.
> > 
> > from include/linux/stddef.h:
> > 
> > enum {
> > 	false	= 0,
> > 	true	= 1
> > };
> 
> Sure, do that for new code, but we don't need these patches popping up for
> current code. That is, it's a preference not a bug.

People describe changes as a "fix" all the time for stuff
that isn't an actual fix for a logic defect but is instead
an update to a particular style preference.

Then the "fix" word causes the patch to be rather uselessly
applied to stable trees by AUTOSEL.

It's especially bad when the 'Fixes: <sha1> ("description")'
tag is also added.

It's a difficult thing to regulate and I don't believe a
good mechanism would be possible to add to checkpatch or
coccinelle to help isolate these things.

git diff -w sometimes helps, but that's not really a thing
that checkpatch could do.

Any suggestions?

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Steven Rostedt]

On Thu, 07 May 2020 12:06:56 -0700
Joe Perches <joe@perches.com> wrote:

> People describe changes as a "fix" all the time for stuff
> that isn't an actual fix for a logic defect but is instead
> an update to a particular style preference.
> 
> Then the "fix" word causes the patch to be rather uselessly
> applied to stable trees by AUTOSEL.
> 
> It's especially bad when the 'Fixes: <sha1> ("description")'
> tag is also added.
> 
> It's a difficult thing to regulate and I don't believe a
> good mechanism would be possible to add to checkpatch or
> coccinelle to help isolate these things.
> 
> git diff -w sometimes helps, but that's not really a thing
> that checkpatch could do.
> 
> Any suggestions?

I'm unfamiliar with how the coccinelle script is used, but I thought there
was some discussion some time back to have checkpatch not produces the same
kinds of warnings to code as it does to patches.

A lot of useless updates were being submitted when people were running
checkpatch on existing kernel code and producing warnings that are not
worth "fixing", but something that new code should try to avoid.

Basically, I'm fine with a warning that tells you that 1/0 is used for
boolean on code being submitted, but we really shouldn't be encouraging
people to change the code that currently exists with such updates.

-- Steve

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Joe Perches]

On Thu, 2020-05-07 at 15:45 -0400, Steven Rostedt wrote:
> On Thu, 07 May 2020 12:06:56 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > People describe changes as a "fix" all the time for stuff
> > that isn't an actual fix for a logic defect but is instead
> > an update to a particular style preference.
> > 
> > Then the "fix" word causes the patch to be rather uselessly
> > applied to stable trees by AUTOSEL.
> > 
> > It's especially bad when the 'Fixes: <sha1> ("description")'
> > tag is also added.
> > 
> > It's a difficult thing to regulate and I don't believe a
> > good mechanism would be possible to add to checkpatch or
> > coccinelle to help isolate these things.
> > 
> > git diff -w sometimes helps, but that's not really a thing
> > that checkpatch could do.
> > 
> > Any suggestions?
> 
> I'm unfamiliar with how the coccinelle script is used, but I thought there
> was some discussion some time back to have checkpatch not produces the same
> kinds of warnings to code as it does to patches.
> 
> A lot of useless updates were being submitted when people were running
> checkpatch on existing kernel code and producing warnings that are not
> worth "fixing", but something that new code should try to avoid.

checkpatch already has several blocks that look like

	if (input_is_a_patch)
		warn(...)
	else if (input_is_a_file)
		check(...)

where by default, check() is not output.

I've also suggested variations discouraging checkpatch
use on files outside of drivers/staging/ multiple times

https://lore.kernel.org/lkml/0753cae7829b98998ac3f5f9fcb52ba1f2475ee1.camel@perches.com/

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Peter Zijlstra]

On Thu, May 07, 2020 at 07:06:25PM +0800, Jason Yan wrote:
> Fix the following coccicheck warning:
> 
> kernel/sched/fair.c:9375:9-10: WARNING: return of 0/1 in function
> 'voluntary_active_balance' with return type bool

That's not a warning, that's a broken cocinelle script, which if these
stupid patches don't stop, I'm going to delete myself!

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Rasmus Villemoes]

On 08/05/2020 10.16, Peter Zijlstra wrote:
> On Thu, May 07, 2020 at 07:06:25PM +0800, Jason Yan wrote:
>> Fix the following coccicheck warning:
>>
>> kernel/sched/fair.c:9375:9-10: WARNING: return of 0/1 in function
>> 'voluntary_active_balance' with return type bool
> 
> That's not a warning, that's a broken cocinelle script, which if these
> stupid patches don't stop, I'm going to delete myself!


I was wondering why I got cc'ed here. Then it dawned on me. What can I
say, I was young.

Ack on nuking it.

Rasmus

Re: [PATCH] sched/fair: Return true,false in voluntary_active_balance()

[Julia Lawall]

On Fri, 8 May 2020, Peter Zijlstra wrote:

> On Thu, May 07, 2020 at 07:06:25PM +0800, Jason Yan wrote:
> > Fix the following coccicheck warning:
> >
> > kernel/sched/fair.c:9375:9-10: WARNING: return of 0/1 in function
> > 'voluntary_active_balance' with return type bool
>
> That's not a warning, that's a broken cocinelle script, which if these
> stupid patches don't stop, I'm going to delete myself!

Peter,

If you don't like the script, please feel free to delete it.

Currently, no one is really maintaining (ie pushing patches to Linus) the
scripts directory.  Masahiro Yamada was doing it but has expressed several
times that he doesn't want to.  I got a kernel.org account, but clearly
haven't had time to figure out how to use it appropriately.  But simply
deleting a file is simple and risk free, so if you want to make the
change please go ahead.

julia