From: Alexander Bluhm <alexander.bluhm@gmx.net>
Subject: Re: Unsplice socket before start the teardown
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 15 Feb 2024 13:20:15 +0100

On Thu, Feb 15, 2024 at 12:28:28PM +0300, Vitaliy Makkoveev wrote:
> Use barriers instead of `ssp_idleto' task and timeout(9)
> re-initialization to wait concurrent somove() and soidle().

Why do you move the unsplice code before the solinger sleep?

Are you sure that you don't change the semantics?  Is all data still
spliced before the connection closes?

bluhm

> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.320
> diff -u -p -r1.320 uipc_socket.c
> --- sys/kern/uipc_socket.c	12 Feb 2024 22:48:27 -0000	1.320
> +++ sys/kern/uipc_socket.c	15 Feb 2024 09:23:28 -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 *);
>  
> @@ -316,36 +314,11 @@ sofree(struct socket *so, int keep_lock)
>  	sigio_free(&so->so_sigio);
>  	klist_free(&so->so_rcv.sb_klist);
>  	klist_free(&so->so_snd.sb_klist);
> -#ifdef SOCKET_SPLICE
> -	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);
> -	}
> -#endif /* SOCKET_SPLICE */
>  	sbrelease(so, &so->so_snd);
>  	sorflush(so);
>  	if (!keep_lock)
>  		sounlock(so);
> -#ifdef SOCKET_SPLICE
> -	if (so->so_sp) {
> -		/* Reuse splice idle, sounsplice() has been called before. */
> -		timeout_set_proc(&so->so_sp->ssp_idleto, soreaper, so);
> -		timeout_add(&so->so_sp->ssp_idleto, 0);
> -	} else
> -#endif /* SOCKET_SPLICE */
> -	{
> -		pool_put(&socket_pool, so);
> -	}
> +	pool_put(&socket_pool, so);
>  }
>  
>  static inline uint64_t
> @@ -371,6 +344,31 @@ soclose(struct socket *so, int flags)
>  	solock(so);
>  	/* Revoke async IO early. There is a final revocation in sofree(). */
>  	sigio_free(&so->so_sigio);
> +
> +#ifdef SOCKET_SPLICE
> +	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);
> +	}
> +	if (so->so_sp) {
> +		sounlock(so);
> +		taskq_barrier(sosplice_taskq);
> +		timeout_del_barrier(&so->so_sp->ssp_idleto);
> +		pool_put(&sosplice_pool, so->so_sp);
> +		solock(so);
> +	}
> +#endif /* SOCKET_SPLICE */
> +
>  	if (so->so_state & SS_ISCONNECTED) {
>  		if (so->so_pcb == NULL)
>  			goto discard;
> @@ -1441,32 +1439,6 @@ sotask(void *arg)
>  
>  	/* Avoid user land starvation. */
>  	yield();
> -}
> -
> -/*
> - * 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);
>  }
>  
>  /*