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

[Stefano Brivio <sbrivio@redhat.com>]

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.

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