Re: [PATCH] tcp: Avoid comparison of expressions with different signedness in RTT_SET()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 5 Mar 2026 13:32:08 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Mar 04, 2026 at 05:32:32PM +0100, Stefano Brivio wrote:
> > With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine
> > on x86_64):
> > 
> > tcp.c: In function 'tcp_update_seqack_wnd':
> > util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare]
> >    40 | #define MIN(x, y)               (((x) < (y)) ? (x) : (y))
> >       |                                       ^
> > tcp_conn.h:63:26: note: in expansion of macro 'MIN'
> >    63 |         (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN))))
> >       |                          ^~~
> > tcp.c:1234:17: note: in expansion of macro 'RTT_SET'
> >  1234 |                 RTT_SET(conn, tinfo->tcpi_rtt);
> >       |                 ^~~~~~~
> > util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare]
> >    40 | #define MIN(x, y)               (((x) < (y)) ? (x) : (y))
> >       |                                                      ^~~
> > tcp_conn.h:63:26: note: in expansion of macro 'MIN'
> >    63 |         (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN))))
> >       |                          ^~~
> > tcp.c:1234:17: note: in expansion of macro 'RTT_SET'
> >  1234 |                 RTT_SET(conn, tinfo->tcpi_rtt);
> >       |                 ^~~~~~~
> > 
> > for some reason, that's not reported by gcc with glibc.
> > 
> > Cast the result of ilog2() to unsigned before using it, as it's always
> > positive the way we're using it. Should this ever break this for
> > whatever unlikely reason, RTT_EXP_MAX is the fallback value we want to
> > use anyway.
> > 
> > Fixes: 000601ba86da ("tcp: Adaptive interval based on RTT for socket-side acknowledgement checks")
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> This is correct, so
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> It's kind of inelegant, though - ilog2() only returns a signed to
> report an error case, which we avoid with the MAX inside the argument.
> That's not a super obvious connection.  I guess we could do:
> 
> 	(unsigned)MAX(0, ilog2(rtt / RTT_STORE_MIN))
> 
> which should be equivalent and makes it slightly more obvious that the
> cast is safe.  Not sure if it's really an improvement, though.

Right, yes, at this point I guess it's clearer. Changed to that in v2.

> This is the only user of ilog2(), so we could also consider replacing
> it with a version that explicitly clamps its argument to >= 1 and
> returns unsigned.

I thought about doing that, but on the other hand we would have a
rather arithmetic function returning an obviously wrong value for some
arguments, which doesn't sound great either.

-- 
Stefano