Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
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.
Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
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
Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
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.
Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
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. >
Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
Also, please don't cross-post with moderated lists, that's just annoying.
Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
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.
Re: [PATCH] sched/fair: Change sched_feat(x) in !CONFIG_SCHED_DEBUG case
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 > Suggested-by: Lukas Bulwahn Reviewed-by: Nicholas Mc Guire > --- > 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 >
