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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3346 bytes --]

On Fri, Mar 06, 2026 at 08:02:26AM +0100, Stefano Brivio wrote:
> 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.

Ok.

> > 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.

Yeah, fair point.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]