Re: [PATCH 10/24] tcp: rename functions that manage buffers

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 6 Feb 2024 12:48:30 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Feb 02, 2024 at 03:11:37PM +0100, Laurent Vivier wrote:
> > To separate these functions from the ones specific to TCP management,
> > we are going to move it to a new file, but before that update their names
> > to reflect their role.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  passt.c |  2 +-
> >  tcp.c   | 84 ++++++++++++++++++++++++++++-----------------------------
> >  tcp.h   |  2 +-
> >  3 files changed, 44 insertions(+), 44 deletions(-)
> > 
> > diff --git a/passt.c b/passt.c
> > index 44d3a0b0548c..10042a9b9789 100644
> > --- a/passt.c
> > +++ b/passt.c
> > @@ -164,7 +164,7 @@ static void timer_init(struct ctx *c, const struct timespec *now)
> >   */
> >  void proto_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
> >  {
> > -	tcp_update_l2_buf(eth_d, eth_s);
> > +	tcp_buf_update_l2(eth_d, eth_s);
> >  	udp_update_l2_buf(eth_d, eth_s);
> >  }
> >  
> > diff --git a/tcp.c b/tcp.c
> > index cdbceed65033..640209533772 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -383,7 +383,7 @@ struct tcp6_l2_head {	/* For MSS6 macro: keep in sync with tcp6_l2_buf_t */
> >  #define ACK		(1 << 4)
> >  /* Flags for internal usage */
> >  #define DUP_ACK		(1 << 5)
> > -#define ACK_IF_NEEDED	0		/* See tcp_send_flag() */
> > +#define ACK_IF_NEEDED	0		/* See tcp_buf_send_flag() */
> >  
> >  #define OPT_EOL		0
> >  #define OPT_NOP		1
> > @@ -960,11 +960,11 @@ static uint16_t tcp_update_check_tcp6(struct ipv6hdr *ip6h)
> >  }
> >  
> >  /**
> > - * tcp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses
> > + * tcp_buf_update_l2() - Update L2 buffers with Ethernet and IPv4 addresses
> >   * @eth_d:	Ethernet destination address, NULL if unchanged
> >   * @eth_s:	Ethernet source address, NULL if unchanged
> >   */
> > -void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
> > +void tcp_buf_update_l2(const unsigned char *eth_d, const unsigned char *eth_s)
> >  {
> >  	int i;
> >  
> > @@ -982,10 +982,10 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
> >  }
> >  
> >  /**
> > - * tcp_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
> > + * tcp_buf_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
> >   * @c:		Execution context
> >   */
> > -static void tcp_sock4_iov_init(const struct ctx *c)
> > +static void tcp_buf_sock4_iov_init(const struct ctx *c)
> >  {
> >  	struct iphdr iph = L2_BUF_IP4_INIT(IPPROTO_TCP);
> >  	struct iovec *iov;
> > @@ -1014,10 +1014,10 @@ static void tcp_sock4_iov_init(const struct ctx *c)
> >  }
> >  
> >  /**
> > - * tcp_sock6_iov_init() - Initialise scatter-gather L2 buffers for IPv6 sockets
> > + * tcp_buf_sock6_iov_init() - Initialise scatter-gather L2 buffers for IPv6 sockets
> >   * @c:		Execution context
> >   */
> > -static void tcp_sock6_iov_init(const struct ctx *c)
> > +static void tcp_buf_sock6_iov_init(const struct ctx *c)
> >  {
> >  	struct iovec *iov;
> >  	int i;
> > @@ -1277,10 +1277,10 @@ static void tcp_rst_do(struct ctx *c, struct tcp_tap_conn *conn);
> >  	} while (0)
> >  
> >  /**
> > - * tcp_l2_flags_buf_flush() - Send out buffers for segments with no data (flags)
> > + * tcp_buf_l2_flags_flush() - Send out buffers for segments with no data (flags)
> >   * @c:		Execution context
> >   */
> > -static void tcp_l2_flags_buf_flush(const struct ctx *c)
> > +static void tcp_buf_l2_flags_flush(const struct ctx *c)
> >  {
> >  	tap_send_frames(c, tcp6_l2_flags_iov, tcp6_l2_flags_buf_used);
> >  	tcp6_l2_flags_buf_used = 0;
> > @@ -1290,10 +1290,10 @@ static void tcp_l2_flags_buf_flush(const struct ctx *c)
> >  }
> >  
> >  /**
> > - * tcp_l2_data_buf_flush() - Send out buffers for segments with data
> > + * tcp_buf_l2_data_flush() - Send out buffers for segments with data
> >   * @c:		Execution context
> >   */
> > -static void tcp_l2_data_buf_flush(const struct ctx *c)
> > +static void tcp_buf_l2_data_flush(const struct ctx *c)
> >  {
> >  	unsigned i;
> >  	size_t m;
> > @@ -1316,8 +1316,8 @@ static void tcp_l2_data_buf_flush(const struct ctx *c)
> >  /* cppcheck-suppress [constParameterPointer, unmatchedSuppression] */
> >  void tcp_defer_handler(struct ctx *c)
> >  {
> > -	tcp_l2_flags_buf_flush(c);
> > -	tcp_l2_data_buf_flush(c);
> > +	tcp_buf_l2_flags_flush(c);
> > +	tcp_buf_l2_data_flush(c);
> >  }
> >  
> >  static void tcp_set_tcp_header(struct tcphdr *th,
> > @@ -1629,7 +1629,7 @@ static int do_tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags,
> >  	return 1;
> >  }
> >  
> > -static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
> > +static int tcp_buf_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)  
> 
> The functions above could reasonably be said to be part of the buffer
> management, but I'm not convinced on this one - it's primary purpose
> is to, well, send a flag, so it uses the buffers, but I wouldn't
> really say it manages them.

