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: Thu, 2 Jan 2025 18:04:11 +0100

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)

> Now we have the only place within soclose() while solock() was taken
> just to only release it. Socket splicing is about buffers, which have
> their own locks, so solock() is not required in this layer, but pointers
> to socket should be safe for dereference, and soref() with sorele()
> are pretty enough for this.
> 
> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> diff -u -p -r1.352 uipc_socket.c
> --- sys/kern/uipc_socket.c	31 Dec 2024 12:19:46 -0000	1.352
> +++ sys/kern/uipc_socket.c	2 Jan 2025 09:31:12 -0000
> @@ -250,9 +250,9 @@ solisten(struct socket *so, int backlog)
>  }
>  
>  void
> -sorele(struct socket *so, int keep_lock)
> +sorele(struct socket *so, int no_unlock)
>  {
> -	if (keep_lock == 0)
> +	if (no_unlock == 0)
>  		sounlock(so);
>  
>  	if (refcnt_rele(&so->so_refcnt) == 0)
> @@ -438,8 +438,7 @@ discard:
>  			sounlock(soback);
>  		}
>  		sbunlock(&soback->so_rcv);
> -		solock(soback);
> -		sorele(soback, 0);
> +		sorele(soback, 1);
>  
>  notsplicedback:
>  		sblock(&so->so_rcv, SBL_WAIT | SBL_NOINTR);