On 28/05/2024 12.37, Mateusz Guzik wrote:
> I misplaced a closing ) in a patch using unlikely() and broke the
> comparison, see [1] for context.
>
> The fix was:
> - if (unlikely(abs(count + amount)) >= batch) {
> + if (unlikely(abs(count + amount) >= batch)) {
>
> Neither kernel build with W=1 nor C=1 (smatch) report the problem.
>
> Given that this compiles fine and for many cases with the bogus usage
> it also happens to produce the correct result, I have to suspect there
> are spots in the kernel with the problem.
>
> Either way, sounds like something smatch should be testing for.
Well, yes. A quick and dirty 'git grep' actually reveals something funny:
arch/x86/events/intel/core.c: return unlikely(event->hw.config &
HSW_IN_TX_CHECKPOINTED) != 0;
drivers/net/ethernet/freescale/dpaa/dpaa_eth.c: if (unlikely(fd_status &
FM_FD_STAT_RX_ERRORS) != 0) {
scripts/checkpatch.pl:# likely/unlikely comparisons similar to
"(likely(foo) > 0)"
So checkpatch should already have such a check, but running it on those
two files doesn't catch the instances; playing around a bit more
suggests that it only works when the argument to (un)likely has no
whitespace - so essentially, consists of a single identifier as in the
comment. My perl fu is far too weak to understand why that is.
Anyway, those two cases are probably harmless, since unlikely() ends up
returning a "normalized boolean" with the !!, and the end result
compared to 0. In both cases, one should probably just remove the '!= 0'
part.
Rasmus
