Re: [PATCH v8 3/6] tcp: Add parameter struct ctx *c to tcp_timer_ctl()

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

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

On Fri, Nov 14, 2025 at 01:01:12AM +0100, Stefano Brivio wrote:
> On Mon, 10 Nov 2025 21:35:24 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Nov 10, 2025 at 05:31:34PM +0800, Yumei Huang wrote:
> > 
> > Should have a commit message explaining that it was recently removed,
> > and why you need it back.  Stefano might be able to add that on merge, though.
> 
> Even better: this should be part of the patch where tcp_timer_ctl()
> needs 'c' again, so that if we are bisecting stuff and end up exactly
> after this patch/commit, we don't get spurious static checkers warnings.
> 
> But we don't typically check those while bisecting (and I don't see any
> good reason to do so), so I don't really care.
> 
> Still, yes, mentioning that, say:
> 
> ---
> Commit x ("y") dropped the 'c' parameter to tcp_timer_ctl() but we'll
> need it again in the next commit, so add it back.
> ---
> 
> would be nice.
> 
> And yes, I can add it myself (even though it doesn't scale much, if I
> need to edit a few commit messages and add references for every
> series...), but looking at comments to 6/6 I think a respin might be
> convenient anyway.

Right.  Adjusting on merge would only make sense if there was nothing
else to polish in a respin.

> If you merge this change with the next patch you don't really need to
> explain it, by the way, as it's obvious from the rest of the commit.
> 
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>  
> > 
> > For the code itself,
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > > ---
> > >  tcp.c | 15 ++++++++-------
> > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tcp.c b/tcp.c
> > > index ca3d742..2f49327 100644
> > > --- a/tcp.c
> > > +++ b/tcp.c
> > > @@ -543,11 +543,12 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
> > >  
> > >  /**
> > >   * tcp_timer_ctl() - Set timerfd based on flags/events, create timerfd if needed
> > > + * @c:		Execution context
> > >   * @conn:	Connection pointer
> > >   *
> > >   * #syscalls timerfd_create timerfd_settime
> > >   */
> > > -static void tcp_timer_ctl(struct tcp_tap_conn *conn)
> > > +static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
> > >  {
> > >  	struct itimerspec it = { { 0 }, { 0 } };
> > >  
> > > @@ -631,7 +632,7 @@ void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn,
> > >  			 * flags and factor this into the logic below.
> > >  			 */
> > >  			if (flag == ACK_FROM_TAP_DUE)
> > > -				tcp_timer_ctl(conn);
> > > +				tcp_timer_ctl(c, conn);
> > >  
> > >  			return;
> > >  		}
> > > @@ -647,7 +648,7 @@ void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn,
> > >  	if (flag == ACK_FROM_TAP_DUE || flag == ACK_TO_TAP_DUE		  ||
> > >  	    (flag == ~ACK_FROM_TAP_DUE && (conn->flags & ACK_TO_TAP_DUE)) ||
> > >  	    (flag == ~ACK_TO_TAP_DUE   && (conn->flags & ACK_FROM_TAP_DUE)))
> > > -		tcp_timer_ctl(conn);
> > > +		tcp_timer_ctl(c, conn);
> > >  }
> > >  
> > >  /**
> > > @@ -702,7 +703,7 @@ void conn_event_do(const struct ctx *c, struct tcp_tap_conn *conn,
> > >  		tcp_epoll_ctl(c, conn);
> > >  
> > >  	if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED))
> > > -		tcp_timer_ctl(conn);
> > > +		tcp_timer_ctl(c, conn);
> > >  }
> > >  
> > >  /**
> > > @@ -1770,7 +1771,7 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> > >  				   seq, conn->seq_from_tap);
> > >  
> > >  			tcp_send_flag(c, conn, ACK);
> > > -			tcp_timer_ctl(conn);
> > > +			tcp_timer_ctl(c, conn);
> > >  
> > >  			if (p->count == 1) {
> > >  				tcp_tap_window_update(c, conn,
> > > @@ -2421,7 +2422,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
> > >  
> > >  	if (conn->flags & ACK_TO_TAP_DUE) {
> > >  		tcp_send_flag(c, conn, ACK_IF_NEEDED);
> > > -		tcp_timer_ctl(conn);
> > > +		tcp_timer_ctl(c, conn);
> > >  	} else if (conn->flags & ACK_FROM_TAP_DUE) {
> > >  		if (!(conn->events & ESTABLISHED)) {
> > >  			flow_dbg(conn, "handshake timeout");
> > > @@ -2443,7 +2444,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
> > >  				return;
> > >  
> > >  			tcp_data_from_sock(c, conn);
> > > -			tcp_timer_ctl(conn);
> > > +			tcp_timer_ctl(c, conn);
> > >  		}
> > >  	} else {
> > >  		struct itimerspec new = { { 0 }, { ACT_TIMEOUT, 0 } };
> > > -- 
> > > 2.51.0
> 
> -- 
> Stefano
> 

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