On Tue, Dec 24, 2024 at 10:21:55PM +0300, Vitaliy Makkoveev wrote:
> This is the updated diff. It uses barriers instead of `ssp_idleto'
> timeout and `ssp_task' task redefinition.

I have tested this diff with my regression, performance, and network
setup.  Works fine.

OK bluhm@

Could you wait one day before commiting this?  Then we have a
snapshot between my and your unlocking diff.  If something goes
wrong, it will be easier to bisect.

> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> diff -u -p -r1.347 uipc_socket.c
> --- sys/kern/uipc_socket.c	19 Dec 2024 22:11:35 -0000	1.347
> +++ sys/kern/uipc_socket.c	24 Dec 2024 19:12:56 -0000
> @@ -62,8 +62,6 @@ int	sosplice(struct socket *, int, off_t
>  void	sounsplice(struct socket *, struct socket *, int);
>  void	soidle(void *);
>  void	sotask(void *);
> -void	soreaper(void *);
> -void	soput(void *);
>  int	somove(struct socket *, int);
>  void	sorflush(struct socket *);
>  
> @@ -327,17 +325,14 @@ sofree(struct socket *so, int keep_lock)
>  			sounlock(head);
>  	}
>  
> -	switch (so->so_proto->pr_domain->dom_family) {
> -	case AF_INET:
> -	case AF_INET6:
> -		if (so->so_proto->pr_type == SOCK_STREAM)
> -			break;
> -		/* FALLTHROUGH */
> -	default:
> +	if (!keep_lock) {
> +		/*
> +		 * sofree() was called from soclose(). Sleep is safe
> +		 * even for tcp(4) sockets.
> +		 */
>  		sounlock(so);
>  		refcnt_finalize(&so->so_refcnt, "sofinal");
>  		solock(so);
> -		break;
>  	}
>  
>  	sigio_free(&so->so_sigio);
> @@ -358,20 +353,20 @@ sofree(struct socket *so, int keep_lock)
>  		(*so->so_proto->pr_domain->dom_dispose)(so->so_rcv.sb_mb);
>  	m_purge(so->so_rcv.sb_mb);
>  
> -	if (!keep_lock)
> +	if (!keep_lock) {
>  		sounlock(so);
>  
>  #ifdef SOCKET_SPLICE
> -	if (so->so_sp) {
> -		/* Reuse splice idle, sounsplice() has been called before. */
> -		timeout_set_flags(&so->so_sp->ssp_idleto, soreaper, so,
> -		    KCLOCK_NONE, TIMEOUT_PROC | TIMEOUT_MPSAFE);
> -		timeout_add(&so->so_sp->ssp_idleto, 0);
> -	} else
> +		if (so->so_sp) {
> +			timeout_del_barrier(&so->so_sp->ssp_idleto);
> +			task_del(sosplice_taskq, &so->so_sp->ssp_task);
> +			taskq_barrier(sosplice_taskq);
> +			pool_put(&sosplice_pool, so->so_sp);
> +		}
>  #endif /* SOCKET_SPLICE */
> -	{
> -		pool_put(&socket_pool, so);
>  	}
> +
> +	pool_put(&socket_pool, so);
>  }
>  
>  static inline uint64_t
> @@ -450,39 +445,10 @@ drop:
>  		}
>  	}
>  discard:
> -	if (so->so_state & SS_NOFDREF)
> -		panic("soclose NOFDREF: so %p, so_type %d", so, so->so_type);
> -	so->so_state |= SS_NOFDREF;
> -
>  #ifdef SOCKET_SPLICE
>  	if (so->so_sp) {
>  		struct socket *soback;
>  
> -		if (so->so_proto->pr_flags & PR_WANTRCVD) {
> -			/*
> -			 * Copy - Paste, but can't relock and sleep in
> -			 * sofree() in tcp(4) case. That's why tcp(4)
> -			 * still rely on solock() for splicing and
> -			 * unsplicing.
> -			 */
> -
> -			if (issplicedback(so)) {
> -				int freeing = SOSP_FREEING_WRITE;
> -
> -				if (so->so_sp->ssp_soback == so)
> -					freeing |= SOSP_FREEING_READ;
> -				sounsplice(so->so_sp->ssp_soback, so, freeing);
> -			}
> -			if (isspliced(so)) {
> -				int freeing = SOSP_FREEING_READ;
> -
> -				if (so == so->so_sp->ssp_socket)
> -					freeing |= SOSP_FREEING_WRITE;
> -				sounsplice(so, so->so_sp->ssp_socket, freeing);
> -			}
> -			goto free;
> -		}
> -
>  		sounlock(so);
>  		mtx_enter(&so->so_snd.sb_mtx);
>  		/*
> @@ -530,8 +496,12 @@ notsplicedback:
>  
>  		solock(so);
>  	}
> -free:
>  #endif /* SOCKET_SPLICE */
> +
> +	if (so->so_state & SS_NOFDREF)
> +		panic("soclose NOFDREF: so %p, so_type %d", so, so->so_type);
> +	so->so_state |= SS_NOFDREF;
> +
>  	/* sofree() calls sounlock(). */
>  	sofree(so, 0);
>  	return (error);
> @@ -1532,8 +1502,7 @@ sosplice(struct socket *so, int fd, off_
>  void
>  sounsplice(struct socket *so, struct socket *sosp, int freeing)
>  {
> -	if ((so->so_proto->pr_flags & PR_WANTRCVD) == 0)
> -		sbassertlocked(&so->so_rcv);
> +	sbassertlocked(&so->so_rcv);
>  	soassertlocked(so);
>  
>  	task_del(sosplice_taskq, &so->so_splicetask);
> @@ -1587,17 +1556,23 @@ sotask(void *arg)
>  	 */
>  
>  	sblock(&so->so_rcv, SBL_WAIT | SBL_NOINTR);
> -	if (sockstream)
> -		solock(so);
> -
>  	if (so->so_rcv.sb_flags & SB_SPLICE) {
> -		if (sockstream)
> +		struct socket *sosp = so->so_sp->ssp_socket;
> +
> +		if (sockstream) {
> +			sblock(&sosp->so_snd, SBL_WAIT | SBL_NOINTR);
> +			solock(so);
>  			doyield = 1;
> +		}
> +
>  		somove(so, M_DONTWAIT);
> +
> +		if (sockstream) {
> +			sounlock(so);
> +			sbunlock(&sosp->so_snd);
> +		}
>  	}
>  
> -	if (sockstream)
> -		sounlock(so);
>  	sbunlock(&so->so_rcv);
>  
>  	if (doyield) {
> @@ -1607,32 +1582,6 @@ sotask(void *arg)
>  }
>  
>  /*
> - * The socket splicing task or idle timeout may sleep while grabbing the net
> - * lock.  As sofree() can be called anytime, sotask() or soidle() could access
> - * the socket memory of a freed socket after wakeup.  So delay the pool_put()
> - * after all pending socket splicing tasks or timeouts have finished.  Do this
> - * by scheduling it on the same threads.
> - */
> -void
> -soreaper(void *arg)
> -{
> -	struct socket *so = arg;
> -
> -	/* Reuse splice task, sounsplice() has been called before. */
> -	task_set(&so->so_sp->ssp_task, soput, so);
> -	task_add(sosplice_taskq, &so->so_sp->ssp_task);
> -}
> -
> -void
> -soput(void *arg)
> -{
> -	struct socket *so = arg;
> -
> -	pool_put(&sosplice_pool, so->so_sp);
> -	pool_put(&socket_pool, so);
> -}
> -
> -/*
>   * Move data from receive buffer of spliced source socket to send
>   * buffer of drain socket.  Try to move as much as possible in one
>   * big chunk.  It is a TCP only implementation.
> @@ -1652,8 +1601,10 @@ somove(struct socket *so, int wait)
>  
>  	if (sockdgram)
>  		sbassertlocked(&so->so_rcv);
> -	else
> +	else {
> +		sbassertlocked(&sosp->so_snd);
>  		soassertlocked(so);
> +	}
>  
>  	mtx_enter(&so->so_rcv.sb_mtx);
>  	mtx_enter(&sosp->so_snd.sb_mtx);