[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Nicholas Mc Guire]

On Mon, Apr 16, 2018 at 10:54:26AM +0200, Philipp Klocke wrote:
> Make sched_feat(x) return 1 or 0 instead of 2**x or 0 when
> sysctl_sched_features is constant, by changing the left shift of the
> mask-bit to a right shift of the bitmap. The behaviour of the code
> remains unchanged.
> We prove this by showing that the compiler can evaluate this shift
> during compile time, which results in generating the same
> machine code as before.
> 
> This patch is motivated by the clang warning Wconstant-logical-operand,
> issued when logically comparing a variable to a constant integer that is
> neither 1 nor 0.  It happens for sched_feat(x) when sysctl_sched_features
> is constant, i.e., CONFIG_SCHED_DEBUG is not set.
> 
> kernel/sched/fair.c:3927:14: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
>         if (initial && sched_feat(START_DEBIT))
>                     ^  ~~~~~~~~~~~~~~~~~~~~~~~
> kernel/sched/fair.c:3927:14: note: use '&' for a bitwise operation
>         if (initial && sched_feat(START_DEBIT))
>                     ^~
>                     &
> kernel/sched/fair.c:3927:14: note: remove constant to silence this warning
>         if (initial && sched_feat(START_DEBIT))
>                    ~^~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This resolves the warning, leaves the code base largely as is.
> 
> Tested with gcc 7.3.1 and clang 6.0.0 on x86_64, comparing resulting
> binaries with diff.
> Applicable to all modern compilers and architectures
> 
> Signed-off-by: Philipp Klocke <Phil_K97@gmx.de>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>

> ---
>  kernel/sched/sched.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index fb5fc458547f..d2aedee6ab0f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1305,7 +1305,11 @@ static const_debug __maybe_unused unsigned int sysctl_sched_features =
>  	0;
>  #undef SCHED_FEAT
>  
> +#ifdef CONFIG_SCHED_DEBUG
>  #define sched_feat(x) (sysctl_sched_features & (1UL << __SCHED_FEAT_##x))
> +#else
> +#define sched_feat(x) ((sysctl_sched_features >> __SCHED_FEAT_##x) & 1UL)
> +#endif
>  

The changed sched_feat(y) line is fine but I do not get/like the 
of the ifdef - keeping the change minimal is ok if there is a
relevant impact but here there is no effective difference (you write
the object code is the same for the !CONFIG_SCHED_DEBUG case)
So I think the ifdef should be kicked here and the proposed change
seems fine to me.

>  #endif /* SCHED_DEBUG && HAVE_JUMP_LABEL */
>  
> -- 
> 2.17.0
> 

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Peter Zijlstra]

On Mon, Apr 16, 2018 at 10:54:26AM +0200, Philipp Klocke wrote:

> This patch is motivated by the clang warning Wconstant-logical-operand,
> issued when logically comparing a variable to a constant integer that is
> neither 1 nor 0.  It happens for sched_feat(x) when sysctl_sched_features
> is constant, i.e., CONFIG_SCHED_DEBUG is not set.
> 
> kernel/sched/fair.c:3927:14: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
>         if (initial && sched_feat(START_DEBIT))
>                     ^  ~~~~~~~~~~~~~~~~~~~~~~~
> kernel/sched/fair.c:3927:14: note: use '&' for a bitwise operation
>         if (initial && sched_feat(START_DEBIT))
>                     ^~
>                     &
> kernel/sched/fair.c:3927:14: note: remove constant to silence this warning
>         if (initial && sched_feat(START_DEBIT))
>                    ~^~~~~~~~~~~~~~~~~~~~~~~~~~


> @@ -1305,7 +1305,11 @@ static const_debug __maybe_unused unsigned int sysctl_sched_features =
>  	0;
>  #undef SCHED_FEAT
>  
> +#ifdef CONFIG_SCHED_DEBUG
>  #define sched_feat(x) (sysctl_sched_features & (1UL << __SCHED_FEAT_##x))
> +#else
> +#define sched_feat(x) ((sysctl_sched_features >> __SCHED_FEAT_##x) & 1UL)
> +#endif

So this is extra ugly, for no gain?

WTH does clang complain about a constant? Can't you just disable that
stupid warning?

Also, if sysctl_sched_features is a constant, the both expressions
_should_ really result in a constant and clang should still warn about
it.

I'm really not seeing why we'd want to do this. Just fix clang to not be
stupid.

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Peter Zijlstra]

Also, please don't cross-post with moderated lists, that's just
annoying.

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Philipp Klocke]

On 20.04.2018 09:57, Peter Zijlstra wrote:

