From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: tcp output MP fixes
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 8 Nov 2024 18:33:08 +0300

On Fri, Nov 08, 2024 at 04:20:41PM +0100, Alexander Bluhm wrote:
> On Wed, Nov 06, 2024 at 11:50:03AM +0100, Alexander Bluhm wrote:
> > Next goal is running TCP socket layer with shared netlock.  I went
> > through tcp_output() and looked for MP races.  Mostly documentation
> > and small fixes.
> 
> tcp_update_sndspace() and tcp_update_rcvspace() only read global
> variables that don't change.  Mark them as such.
> 
> And can we get braces consistently around the multi-line if blocks?
>

Yes please. 

> ok?
> 

ok mvs.

> bluhm
> 
> Index: netinet/tcp_usrreq.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_usrreq.c,v
> diff -u -p -r1.231 tcp_usrreq.c
> --- netinet/tcp_usrreq.c	12 Apr 2024 16:07:09 -0000	1.231
> +++ netinet/tcp_usrreq.c	8 Nov 2024 15:15:57 -0000
> @@ -102,15 +102,20 @@
>  #include <netinet6/in6_var.h>
>  #endif
>  
> +/*
> + * Locks used to protect global variables in this file:
> + *	I	immutable after creation
> + */
> +
>  #ifndef TCP_SENDSPACE
>  #define	TCP_SENDSPACE	1024*16
>  #endif
> -u_int	tcp_sendspace = TCP_SENDSPACE;
> +u_int	tcp_sendspace = TCP_SENDSPACE;		/* [I] */
>  #ifndef TCP_RECVSPACE
>  #define	TCP_RECVSPACE	1024*16
>  #endif
> -u_int	tcp_recvspace = TCP_RECVSPACE;
> -u_int	tcp_autorcvbuf_inc = 16 * 1024;
> +u_int	tcp_recvspace = TCP_RECVSPACE;		/* [I] */
> +u_int	tcp_autorcvbuf_inc = 16 * 1024;		/* [I] */
>  
>  const struct pr_usrreqs tcp_usrreqs = {
>  	.pru_attach	= tcp_attach,
> @@ -1516,13 +1521,14 @@ tcp_update_sndspace(struct tcpcb *tp)
>  		/* low on memory try to get rid of some */
>  		if (tcp_sendspace < nmax)
>  			nmax = tcp_sendspace;
> -	} else if (so->so_snd.sb_wat != tcp_sendspace)
> +	} else if (so->so_snd.sb_wat != tcp_sendspace) {
>  		/* user requested buffer size, auto-scaling disabled */
>  		nmax = so->so_snd.sb_wat;
> -	else
> +	} else {
>  		/* automatic buffer scaling */
>  		nmax = MIN(sb_max, so->so_snd.sb_wat + tp->snd_max -
>  		    tp->snd_una);
> +	}
>  
>  	/* a writable socket must be preserved because of poll(2) semantics */
>  	if (sbspace(so, &so->so_snd) >= so->so_snd.sb_lowat) {
> @@ -1556,10 +1562,10 @@ tcp_update_rcvspace(struct tcpcb *tp)
>  		/* low on memory try to get rid of some */
>  		if (tcp_recvspace < nmax)
>  			nmax = tcp_recvspace;
> -	} else if (so->so_rcv.sb_wat != tcp_recvspace)
> +	} else if (so->so_rcv.sb_wat != tcp_recvspace) {
>  		/* user requested buffer size, auto-scaling disabled */
>  		nmax = so->so_rcv.sb_wat;
> -	else {
> +	} else {
>  		/* automatic buffer scaling */
>  		if (tp->rfbuf_cnt > so->so_rcv.sb_hiwat / 8 * 7)
>  			nmax = MIN(sb_max, so->so_rcv.sb_hiwat +
>