From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: tcp(4): use per-sockbuf mutex to protect `so_rcv' socket buffer
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 19 Dec 2024 22:32:59 +0100

On Sun, Dec 08, 2024 at 01:04:42AM +0300, Vitaliy Makkoveev wrote:
> Let's make tcp(4) reception path a little bit more parallel. This diff
> only unlocks soreceive() path, somove() path still locked exclusively.
> Also, exclusive socket lock will be taken each time before pri_rcvd()
> call in the soreceive() path.
> 
> We always hold both socket and sb_mtx locks while modifying
> SS_CANTRCVMORE bit, so socket lock is enough to check it the protocol
> input path.
> 
> To keep this diff small, I left sbmtx*() wrappers as is.

Just two naming issues below

OK bluhm@

> @@ -1869,13 +1874,19 @@ step6:
>  	 */
>  	if ((tiflags & TH_URG) && th->th_urp &&
>  	    TCPS_HAVERCVDFIN(tp->t_state) == 0) {
> +		u_long usage;
> +
>  		/*
>  		 * This is a kludge, but if we receive and accept
>  		 * random urgent pointers, we'll crash in
>  		 * soreceive.  It's hard to imagine someone
>  		 * actually wanting to send this much urgent data.
>  		 */
> -		if (th->th_urp + so->so_rcv.sb_cc > sb_max) {
> +		mtx_enter(&so->so_rcv.sb_mtx);
> +		usage = th->th_urp + so->so_rcv.sb_cc;
> +		mtx_leave(&so->so_rcv.sb_mtx);
> +
> +		if (usage > sb_max) {
>  			th->th_urp = 0;			/* XXX */
>  			tiflags &= ~TH_URG;		/* XXX */
>  			goto dodata;			/* XXX */

This is the urgent pointer.  It has nothing to do with usage.
Can you call the variable urgent?

> @@ -195,7 +195,7 @@ int
>  tcp_output(struct tcpcb *tp)
>  {
>  	struct socket *so = tp->t_inpcb->inp_socket;
> -	long len, win, txmaxseg;
> +	long len, win, sb_hiwat, txmaxseg;
>  	int off, flags, error;
>  	struct mbuf *m;
>  	struct tcphdr *th;
> @@ -373,7 +373,10 @@ again:
>  	if (off + len < so->so_snd.sb_cc)
>  		flags &= ~TH_FIN;
>  
> -	win = sbspace(so, &so->so_rcv);
> +	mtx_enter(&so->so_rcv.sb_mtx);
> +	win = sbspace_locked(so, &so->so_rcv);
> +	sb_hiwat = (long) so->so_rcv.sb_hiwat; 
> +	mtx_leave(&so->so_rcv.sb_mtx);
>  
>  	/*
>  	 * Sender silly window avoidance.  If connection is idle

When looking at the TCP code, it is important to know whether the
value is from the receive or send buffer.  Please rename variable
sb_hiwat to rcv_hiwat.

bluhm