> On Mon, Apr 16, 2018 at 10:54:26AM +0200, Philipp Klocke wrote:
>
>> This patch is motivated by the clang warning Wconstant-logical-operand,
>> issued when logically comparing a variable to a constant integer that is
>> neither 1 nor 0.  It happens for sched_feat(x) when sysctl_sched_features
>> is constant, i.e., CONFIG_SCHED_DEBUG is not set.
>>
>> kernel/sched/fair.c:3927:14: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
>>         if (initial && sched_feat(START_DEBIT))
>>                     ^  ~~~~~~~~~~~~~~~~~~~~~~~
>> kernel/sched/fair.c:3927:14: note: use '&' for a bitwise operation
>>         if (initial && sched_feat(START_DEBIT))
>>                     ^~
>>                     &
>> kernel/sched/fair.c:3927:14: note: remove constant to silence this warning
>>         if (initial && sched_feat(START_DEBIT))
>>                    ~^~~~~~~~~~~~~~~~~~~~~~~~~~
>> @@ -1305,7 +1305,11 @@ static const_debug __maybe_unused unsigned int sysctl_sched_features =
>>  	0;
>>  #undef SCHED_FEAT
>>  
>> +#ifdef CONFIG_SCHED_DEBUG
>>  #define sched_feat(x) (sysctl_sched_features & (1UL << __SCHED_FEAT_##x))
>> +#else
>> +#define sched_feat(x) ((sysctl_sched_features >> __SCHED_FEAT_##x) & 1UL)
>> +#endif
> So this is extra ugly, for no gain?
The gain is stopping a warning that clutters the output log of clang.
To improve readability, one can drop the ifdef-structure and just keep
the right shift version, like Nicholas suggested. This will have a (very
small)
impact on performance in CONFIG_SCHED_DEBUG case, but when
debugging, performance is no problem anyways.
> WTH does clang complain about a constant? Can't you just disable that
> stupid warning?
There are 2 ways to disable the warning. Either disable it for this
particular
occurrence, which clutters the code with #pragma's. THIS is really ugly.
Or disable it globally and maybe miss some important/helpful warnings.
> Also, if sysctl_sched_features is a constant, the both expressions
> _should_ really result in a constant and clang should still warn about
> it.
No, because clang only warns if the constant is neither 1 nor 0.
(These being the 'best' integer representations of booleans)
> I'm really not seeing why we'd want to do this. Just fix clang to not be
> stupid.
>

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Peter Zijlstra]

On Fri, Apr 20, 2018 at 06:29:07PM +0200, Philipp Klocke wrote:
> The gain is stopping a warning that clutters the output log of clang.

Well, you should not be using clang anyway. It is known to miscompile
the kernel.

> To improve readability, one can drop the ifdef-structure and just keep
> the right shift version, like Nicholas suggested. This will have a (very
> small)
> impact on performance in CONFIG_SCHED_DEBUG case, but when
> debugging, performance is no problem anyways.

See that is two bad choices.

> > Also, if sysctl_sched_features is a constant, the both expressions
> > _should_ really result in a constant and clang should still warn about
> > it.
> No, because clang only warns if the constant is neither 1 nor 0.
> (These being the 'best' integer representations of booleans)

Then won't something like so work?

#define sched_feat(x) !!(sysctl_sched_features & (1UL << __SCHED_FEAT_##x))

That forces it into _Bool space (0/1) and per the above rule will no
longer warn.

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Lukas Bulwahn]

On Fri, 20 Apr 2018, Peter Zijlstra wrote:

> On Fri, Apr 20, 2018 at 06:29:07PM +0200, Philipp Klocke wrote:
> > The gain is stopping a warning that clutters the output log of clang.
> 
> Well, you should not be using clang anyway. It is known to miscompile
> the kernel.
> 

There are some advantages of having a second compiler that can compile
the kernel (https://lwn.net/Articles/734071/). Some people in the kernel 
community and LLVM community are trying to get that to work.

We also want a zero-warning policy for clang, similar to gcc. 
Hence, this motivates to have a look at those few clang warnings and come 
up with patches for them.

This does not imply to make changes at any cost, and we need to determine 
a proper patch to either change the source code, disable the warning in 
the build script or annotate the file with some clang-specific pragmas.

To us, a minor change in the source sounded most reasonable after looking
at all three possible patches. Philipp might need another iteration, but
it generally looks sound to me if we get the details flattened out. 

Lukas

[PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case

[Peter Zijlstra]

On Fri, Apr 20, 2018 at 11:29:33PM +0200, Lukas Bulwahn wrote:
> 
> On Fri, 20 Apr 2018, Peter Zijlstra wrote:
> 
> > On Fri, Apr 20, 2018 at 06:29:07PM +0200, Philipp Klocke wrote:
> > > The gain is stopping a warning that clutters the output log of clang.
> > 
> > Well, you should not be using clang anyway. It is known to miscompile
> > the kernel.
> > 
> 
> There are some advantages of having a second compiler that can compile
> the kernel (https://lwn.net/Articles/734071/). Some people in the kernel 
> community and LLVM community are trying to get that to work.

Sure, not arguing against that. Just saying clang isn't there yet and it
has much bigger problems than a stray warning.

> We also want a zero-warning policy for clang, similar to gcc. 
> Hence, this motivates to have a look at those few clang warnings and come 
> up with patches for them.
> 
> This does not imply to make changes at any cost, and we need to determine 
> a proper patch to either change the source code, disable the warning in 
> the build script or annotate the file with some clang-specific pragmas.
> 
> To us, a minor change in the source sounded most reasonable after looking
> at all three possible patches. Philipp might need another iteration, but
> it generally looks sound to me if we get the details flattened out. 

Given the history of compiler warnings; I would really like to have some
text that explains why the warning is useful and should be worked
around.

To me the warning under discussion seems very dodgy and I would propose
to disable it entirely. Using a value other than 0/1 for boolean
expressions is fairly common, it being a compile time constant doesn't
seem to make much difference to me.