Right, this and tcp_data_from_sock() below really implement TCP logic.
I'd be happy if we could avoid this patch and 11/24, but I didn't reach
the point where you need them, yet.

> >  {
> >  	size_t optlen = 0;
> >  	struct iovec *iov;
> > @@ -1664,7 +1664,7 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
> >  		}
> >  
> >  		if (tcp4_l2_flags_buf_used > ARRAY_SIZE(tcp4_l2_flags_buf) - 2)
> > -			tcp_l2_flags_buf_flush(c);
> > +			tcp_buf_l2_flags_flush(c);
> >  	} else {
> >  		struct tcp6_l2_flags_buf_t *b6;
> >  
> > @@ -1688,7 +1688,7 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
> >  		}
> >  
> >  		if (tcp6_l2_flags_buf_used > ARRAY_SIZE(tcp6_l2_flags_buf) - 2)
> > -			tcp_l2_flags_buf_flush(c);
> > +			tcp_buf_l2_flags_flush(c);
> >  	}
> >  
> >  	return 0;
> > @@ -1704,7 +1704,7 @@ static void tcp_rst_do(struct ctx *c, struct tcp_tap_conn *conn)
> >  	if (conn->events == CLOSED)
> >  		return;
> >  
> > -	if (!tcp_send_flag(c, conn, RST))
> > +	if (!tcp_buf_send_flag(c, conn, RST))
> >  		conn_event(c, conn, CLOSED);
> >  }
> >  
> > @@ -2024,7 +2024,7 @@ static void tcp_conn_from_tap(struct ctx *c,
> >  	} else {
> >  		tcp_get_sndbuf(conn);
> >  
> > -		if (tcp_send_flag(c, conn, SYN | ACK))
> > +		if (tcp_buf_send_flag(c, conn, SYN | ACK))
> >  			return;
> >  
> >  		conn_event(c, conn, TAP_SYN_ACK_SENT);
> > @@ -2100,7 +2100,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >  		iov = tcp4_l2_iov + tcp4_l2_buf_used++;
> >  		iov->iov_len = tap_iov_len(c, &b->taph, ip_len);
> >  		if (tcp4_l2_buf_used > ARRAY_SIZE(tcp4_l2_buf) - 1)
> > -			tcp_l2_data_buf_flush(c);
> > +			tcp_buf_l2_data_flush(c);
> >  	} else if (CONN_V6(conn)) {
> >  		struct tcp6_l2_buf_t *b = &tcp6_l2_buf[tcp6_l2_buf_used];
> >  
> > @@ -2112,12 +2112,12 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >  		iov = tcp6_l2_iov + tcp6_l2_buf_used++;
> >  		iov->iov_len = tap_iov_len(c, &b->taph, ip_len);
> >  		if (tcp6_l2_buf_used > ARRAY_SIZE(tcp6_l2_buf) - 1)
> > -			tcp_l2_data_buf_flush(c);
> > +			tcp_buf_l2_data_flush(c);
> >  	}
> >  }
> >  
> >  /**
> > - * tcp_data_from_sock() - Handle new data from socket, queue to tap, in window
> > + * tcp_buf_data_from_sock() - Handle new data from socket, queue to tap, in window  
> 
> Same with this one.
> 
> >   * @c:		Execution context
> >   * @conn:	Connection pointer
> >   *
> > @@ -2125,7 +2125,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >   *
> >   * #syscalls recvmsg
> >   */
> > -static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
> > +static int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
> >  {
> >  	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
> >  	int fill_bufs, send_bufs = 0, last_len, iov_rem = 0;
> > @@ -2169,7 +2169,7 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
> >  
> >  	if (( v4 && tcp4_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp4_l2_buf)) ||
> >  	    (!v4 && tcp6_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp6_l2_buf))) {
> > -		tcp_l2_data_buf_flush(c);
> > +		tcp_buf_l2_data_flush(c);
> >  
> >  		/* Silence Coverity CWE-125 false positive */
> >  		tcp4_l2_buf_used = tcp6_l2_buf_used = 0;
> > @@ -2195,7 +2195,7 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn)
> >  
> >  	if (!len) {
> >  		if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) == SOCK_FIN_RCVD) {
> > -			if ((ret = tcp_send_flag(c, conn, FIN | ACK))) {
> > +			if ((ret = tcp_buf_send_flag(c, conn, FIN | ACK))) {
> >  				tcp_rst(c, conn);
> >  				return ret;
> >  			}
> > @@ -2378,7 +2378,7 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
> >  			   max_ack_seq, conn->seq_to_tap);
> >  		conn->seq_ack_from_tap = max_ack_seq;
> >  		conn->seq_to_tap = max_ack_seq;
> > -		tcp_data_from_sock(c, conn);
> > +		tcp_buf_data_from_sock(c, conn);
> >  	}
> >  
> >  	if (!iov_i)
> > @@ -2394,14 +2394,14 @@ eintr:
> >  			 *   Then swiftly looked away and left.
> >  			 */
> >  			conn->seq_from_tap = seq_from_tap;
> > -			tcp_send_flag(c, conn, ACK);
> > +			tcp_buf_send_flag(c, conn, ACK);
> >  		}
> >  
> >  		if (errno == EINTR)
> >  			goto eintr;
> >  
> >  		if (errno == EAGAIN || errno == EWOULDBLOCK) {
> > -			tcp_send_flag(c, conn, ACK_IF_NEEDED);
> > +			tcp_buf_send_flag(c, conn, ACK_IF_NEEDED);
> >  			return p->count - idx;
> >  
> >  		}
> > @@ -2411,7 +2411,7 @@ eintr:
> >  	if (n < (int)(seq_from_tap - conn->seq_from_tap)) {
> >  		partial_send = 1;
> >  		conn->seq_from_tap += n;
> > -		tcp_send_flag(c, conn, ACK_IF_NEEDED);
> > +		tcp_buf_send_flag(c, conn, ACK_IF_NEEDED);
> >  	} else {
> >  		conn->seq_from_tap += n;
> >  	}
> > @@ -2424,7 +2424,7 @@ out:
> >  		 */
> >  		if (conn->seq_dup_ack_approx != (conn->seq_from_tap & 0xff)) {
> >  			conn->seq_dup_ack_approx = conn->seq_from_tap & 0xff;
> > -			tcp_send_flag(c, conn, DUP_ACK);
> > +			tcp_buf_send_flag(c, conn, DUP_ACK);
> >  		}
> >  		return p->count - idx;
> >  	}
> > @@ -2438,7 +2438,7 @@ out:
> >  
> >  		conn_event(c, conn, TAP_FIN_RCVD);
> >  	} else {
> > -		tcp_send_flag(c, conn, ACK_IF_NEEDED);
> > +		tcp_buf_send_flag(c, conn, ACK_IF_NEEDED);
> >  	}
> >  
> >  	return p->count - idx;
> > @@ -2474,8 +2474,8 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn,
> >  	/* The client might have sent data already, which we didn't
> >  	 * dequeue waiting for SYN,ACK from tap -- check now.
> >  	 */
> > -	tcp_data_from_sock(c, conn);
> > -	tcp_send_flag(c, conn, ACK);
> > +	tcp_buf_data_from_sock(c, conn);
> > +	tcp_buf_send_flag(c, conn, ACK);
> >  }
> >  
> >  /**
> > @@ -2555,7 +2555,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
> >  			conn->seq_from_tap++;
> >  
> >  			shutdown(conn->sock, SHUT_WR);
> > -			tcp_send_flag(c, conn, ACK);
> > +			tcp_buf_send_flag(c, conn, ACK);
> >  			conn_event(c, conn, SOCK_FIN_SENT);
> >  
> >  			return 1;
> > @@ -2566,7 +2566,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
> >  
> >  		tcp_tap_window_update(conn, ntohs(th->window));
> >  
> > -		tcp_data_from_sock(c, conn);
> > +		tcp_buf_data_from_sock(c, conn);
> >  
> >  		if (p->count - idx == 1)
> >  			return 1;
> > @@ -2596,7 +2596,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
> >  	if ((conn->events & TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_SENT)) {
> >  		shutdown(conn->sock, SHUT_WR);
> >  		conn_event(c, conn, SOCK_FIN_SENT);
> > -		tcp_send_flag(c, conn, ACK);
> > +		tcp_buf_send_flag(c, conn, ACK);
> >  		ack_due = 0;
> >  	}
> >  
> > @@ -2630,7 +2630,7 @@ static void tcp_connect_finish(struct ctx *c, struct tcp_tap_conn *conn)
> >  		return;
> >  	}
> >  
> > -	if (tcp_send_flag(c, conn, SYN | ACK))
> > +	if (tcp_buf_send_flag(c, conn, SYN | ACK))
> >  		return;
> >  
> >  	conn_event(c, conn, TAP_SYN_ACK_SENT);
> > @@ -2698,7 +2698,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c,
> >  
> >  	conn->wnd_from_tap = WINDOW_DEFAULT;
> >  
> > -	tcp_send_flag(c, conn, SYN);
> > +	tcp_buf_send_flag(c, conn, SYN);
> >  	conn_flag(c, conn, ACK_FROM_TAP_DUE);
> >  
> >  	tcp_get_sndbuf(conn);
> > @@ -2762,7 +2762,7 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
> >  		return;
> >  
> >  	if (conn->flags & ACK_TO_TAP_DUE) {
> > -		tcp_send_flag(c, conn, ACK_IF_NEEDED);
> > +		tcp_buf_send_flag(c, conn, ACK_IF_NEEDED);
> >  		tcp_timer_ctl(c, conn);
> >  	} else if (conn->flags & ACK_FROM_TAP_DUE) {
> >  		if (!(conn->events & ESTABLISHED)) {
> > @@ -2778,7 +2778,7 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
> >  			flow_dbg(conn, "ACK timeout, retry");
> >  			conn->retrans++;
> >  			conn->seq_to_tap = conn->seq_ack_from_tap;
> > -			tcp_data_from_sock(c, conn);
> > +			tcp_buf_data_from_sock(c, conn);
> >  			tcp_timer_ctl(c, conn);
> >  		}
> >  	} else {
> > @@ -2833,7 +2833,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
> >  			conn_event(c, conn, SOCK_FIN_RCVD);
> >  
> >  		if (events & EPOLLIN)
> > -			tcp_data_from_sock(c, conn);
> > +			tcp_buf_data_from_sock(c, conn);
> >  
> >  		if (events & EPOLLOUT)
> >  			tcp_update_seqack_wnd(c, conn, 0, NULL);
> > @@ -3058,10 +3058,10 @@ int tcp_init(struct ctx *c)
> >  		tc_hash[b] = FLOW_SIDX_NONE;
> >  
> >  	if (c->ifi4)
> > -		tcp_sock4_iov_init(c);
> > +		tcp_buf_sock4_iov_init(c);
> >  
> >  	if (c->ifi6)
> > -		tcp_sock6_iov_init(c);
> > +		tcp_buf_sock6_iov_init(c);
> >  
> >  	memset(init_sock_pool4,		0xff,	sizeof(init_sock_pool4));
> >  	memset(init_sock_pool6,		0xff,	sizeof(init_sock_pool6));
> > diff --git a/tcp.h b/tcp.h
> > index b9f546d31002..e7dbcfa2ddbd 100644
> > --- a/tcp.h
> > +++ b/tcp.h
> > @@ -23,7 +23,7 @@ int tcp_init(struct ctx *c);
> >  void tcp_timer(struct ctx *c, const struct timespec *now);
> >  void tcp_defer_handler(struct ctx *c);
> >  
> > -void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s);
> > +void tcp_buf_update_l2(const unsigned char *eth_d, const unsigned char *eth_s);
> >  
> >  /**
> >   * union tcp_epoll_ref - epoll reference portion for TCP connections  
> 

-- 
Stefano