From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Make socket lock optional to sorele()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 3 Jan 2025 13:05:43 +0100

On Fri, Jan 03, 2025 at 01:53:53PM +0300, Vitaliy Makkoveev wrote:
> On Thu, Jan 02, 2025 at 06:04:11PM +0100, Alexander Bluhm wrote:
> > On Thu, Jan 02, 2025 at 12:42:42PM +0300, Vitaliy Makkoveev wrote:
> > > Previously solock() was required() to call sorele() only to make happy
> > > the assertion within sbrelease(). Now nothing within sorele() requires
> > > solock(), so it could be called lockless. So I propose to rename the
> > > `kkep_lock' arg of sorele to `no_unlock' and conceptually allow this.
> > 
> > I don't like boolean paramter that have no_ in their name.
> > 
> > Can we remove the keep_lock parameter and explicitly call sounlock()
> > before calling sorele() where needed?
> > 
> > sorele(so, 0) -> sounlock(so); sorele(so)
> > sorele(so, 1) -> sorele(so)
> > 
> 
> Makes sense. I do many of explicit sounlock() before sorele() with
> unlocked socket splicing, so the `keep_lock' arg became useless.

OK bluhm@

> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> diff -u -p -r1.354 uipc_socket.c
> --- sys/kern/uipc_socket.c	3 Jan 2025 09:59:25 -0000	1.354
> +++ sys/kern/uipc_socket.c	3 Jan 2025 10:48:19 -0000
> @@ -250,11 +250,8 @@ solisten(struct socket *so, int backlog)
>  }
>  
>  void
> -sorele(struct socket *so, int keep_lock)
> +sorele(struct socket *so)
>  {
> -	if (keep_lock == 0)
> -		sounlock(so);
> -
>  	if (refcnt_rele(&so->so_refcnt) == 0)
>  		return;
>  
> @@ -314,18 +311,23 @@ sofree(struct socket *so, int keep_lock)
>  
>  			if (so->so_onq != &head->so_q0) {
>  				sounlock(so);
> -				sorele(head, 0);
> +				sounlock(head);
> +				sorele(head);
>  				return;
>  			}
>  		}
>  
>  		soqremque(so, 0);
>  
> -		if (persocket)
> -			sorele(head, 0);
> +		if (persocket) {
> +			sounlock(head);
> +			sorele(head);
> +		}
>  	}
>  
> -	sorele(so, keep_lock);
> +	if (!keep_lock)
> +		sounlock(so);
> +	sorele(so);
>  }
>  
>  static inline uint64_t
> @@ -438,8 +440,7 @@ discard:
>  			sounlock(soback);
>  		}
>  		sbunlock(&soback->so_rcv);
> -		solock(soback);
> -		sorele(soback, 0);
> +		sorele(soback);
>  
>  notsplicedback:
>  		sblock(&so->so_rcv, SBL_WAIT | SBL_NOINTR);
> Index: sys/kern/uipc_socket2.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
> diff -u -p -r1.162 uipc_socket2.c
> --- sys/kern/uipc_socket2.c	30 Dec 2024 12:12:35 -0000	1.162
> +++ sys/kern/uipc_socket2.c	3 Jan 2025 10:48:19 -0000
> @@ -110,7 +110,8 @@ soisconnected(struct socket *so)
>  			solock(so);
>  
>  			if (so->so_onq != &head->so_q0) {
> -				sorele(head, 0);
> +				sounlock(head);
> +				sorele(head);
>  				return;
>  			}
>  
> @@ -121,8 +122,10 @@ soisconnected(struct socket *so)
>  		sorwakeup(head);
>  		wakeup_one(&head->so_timeo);
>  
> -		if (persocket)
> -			sorele(head, 0);
> +		if (persocket) {
> +			sounlock(head);
> +			sorele(head);
> +		}
>  	} else {
>  		wakeup(&so->so_timeo);
>  		sorwakeup(so);
> Index: sys/netinet/in_pcb.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> diff -u -p -r1.308 in_pcb.c
> --- sys/netinet/in_pcb.c	3 Jan 2025 00:49:26 -0000	1.308
> +++ sys/netinet/in_pcb.c	3 Jan 2025 10:48:19 -0000
> @@ -636,7 +636,7 @@ in_pcbsolock(struct inpcb *inp)
>  		return NULL;
>  
>  	rw_enter_write(&so->so_lock);
> -	sorele(so, 1);
> +	sorele(so);
>  
>  	return so;
>  }
> Index: sys/sys/socketvar.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/socketvar.h,v
> diff -u -p -r1.137 socketvar.h
> --- sys/sys/socketvar.h	3 Jan 2025 00:49:26 -0000	1.137
> +++ sys/sys/socketvar.h	3 Jan 2025 10:48:19 -0000
> @@ -425,7 +425,7 @@ int	socreate(int, struct socket **, int,
>  int	sodisconnect(struct socket *);
>  struct socket *soalloc(const struct protosw *, int);
>  void	sofree(struct socket *, int);
> -void	sorele(struct socket *, int);
> +void	sorele(struct socket *);
>  int	sogetopt(struct socket *, int, int, struct mbuf *);
>  void	sohasoutofband(struct socket *);
>  void	soisconnected(struct socket *);