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.
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.
participants (2)
-
Peter Zijlstra -
Philipp Klocke